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

Replace spansql #84

Closed
wants to merge 3 commits into from

Conversation

apstndb
Copy link

@apstndb apstndb commented Mar 10, 2023

WHAT

Replace spansql with statement separator which is derived from spanner-cli implementation.

WHY

#83

TODO

  • Add tests

@kazegusuri
Copy link
Collaborator

hmm, this seems a similar implementation in spansql package. This is another kind of parser I think so it's not good to have the implementation inside each OSS.
It is better to have an independent library that contains this kind of parser. Or I think it's enough if spansql supports this feature. The current problem is that spansql does not support parsing raw statements without parsing the details of the statement.
So if spansql provides parseRawStatements() function that returns raw statements as array of string I think it's the best.

@apstndb
Copy link
Author

apstndb commented Mar 12, 2023

It is not a Cloud Spanner client library so I don't think it can be merged into https://github.com/googleapis/google-cloud-go.
spansql was born to implement spannertest but they doesn't need this implementation.

IMHO, it can be maintained as a standalone repo in cloudspannerecosystem.
Otherwise I don't want wrench to depend on another personal repo.

Note:
spanner-cli needs to handle ‘\G‘ separators so they can't reuse the statement separator implementation without some kind of a extention point.

@apstndb
Copy link
Author

apstndb commented Mar 12, 2023

It might be possible to describe it as a helper function for using UpdateDDL and Batch DML.

@kazegusuri
Copy link
Collaborator

It is not a Cloud Spanner client library so I don't think it can be merged into https://github.com/googleapis/google-cloud-go.
spansql was born to implement spannertest but they doesn't need this implementation.

It seems most of the recent changes to spansql are not related to spannertest.

@apstndb
Copy link
Author

apstndb commented Mar 12, 2023

It seems most of the recent changes to spansql are not related to spannertest.

I think there is a difference between that and this case because ParseDDL, ParseDML, and ParseQuery is already exported functions and they usually accept PRs to fix them.

The last update by Googler seems to be over a year old.
https://github.com/googleapis/google-cloud-go/commits/main/spanner/spansql

@apstndb
Copy link
Author

apstndb commented Mar 12, 2023

I would like to correct one factual error.
ParseDML was added for wrench itself.
googleapis/google-cloud-go#6349

(Added) Originally, UPDATE, DELETE, and INSERT was only supported by ParseQuery.
googleapis/google-cloud-go@00dd38a
googleapis/google-cloud-go@1dec6f6
googleapis/google-cloud-go@c6185cf

@apstndb
Copy link
Author

apstndb commented Mar 13, 2023

spanner-cli needs to handle \G separators so they can't reuse the statement separator implementation without some kind of a extention point.

I have implemented customizable separator for demonstrating purpose.
apstndb@2bd1350

@apstndb apstndb closed this Mar 14, 2023
@apstndb
Copy link
Author

apstndb commented Mar 14, 2023

After internal discussion, this PR was closed as it has no chance of being approved unless there is a place to maintain the statement separator.

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