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

feat(spanner/spansql): support table_hint_expr at from_clause on query_statement #4457

Merged
merged 4 commits into from Jul 20, 2021

Conversation

nktks
Copy link
Contributor

@nktks nktks commented Jul 19, 2021

This fixes parse error when query statement includes table hint expr.
This add function parseHints and use in parsing table hints and join hints.

query_statement

This fixes parse error when query statement includes table hint expr.
This add function parseHints and use in parsing table hints and join
hints.
@nktks nktks requested review from hengfengli, skuruppu and a team as code owners July 19, 2021 06:38
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Spanner API. label Jul 19, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 19, 2021
@hengfengli
Copy link
Contributor

@olavloite Can you please review this PR? Thanks.

Copy link
Contributor

@olavloite olavloite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. This looks mostly good to me, but it seems that the documentation is not 100% correct on whether multiple table hints are allowed or not. Cloud Spanner actually does support more than one table hint, so we should try to implement it in the same way in this emulator. See my comments and example below.

@@ -2111,6 +2111,13 @@ func (p *parser) parseSelectFrom() (SelectFrom, *parseError) {
return nil, err
}
sf := SelectFromTable{Table: tname}
if p.eat("@") {
hint, err := p.parseHints(map[string]string{}, false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... It seems that the documentation is slightly off here. It is actually possible to have more than one table hint, so the variable here should be renamed to hints and the parseHints(..) function should be called with true rather than false.

For example the following query is accepted by Cloud Spanner:

select *
from albums@{FORCE_INDEX=_BASE_TABLE, GROUPBY_SCAN_OPTIMIZATION=FALSE}

if err != nil {
return nil, err
}
sf.Hint = hint
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: same as above, rename to sf.Hints

spanner/spansql/parser_test.go Show resolved Hide resolved
if len(sft.Hint) > 0 {
for k, v := range sft.Hint {
str += fmt.Sprintf("@{%s=%s}", k, v)
// table hint exists only one key
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, it seems that the documentation is not completely clear on this, but Cloud Spanner does accept more than one table hint.

@@ -394,6 +394,7 @@ type SelectFrom interface {
type SelectFromTable struct {
Table ID
Alias ID // empty if not aliased
Hint map[string]string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rename to Hints

@nktks
Copy link
Contributor Author

nktks commented Jul 19, 2021

Thanks for your kind review.
I checked multiple table hint keys can use at my spanner instance.
And I fixed to parseHints always parse multiple keys.

@nktks nktks requested a review from olavloite July 19, 2021 09:39
Copy link
Contributor

@olavloite olavloite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM

@olavloite
Copy link
Contributor

@hengfengli I think we need an approval from either you or @skuruppu in order to merge this.

@hengfengli hengfengli added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 19, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 19, 2021
spanner/spansql/sql.go Outdated Show resolved Hide resolved
@hengfengli hengfengli added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 19, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 19, 2021
@hengfengli
Copy link
Contributor

The integration test fails due to #4450. I will merge this PR after it is merged.

@hengfengli hengfengli added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 20, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 20, 2021
@hengfengli hengfengli merged commit 7047808 into googleapis:master Jul 20, 2021
@hengfengli
Copy link
Contributor

@nakatamixi Thank you for your contribution!

@nktks
Copy link
Contributor Author

nktks commented Jul 20, 2021

Thank you!

@nktks nktks deleted the table-hint branch July 20, 2021 03:08
nktks pushed a commit to nktks/google-cloud-go that referenced this pull request Jul 21, 2021
googleapis#4457 made unstable SQL string, and fail test sometimes like this.
```
--- FAIL: TestSQL (0.00s)
    sql_test.go:410: {{false [A] [{Table  map[FORCE_INDEX:Idx GROUPBY_SCAN_OPTIMIZATION:TRUE]}] {4 B b <nil>} [] [] []} [] <nil> <nil>}.SQL() wrong.
         got SELECT A FROM Table@{GROUPBY_SCAN_OPTIMIZATION=TRUE,FORCE_INDEX=Idx} WHERE B = @b
        want SELECT A FROM Table@{FORCE_INDEX=Idx,GROUPBY_SCAN_OPTIMIZATION=TRUE} WHERE B = @b
```
Because map range result is unstable.
This PR makes the SQL output stable by sorting the Hints keys.
nktks pushed a commit to nktks/google-cloud-go that referenced this pull request Jul 21, 2021
googleapis#4457 made unstable SQL string, and fail test sometimes like this.
```
--- FAIL: TestSQL (0.00s)
    sql_test.go:410: {{false [A] [{Table  map[FORCE_INDEX:Idx GROUPBY_SCAN_OPTIMIZATION:TRUE]}] {4 B b <nil>} [] [] []} [] <nil> <nil>}.SQL() wrong.
         got SELECT A FROM Table@{GROUPBY_SCAN_OPTIMIZATION=TRUE,FORCE_INDEX=Idx} WHERE B = @b
        want SELECT A FROM Table@{FORCE_INDEX=Idx,GROUPBY_SCAN_OPTIMIZATION=TRUE} WHERE B = @b
```
Because map range result is unstable.
This PR makes the SQL output stable by sorting the Hints keys.
hengfengli pushed a commit that referenced this pull request Jul 21, 2021
#4457 made unstable SQL string, and fail test sometimes like this.
Because map range result is unstable.
This PR makes the SQL output stable by sorting the Hints keys.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants