fix comment handling #150
fix comment handling #150
Conversation
separator.go
Outdated
|
|
||
| // no comment found | ||
| if terminate == "" { | ||
| // emit space if comments are found and the next character is not a whitespace |
There was a problem hiding this comment.
I have two questions.
- Initially, this was a little bit difficult for me to understand because the block under
if found && i < len(s.str) {is executed in the 2nd iteration of the outer loop. Doesn't it work if we just puts.sb.WriteRune(' ')at here and here? Both lines are points where the comment termination found. - It looks like the whitespace is not inserted if the next character is a whitespace. Do we need to care about whether the next character is a whitespace or not? Why not simply converting "a comment" to the "whitespace"?
There was a problem hiding this comment.
I think we need to discuss about expected behavior as a design decision.
- If we can think only token separation.
- We can always put the whitespace.
- If we need to concern about query text in query stats, audit logs, etc.
- We may need to define some expected behaviors.
example1.
'SELECT 1# comment' → 'SELECT 1' or 'SELECT 1 '?
Need to consider about end of string?
This space will be always trimmed because of strings.TrimSpace
example2 from test case
# comment;
SELECT /* comment */ 1; --comment
SELECT 2;/* comment */
Can it be interpret as SELECT 1 and SELECT 2?
There was a problem hiding this comment.
Correction: example1 will not be SELECT 1 because strings.TrimSpace is applied
https://github.com/cloudspannerecosystem/spanner-cli/blob/v0.9.16/separator.go#L290
There was a problem hiding this comment.
The current implementation is already stripping the comment from the query, so I think users won't expect the query is intact when writing a comment in the query string.
Having said that, it might be good idea to change the current implementation to keep the comment in query string. Then we can defer to the Spanner server side for the handling of comment characters in a query string. (It looks like Spanner keeps the comment characters in query stats.)
There was a problem hiding this comment.
I agree.
Currently, Cloud Spanner rejects comments in DDL so comment strip is needed for DDLs,
but DML and SELECT statements can process comments and there are motivations to keep the comment.
Note: I have some experimental implementation of comment preserving separator which is derived on this statement separator.
https://github.com/apstndb/gsqlsep
There was a problem hiding this comment.
May be a solution.
- Separate queries (preserving comments)
- Strip comments and detect kind of statement
- a. execute stripped DDL
b. execute raw DML/SELECT
There was a problem hiding this comment.
Sounds good! Is it possible to implement the solution in https://github.com/apstndb/gsqlsep and switch to use it from spanner-cli?
There was a problem hiding this comment.
I'd like to PoC using gsqlsep.
|
I have experimentally replaced statement separator with gsqlsep and implement comment preserving(only |
cli.go
Outdated
| @@ -328,13 +329,16 @@ func readInteractiveInput(rl *readline.Instance, prompt string) (*inputStatement | |||
| } | |||
| input += line + "\n" | |||
|
|
|||
| statements := separateInput(input) | |||
| statements := gsqlsep.SeparateInputPreserveComments(input, `\G`) | |||
There was a problem hiding this comment.
| statements := gsqlsep.SeparateInputPreserveComments(input, `\G`) | |
| statements := gsqlsep.SeparateInputPreserveComments(input, delimiterVertical) |
might be better? Same for others.
cli.go
Outdated
| @@ -516,3 +520,7 @@ func handleInterrupt(cancel context.CancelFunc) { | |||
| <-c | |||
| cancel() | |||
| } | |||
|
|
|||
| func stripComments(input string) string { | |||
There was a problem hiding this comment.
Just a random idea.
Random idea 1
What do you think about changing gsqlsep.InputStatement as follows,
type InputStatement struct {
Statement string
StatementWithoutComment string
Terminator string
}
and use StatementWithoutComment for input match and Statement for other use case? Then we don't need to call gsqlsep.Separate... functions in two difference places.
Random idea 2
What do you think about adding StripComments() method to gsqlsep.InputStatement?
| return `\G` | ||
| } | ||
| return "" | ||
| delim string | ||
| } | ||
|
|
||
| func separateInput(input string) []inputStatement { |
There was a problem hiding this comment.
If we keep this function, why not calling this function in other places? (not directly call gsqlsep functions)
|
I added InputStatement.StripComments into gsqlsep and enhanced |
separator.go
Outdated
| s.sb.Reset() | ||
| } | ||
| var result []inputStatement | ||
| for _, stmt := range gsqlsep.SeparateInputPreserveComments(input, `\G`) { |
There was a problem hiding this comment.
| for _, stmt := range gsqlsep.SeparateInputPreserveComments(input, `\G`) { | |
| for _, stmt := range gsqlsep.SeparateInputPreserveComments(input, delimiterVertical) { |
separator.go
Outdated
| } | ||
| var result []inputStatement | ||
| for _, stmt := range gsqlsep.SeparateInputPreserveComments(input, `\G`) { | ||
| stmt := stmt |
There was a problem hiding this comment.
What is the reason we need to capture this in a local variable?
There was a problem hiding this comment.
I don't think it is needed.
This PR changes comment handling in the statement separator.
Comments are replaced by a single whitespace if the next character is not whitespaces.
It may be better to check previous character of the comment but it is not easy without changing structure.
fixes #149