cloudspannerecosystem / wrench

wrench - Schema management tool for Cloud Spanner -
MIT License
236 stars 46 forks source link

Replace spansql #84

Closed apstndb closed 1 year ago

apstndb commented 1 year ago

WHAT

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

WHY

83

TODO

kazegusuri commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

kazegusuri commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

I would like to correct one factual error. ParseDML was added for wrench itself. https://github.com/googleapis/google-cloud-go/pull/6349

(Added) Originally, UPDATE, DELETE, and INSERT was only supported by ParseQuery. https://github.com/googleapis/google-cloud-go/commit/00dd38a7a500bb363c15864ae83ac62c5146fdfb https://github.com/googleapis/google-cloud-go/commit/1dec6f6a31768a3f70bfec7274828301c22ea10b https://github.com/googleapis/google-cloud-go/commit/c6185cffc7f23741ac4a230aadee74b3def85ced

apstndb commented 1 year ago

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. https://github.com/apstndb/wrench/commit/2bd13506ff45f9a688dae4696ea70fc9c76d8acf

apstndb commented 1 year ago

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.