sql: don't use the grammar to enforce expression const-ness in AOST #27206
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
This was referenced Jul 5, 2018
knz
changed the title
maddyblue
reviewed
Jul 9, 2018
| @@ -23,6 +23,15 @@ SELECT * FROM t AS OF SYSTEM TIME INTERVAL '-1ns' | |||
| ---- | |||
| 2 | |||
|
|
|||
| # Verify that we can use computed expressions. | |||
There was a problem hiding this comment.
Add another test with AOST now() to verify non-constant expressions don't work.
|
Also could you remove the restriction in to make sure this fixes the pretty print round trip tests? |
craig bot
pushed a commit
that referenced
this pull request
Jul 9, 2018
27212: server: clarify TestReportUsage r=knz a=knz Needed to clarify this and remove the magic constant "16" while working on #27206. Release note: None Co-authored-by: Raphael 'kena' Poss <[email protected]>
knz
commented
Jul 9, 2018
There was a problem hiding this comment.
Also could you remove the restriction in cockroach/pkg/testutils/sqlutils/pretty.go Line 30 in ca89c4f
Done
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/sql/logictest/testdata/logic_test/as_of, line 26 at r1 (raw file):
Previously, mjibson (Matt Jibson) wrote…
Add another test with
AOST now()to verify non-constant expressions don't work.
Done.
maddyblue
approved these changes
Jul 9, 2018
There was a problem hiding this comment.
Reviewable status:
|
Thank you!
bors r+
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
|
Merge conflict |
Prior to this patch, AS OF SYSTEM TIME relied on the grammar to
restrict its operand to just constants. This was an unfortunate
approach because it prevented other constant expressions like `('a' ||
'b')::INTERVAL` (or, encountered during tests, `'1ms':::INTERVAL`).
This patch corrects this situation by relaxing the grammar constraint
and instead verifying the const-ness of the expression after type
checking.
Release note (sql change): AS OF SYSTEM TIME can now use some more
complex expressions to compute the desired time stamp.
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Jul 10, 2018
27206: sql: don't use the grammar to enforce expression const-ness in AOST r=knz a=knz Fixes #26976. Prior to this patch, AS OF SYSTEM TIME relied on the grammar to restrict its operand to just constants. This was an unfortunate approach because it prevented other constant expressions like `('a' || 'b')::INTERVAL` (or, encountered during tests, `'1ms':::INTERVAL`). This patch corrects this situation by relaxing the grammar constraint and instead verifying the const-ness of the expression after type checking. Release note (sql change): AS OF SYSTEM TIME can now use some more complex expressions to compute the desired time stamp. Co-authored-by: Raphael 'kena' Poss <[email protected]>
Build succeeded |
craig bot
pushed a commit
that referenced
this pull request
Jul 12, 2018
27214: sql/parser: remove the now-unused a_expr_const non-terminal r=knz a=knz First 3 commits from #27212, #27206 and #27213 27434: roachtest: skip tests run on expired clusters r=benesch a=petermattis Check to see if the cluster is expired before starting a test. Tests are skipped if the cluster is expired which prevents 1 timed out test from causing a cascade of issues to be posted for subsequent subtests. Fixes #27166 Release note: None Co-authored-by: Raphael 'kena' Poss <[email protected]> Co-authored-by: Peter Mattis <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #26976.
Prior to this patch, AS OF SYSTEM TIME relied on the grammar to
restrict its operand to just constants. This was an unfortunate
approach because it prevented other constant expressions like
('a' || 'b')::INTERVAL(or, encountered during tests,'1ms':::INTERVAL).This patch corrects this situation by relaxing the grammar constraint
and instead verifying the const-ness of the expression after type
checking.
Release note (sql change): AS OF SYSTEM TIME can now use some more
complex expressions to compute the desired time stamp.