sql: avoid using Tuple in ValuesClause#28191
Conversation
|
This change is |
f1f4b2d to
b4dd1d1
Compare
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/sql/sem/tree/values.go, line 28 at r1 (raw file):
// ValuesClause represents a VALUES clause. type ValuesClause struct { Tuples []Exprs
[nit] should we update the member name? Maybe Rows?
pkg/sql/sem/tree/values.go, line 38 at r1 (raw file):
ctx.WriteString(comma) ctx.WriteByte('(') ctx.FormatNode(&node.Tuples[i])
Is & needed here? node.Tuples[i] is an Expr..
rjnn
left a comment
There was a problem hiding this comment.
Reviewed 15 of 15 files at r1.
Reviewable status:
b4dd1d1 to
ac0528f
Compare
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/values.go, line 28 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] should we update the member name? Maybe
Rows?
Done.
pkg/sql/sem/tree/values.go, line 38 at r1 (raw file):
Previously, RaduBerinde wrote…
Is
&needed here?node.Tuples[i]is anExpr..
no, node.Tuples[i] is an Exprs. This implements NodeFormatter by pointer, not by value.
|
TFYRs! bors r+ |
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/testutils/lint/lint_test.go, line 108 at r2 (raw file):
t.Run("TestLowercaseFunctionNames", func(t *testing.T) { t.Skip("woo")
Leftover?
|
bors r- |
Canceled |
|
yeah good catch |
The `Tuple` AST node is really about scalar tuples. The `VALUES` clause operands are not really scalar tuples. So instead use `Exprs` for `ValuesClause`. This will simplify later patch to fix the handling of scalar tuples. Release note: None
ac0528f to
246afb6
Compare
|
bors r+ |
28191: sql: avoid using Tuple in ValuesClause r=knz a=knz Forked off from #28143. Needed for #26624. The `Tuple` AST node is really about scalar tuples. The `VALUES` clause operands are not really scalar tuples. So instead use `Exprs` for `ValuesClause`. This will simplify later patch to fix the handling of scalar tuples. Release note: None Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
Build succeeded |
Forked off from #28143.
Needed for #26624.
The
TupleAST node is really about scalar tuples. TheVALUESclause operands are not really scalar tuples. So instead use
ExprsforValuesClause.This will simplify later patch to fix the handling of scalar tuples.
Release note: None