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

Edge case: Support query expression starting with paren as SelectStatement #196

Closed
apstndb opened this issue Nov 10, 2024 · 4 comments · Fixed by #197
Closed

Edge case: Support query expression starting with paren as SelectStatement #196

apstndb opened this issue Nov 10, 2024 · 4 comments · Fixed by #197

Comments

@apstndb
Copy link
Collaborator

apstndb commented Nov 10, 2024

I have found a edge case that query expression starting with paren is a valid statement, but it has failed in spanner-cli.

spanner> (SELECT 1);
ERROR: invalid statement

In gcloud spanner databases execute-sql, it has succeeded.

$ gcloud spanner databases execute-sql ${SPANNER_DATABASE} --sql '(SELECT 1)'                           
(Unspecified)
1

https://cloud.google.com/spanner/docs/reference/standard-sql/query-syntax#sql_syntax

query_statement:
  [ statement_hint_expr ]
  [ table_hint_expr ]
  [ join_hint_expr ]
  query_expr

query_expr:
  [ WITH cte[, ...] ]
  { select | ( query_expr ) | set_operation }
  [ ORDER BY expression [{ ASC | DESC }] [, ...] ]
  [ LIMIT count [ OFFSET skip_rows ] ]

I expects it will be important when pipe syntax is available.

// rewrite from traditional subquery
(
SELECT * FROM Singers
...
) |> CALL ML.PREDICT(...)

(In this case, paren is actually not required.)

@apstndb apstndb changed the title Edge case: support query expression with paren as SelectStatement Edge case: Support query expression starts with paren as SelectStatement Nov 10, 2024
@apstndb apstndb changed the title Edge case: Support query expression starts with paren as SelectStatement Edge case: Support query expression starting with paren as SelectStatement Nov 10, 2024
@yfuruyama
Copy link
Collaborator

yfuruyama commented Nov 12, 2024

Thank you for pointing it out. I created a quick fix on the current regular expression: #197.

If we'll find a critical limitation of current regular expression style to match the statement, we may need to move to hybrid approach with query parsing (using like memefish) & regular expression to support some spanner-cli unique statements (e.g. BEGIN, COMMIT, EXPLAIN, etc).

@apstndb
Copy link
Collaborator Author

apstndb commented Nov 12, 2024

Yeah, I think there is no problem to use regex to statement kind detection, but it is possible to use memefish.

@yfuruyama
Copy link
Collaborator

Currently, the type (Query/DDL/DML) of all native Google SQL statements can be determined just by looking at the first non-hint token.

If we can detect the statement just by using lexer and looking at the first non-hint token, I think it's not much difference than using regular expression.

Maybe we can come back this topic again once we find the critical limitation of current regex style.

@apstndb
Copy link
Collaborator Author

apstndb commented Nov 12, 2024

Yeah ,I don't think it is a significant difference.
When regular DMLs support statement hints, we might reconsider about regex. (Still it can be like @\{.*\} (SELECT|UPDATE|INSERT|DELETE)\s+)

  • (Currently, statement hints are only in partitioned DMLs Support Partitioned DML with statement hints #193 )
  • Additionally, we can replace comment strip logic with lexer based implementation.
    • But spanner-cli can't replace statement separation logic because of \G support.

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 a pull request may close this issue.

2 participants