opt: Add array and tuple constant folding rules #37404
Conversation
Move all folding rules to fold_constants.opt. Move all custom functions to a new fold_constants.go file. Consolidate tests. Release note: None
|
This change is |
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @justinj, @RaduBerinde, and @rytaft)
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/norm/testdata/rules/fold_constants, line 839 at r2 (raw file):
# NOTE: Use constant array access to avoid triggering ColumnAccess::TypeCheck # constant tuple folding. opt
[nit] expect=FoldColumnAccess ?
pkg/sql/opt/norm/testdata/rules/fold_constants, line 868 at r2 (raw file):
├── key: () ├── fd: ()-->(1) └── ('foo',) [type=tuple{string}]
Maybe add a test case where we get an evaluation error? (argument could be crdb_internal.force_error('', 'some-error'))
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1, 3 of 3 files at r2.
Reviewable status:
pkg/sql/opt/norm/fold_constants.go, line 276 at r2 (raw file):
// Case 1: The input is a static tuple constructor. if tup, ok := input.(*memo.TupleExpr); ok { return tup.Elems[idx]
What if idx is outside the bounds of the tuple (similar to the array case above)? Is that even possible?
pkg/sql/opt/norm/fold_constants.go, line 283 at r2 (raw file):
datum := memo.ExtractConstDatum(input) colName := input.DataType().TupleLabels()[idx]
same here
pkg/sql/opt/norm/testdata/rules/fold_constants, line 847 at r2 (raw file):
├── scan a └── projections └── const: 'foo' [type=string]
It seems odd that this results in a scalar while the query below results in a tuple... Is that expected?
|
pkg/sql/opt/norm/testdata/rules/fold_constants, line 847 at r2 (raw file): Previously, rytaft wrote…
Never mind I see why |
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/norm/fold_constants.go, line 276 at r2 (raw file):
Previously, rytaft wrote…
What if
idxis outside the bounds of the tuple (similar to the array case above)? Is that even possible?
It's not possible, because the index is derived during type checking according to the static tuple type.
pkg/sql/opt/norm/testdata/rules/fold_constants, line 868 at r2 (raw file):
Previously, RaduBerinde wrote…
Maybe add a test case where we get an evaluation error? (argument could be
crdb_internal.force_error('', 'some-error'))
The evaluation only happens for DTuple. It should never fail, but I put the error check there for completeness. Adding force_error forces "Case 1", where the input is a static tuple constructor (and we don't eval the tuple field exprs).
|
pkg/sql/opt/norm/testdata/rules/fold_constants, line 868 at r2 (raw file): Previously, andy-kimball (Andy Kimball) wrote…
Ah, thanks, that makes sense. |
|
bors r+ |
|
bors r- |
Canceled |
Add FoldIndirection and FoldColumnAccess rules to fold expressions like this: ARRAY[i, i+1][1] ARRAY[1, 2, 3][2] (((i, i+1) as foo, bar)).foo (((1, 2) as foo, bar)).bar Release note: None
|
bors r+ |
37404: opt: Add array and tuple constant folding rules r=andy-kimball a=andy-kimball Add FoldIndirection and FoldColumnAccess rules to fold expressions like this: ARRAY[i, i+1][1] ARRAY[1, 2, 3][2] (((i, i+1) as foo, bar)).foo (((1, 2) as foo, bar)).bar Release note: None Co-authored-by: Andrew Kimball <andyk@cockroachlabs.com>
Build succeeded |
Add FoldIndirection and FoldColumnAccess rules to fold expressions
like this:
ARRAY[i, i+1][1]
ARRAY[1, 2, 3][2]
(((i, i+1) as foo, bar)).foo
(((1, 2) as foo, bar)).bar
Release note: None