Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add --ignore-unsupported-statements flag #116

Conversation

nktks
Copy link
Contributor

@nktks nktks commented Mar 10, 2023

WHAT

  • This PR adds --ignore-unsupported-statements flag to v2 yo generate command.

Context

When user manages several DDLs in one schema file,
user should filter DDLs to pass yo generate which yo doesn't support.
This makes it difficult for user to use DDLs that yo does not support.

--ignore-unsupported-statements flag enables yo to skip these statements, and continue to generate codes.
And if the user really made a mistake in supported syntax in DDL such as CREATE TABLE, CREATE INDEX, ALTER TABLE, this case still failed.
Therefore, I don't think adding this flag will make it easy for users to make problems.
How do you think?

ref:
cloudspannerecosystem/wrench#83

I agreeed CLA.

Thank you!

@nktks nktks changed the title add --skip-invalid-statements option add --skip-invalid-statements flag Mar 10, 2023
@nktks nktks force-pushed the feature/add-skip-invalid-statement-option branch from ebd00cb to 23dc7f2 Compare March 10, 2023 09:57
@nktks nktks marked this pull request as ready for review March 10, 2023 10:00
@nktks nktks changed the title add --skip-invalid-statements flag add --ignore-unsupported-statements flag Mar 10, 2023
@kazegusuri
Copy link
Collaborator

It seems the improvement of statements separation is good but it's the same problem I commented in the issue in the wrench repo.
For the feature for ignoring unsupported statements, I'm not sure is useful or not. If we can ignore the statements, we have another problem that we cannot use the tables in yo.

@nktks
Copy link
Contributor Author

nktks commented Mar 11, 2023

It seems the improvement of statements separation is good but it's the same problem I commented in the issue in the wrench repo.

I think so too.
It will be good to have separating stmt implementation in spansql or something other place like under cloudspannerechosystem.

For the feature for ignoring unsupported statements, I'm not sure is useful or not.

I think, if user has below file:

CREATE ROLE hoge; -- want to use fgac role, doesn't supported yo yet.
CREATE CHANGE STREAM All FOR ALL; -- want to use change stream, doesn't supported yo yet.
CREATE TABLE A (id INT64 NOT NULL,) PRIMARY KEY(id); -- want to use yo for code generation

currently, yo generate will fail, and with --ignore-unsupported-statements enables to generate code related table A.

If we can ignore the statements, we have another problem that we cannot use the tables in yo.

If we use new feature in CREATE TABLE DDL which yo(spansql) doesn't support,
yo generate still fail same as current version, so I think another problem will not occur.

Or do you mean another problem occurs?

@nktks
Copy link
Contributor Author

nktks commented Mar 13, 2023

I modified code:

  • stop to use copied statement separation
  • Fixed to still fail on statements that spansql doesn't support, and not fail only on statements that spansql supports but yo doesn't support.

@nktks
Copy link
Contributor Author

nktks commented Feb 13, 2024

I close this ticket.
Instead I'll create another PR to skip CREATE CHANGE STREAM statements.

@nktks nktks closed this Feb 13, 2024
@nktks nktks deleted the feature/add-skip-invalid-statement-option branch February 13, 2024 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants