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.

Sign up for GitHub

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

Jump to bottom

sql: support scalar expressions (& placeholders) for alter range ... relocate #73807

Merged
merged 3 commits into from
Dec 15, 2021
Merged

sql: support scalar expressions (& placeholders) for alter range ... relocate #73807

merged 3 commits into from
Dec 15, 2021

Conversation

knz
Copy link
Contributor

@knz knz commented Dec 14, 2021 •
edited
Loading

Fixes #73315.
See the individual commits for details.

@knz knz requested review from arulajmani and lunevalex December 14, 2021 19:04
@knz knz requested a review from a team as a code owner December 14, 2021 19:04
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor Author

knz commented Dec 14, 2021

@rafiss @otan do you have some yacc-fu that'd be useful here? By defining clauses as FROM a_expr TO a_expr, we get a shift/reduce conflict on TO inside the interval syntax (e.g. INTERVAL DAYS TO MONTHS). Is there a way to ask yacc to prefer shifting in that case?

@knz knz force-pushed the 20211213-kv-alter2 branch from 88161fe to 22f0af4 Compare December 14, 2021 19:21
@knz knz requested a review from a team as a code owner December 14, 2021 19:21
@otan
Copy link
Contributor

otan commented Dec 14, 2021

you could try use %prec or maybe even consider b_expr or below (or force parenthesis around FROM/TO)

@knz
Copy link
Contributor Author

knz commented Dec 14, 2021

you could try use %prec

can you explain how that works? Or maybe link to a doc that explains?

or maybe even consider b_expr or below

unfortunately the interval syntax is in d_expr (the narrowest one)

or force parenthesis around FROM/TO

yeah I thought about that, and if there's no solution via %prec, I'll put the choice to a vote on the KV team: either TO y FROM x, or FROM (x) TO (y)

@otan
Copy link
Contributor

otan commented Dec 14, 2021

Best I can find is https://www.ibm.com/docs/en/zos/2.3.0?topic=section-precedence-in-grammar-rules, I only have a vague understanding of it cargo culting from pg

RaduBerinde
RaduBerinde reviewed Dec 14, 2021
View reviewed changes
Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

:lgtm: modulo the syntax discussion

If we support placeholders now, it would be good to have some tests for that. I can't find many existing tests for RELOCATE, maybe we can add some similar to TestSplitAt

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani and @lunevalex)

lunevalex
lunevalex approved these changes Dec 14, 2021
View reviewed changes
Copy link
Collaborator

@lunevalex lunevalex left a comment

Choose a reason for hiding this comment

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

LGTM

@knz
Copy link
Contributor Author

knz commented Dec 14, 2021

Thanks to the goyacc source code, I found out that it uses a super old algorithm that only supports real token names in %prec (not pseudo-tokens) for the shifting rule. Once I understood that, I was able to fix the syntax.

@knz knz force-pushed the 20211213-kv-alter2 branch from 22f0af4 to 4ce7404 Compare December 14, 2021 21:16
@knz
Copy link
Contributor Author

knz commented Dec 14, 2021

For future reference, I troubleshooted my yacc problem with the following mini-grammar:

%token RELOCATE FROM TO FOR SELECT DAY TO MINUTE

%left INTERVAL_SHORT
%left TO

%%

top:
   RELOCATE FROM a TO a FOR SELECT

a: b

b: DAY           %prec INTERVAL_SHORT
 | DAY TO MINUTE %prec TO

otan
otan reviewed Dec 14, 2021
View reviewed changes
pkg/ccl/logictestccl/testdata/logic_test/tenant_unsupported Outdated
@@ -73,10 +73,10 @@ statement error operation is unsupported in multi-tenancy mode
ALTER RANGE RELOCATE LEASE TO 2 FOR SELECT range_id from crdb_internal.ranges where table_name = 'kv'

statement error operation is unsupported in multi-tenancy mode
ALTER RANGE 1 RELOCATE FROM 1 TO 2
ALTER RANGE 1 RELOCATE TO 2 FROM 1
Copy link
Contributor

Choose a reason for hiding this comment

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

revert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Done.

@knz
Copy link
Contributor Author

knz commented Dec 14, 2021 •
edited
Loading

For @RaduBerinde:

If we support placeholders now, it would be good to have some tests for that

Good point. I modified the existing test in kvserver/client_lease_test.go to exercise the new code path.

@knz knz force-pushed the 20211213-kv-alter2 branch from 4ce7404 to 2a162b8 Compare December 14, 2021 21:26
@blathers-crl blathers-crl bot requested a review from otan December 14, 2021 21:26
@knz knz force-pushed the 20211213-kv-alter2 branch from 2a162b8 to f5e30c7 Compare December 14, 2021 21:30
@knz knz changed the title sql: support scalar expressions (& placeholders) for relocate store IDs sql: support scalar expressions (& placeholders) for alter range ... relocate Dec 14, 2021
@knz
Copy link
Contributor Author

knz commented Dec 14, 2021

I have extended the PR with a 2nd commit that also adds placeholder support in the first position, so that this PR closes the linked issue entirely.

Please take another look.

@knz knz force-pushed the 20211213-kv-alter2 branch from 249d818 to d84d380 Compare December 14, 2021 22:06
otan
otan approved these changes Dec 14, 2021
View reviewed changes
Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

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

lgtm, some small test failure that look like simple rewrites

@knz knz force-pushed the 20211213-kv-alter2 branch from d84d380 to 3ddfc4f Compare December 15, 2021 05:54
@knz
Copy link
Contributor Author

knz commented Dec 15, 2021

TFYRs!

bors r=lunevalex,otan

@knz
Copy link
Contributor Author

knz commented Dec 15, 2021

bors r-

@craig
Copy link
Contributor

craig bot commented Dec 15, 2021

Canceled.

@knz
Copy link
Contributor Author

knz commented Dec 15, 2021

bors r=lunevalex,otan

knz added 3 commits December 15, 2021 07:22
@knz
sql: support scalar expressions (& placeholders) for relocate store IDs
9c96ac2 
This commit makes it possible to use arbitrary scalar
expressions (incl subqueries, placeholders etc) in the FROM/TO clauses
of ALTER RANGE ... RELOCATE statements.

For example:

```
ALTER RANGE 123 RELOCATE FROM $1 TO $2
ALTER RANGE 123 RELOCATE FROM $1 TO $1+10
```

Release note (sql change): The experimental ALTER RANGE...RELOCATE
syntax now accepts arbitrary scalar expressions as the source and target
store IDs.

Release note (sql change): The output of `EXPLAIN ALTER RANGE
... RELOCATE` now includes the source and target store IDs.
@knz
sql: support scalar expressions for the simple range ID in relocate
760e1f4 
This commit makes it possible to use arbitrary scalar
expressions (incl subqueries, placeholders etc) in the RANGE clause
of ALTER RANGE ... RELOCATE statements.

For example:

```
ALTER RANGE $1 RELOCATE FROM 3 TO 4
```

Release note (sql change): The experimental ALTER RANGE...RELOCATE
syntax now accepts arbitrary scalar expressions as the range ID
when the FOR clause is not used.
@knz
sql/opt/exec: include replica subject in EXPLAIN ALTER RANGE RELOCATE
77f169d 
Release note (sql change): The output of `EXPLAIN ALTER RANGE
... RELOCATE` now includes which replicas are subject to the relocation.
@knz knz force-pushed the 20211213-kv-alter2 branch from 7d190dc to 77f169d Compare December 15, 2021 06:22
@craig
Copy link
Contributor

craig bot commented Dec 15, 2021

Canceled.

@knz
Copy link
Contributor Author

knz commented Dec 15, 2021

bors r=lunevalex,otan

@craig
Copy link
Contributor

craig bot commented Dec 15, 2021

Build succeeded:

@craig craig bot merged commit f6f60c6 into cockroachdb:master Dec 15, 2021
This was referenced Dec 15, 2021
Open
Open
Open
@knz knz deleted the 20211213-kv-alter2 branch December 15, 2021 09:06
@RaduBerinde
Copy link
Member

Opt changes LGTM.

@knz knz mentioned this pull request Jun 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaduBerinde RaduBerinde RaduBerinde left review comments

@lunevalex lunevalex lunevalex approved these changes

@otan otan otan approved these changes

@arulajmani arulajmani Awaiting requested review from arulajmani

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kv: add placeholders to ALTER RANGE RELOCATE
5 participants
@knz @cockroach-teamcity @otan @RaduBerinde @lunevalex