sql: format tuple expressions with ROW prefix#28100
sql: format tuple expressions with ROW prefix#28100rjnn wants to merge 1 commit intocockroachdb:masterfrom
Conversation
6609052 to
3c36538
Compare
|
thanks! I'll look into it tomorrow morning. (I have some edge cases in mind which I want to test manually) |
Unconditionally format tuple expressions (e.g. (1,2,3) as ROW(1,2,3). This disambiguates singleton tuples (e.g. (1)) so that they are correctly interpreted as tuples upon deserialization, rather than parenthesized base types. Release note: None
knz
left a comment
There was a problem hiding this comment.
I made a preliminary review of this PR. Take it with a grain of salt, I still need to run some tests. Thanks for this first attempt.
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained
pkg/sql/sem/tree/datum.go, line 2495 at r1 (raw file):
typ types.TTuple row bool
I believe you do not need this field any more.
pkg/sql/sem/tree/datum.go, line 2678 at r1 (raw file):
if ctx.HasFlags(FmtParsable) { ctx.WriteString("ROW") if len(d.D) == 0 {
Empty tuples are so rare that you can drop the if len(d.D) == 0 case here. The formatting of the tuple below will produce the right output.
pkg/sql/sem/tree/eval.go, line 3787 at r1 (raw file):
tuple.D[i] = d } tuple.row = t.Row
ditto uneeded
pkg/sql/sem/tree/expr.go, line 418 at r1 (raw file):
binExprFmtWithParenAndSubOp(ctx, node.Left, node.SubOperator.String(), opStr, node.Right) } else { if t, ok := node.Right.(*Tuple); ok {
You need the special case for the suboperator case above too.
pkg/sql/sem/tree/expr.go, line 423 at r1 (raw file):
ctx.WriteString(opStr) ctx.WriteByte(' ') ctx.WriteByte('(')
Combine the two WriteByte calls. Also, you need ROW here if and only if the oeprator is not IN/NOT IN
pkg/sql/sem/tree/expr.go, line 770 at r1 (raw file):
} if ctx.HasFlags(FmtParsable) { ctx.WriteString("ROW")
That's too many occurrences of ROW, IMHO.
|
Superceded by #28143. |
28143: sql: fix the handling of tuples in distsql r=knz a=knz Fixes #26624. Supersedes #28100. This patch fixes the bug where 0-valued and 1-valued tuples were not properly serialized in distsql processor/flow specs, causing these special tuples to not be properly supported for distributed execution. It also fixes the bug where labeled DTuples were not serialized with their tuples, causing labeled tuples to be broken with distributed execution. Release note: None 28217: storage: dissallow empty HeartbeatRequest.Now r=andreimatei a=andreimatei Release note: None 28219: distsql: fix a missing context in log message r=arjunravinarayan a=arjunravinarayan Release note: None Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com> Co-authored-by: Andrei Matei <andrei@cockroachlabs.com> Co-authored-by: Arjun Narayan <arjun@cockroachlabs.com>
Unconditionally format tuple expressions (e.g. (1,2,3) as
ROW(1,2,3). This disambiguates singleton tuples (e.g. (1)) so that
they are correctly interpreted as tuples upon deserialization, rather
than parenthesized base types.
Currently failing logic tests that require rewriting.