sql: fix the handling of tuples in distsql#28143
sql: fix the handling of tuples in distsql#28143craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
Ok I was a bit too trigger happy pushing this PR to everyone. There are really multiple changes/fixes in there. I will split them, add some missing tests and submit the changes separately. |
rjnn
left a comment
There was a problem hiding this comment.
Reviewed 31 of 31 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained
pkg/sql/sem/tree/expr.go, line 741 at r1 (raw file):
// col_name.go. It is also used when pretty-printing for human // consumption, see pretty.go. Row bool
Is this bool needed any more?
28128: sql: casting to timestamp should strip tz info r=jordanlewis a=jordanlewis And binary operations between timestamp and timestamptz should strip the tz from the timestamptz. Confirmed that all of this behavior matches postgres (and that it didn't before). Release note (bug fix): correct casts and binary operators between timestamptz and timestamp in some cases. 28149: sql: avoid using Tuple in RangePartition r=knz a=knz Forked off #28143, needed for #25522 / #26624. The `Tuple` AST node is really for scalar tuples. RangePartition is not using a scalar tuple. So split them. Release note: None Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com> Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
pkg/sql/insert.go
Outdated
| for i := len(tuple); i < len(cols); i++ { | ||
| copyTuple() | ||
| tuple.Exprs = append(tuple.Exprs, defaultExpr(len(tuple.Exprs))) | ||
| ret.Tuples[tIdx] = append(ret.Tuples[tIdx], defaultExpr(len(tuple))) |
There was a problem hiding this comment.
@arjunravinarayan to follow-up on our conversation, this is the line where I introduced a bug.
The code should read instead:
tuple = append(tuple, defaultExpr(len(tuple)))
ret.Tuples[tIdx] = tuple65a18f0 to
64fd0df
Compare
|
OK PR has been split and simplified. RFAL! |
|
(the first 3 commits are from different PRs but are prerequisites to this. I will rebase when those merge.) |
64fd0df to
022fb74
Compare
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/expr.go, line 741 at r1 (raw file):
Previously, arjunravinarayan (Arjun Narayan) wrote…
Is this bool needed any more?
Yes it is needed to derive a column name automatically like pg in e.g. select row(123).
5914000 to
909764b
Compare
909764b to
b36f852
Compare
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>
b36f852 to
936fe37
Compare
28192: sql/parser: parsing ROW produces *Tuple, not Tuple r=knz a=knz Forked off from #28143. Needed for #26624. The tree.Tuple result of parsing tuples was immediately re-allocated on the heap in the parent action rule. This patch simplifies this by allocating on the heap upfront. Release note: None Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
936fe37 to
56db7a0
Compare
jordanlewis
left a comment
There was a problem hiding this comment.
Generally LGTM though I'm concerned that some of the CLI outputs don't match postgres. See comments
Reviewed 1 of 31 files at r1, 34 of 34 files at r2.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/srfs, line 418 at r2 (raw file):
("c",1) ("b",2) ("a",3)
I think this probably is fine, but FYI this is not what postgres outputs.
jordan=# SELECT information_schema._pg_expandarray(ARRAY['c', 'b', 'a']);
_pg_expandarray
-----------------
(c,1)
(b,2)
(a,3)
(3 rows)
pkg/sql/logictest/testdata/logic_test/srfs, line 545 at r2 (raw file):
("(3,1)",123) ("(2,2)",123) ("(1,3)",123)
jordan=# SELECT ((i.keys).*, 123) FROM (SELECT information_schema._pg_expandarray(ARRAY[3,2,1]) AS keys) AS i;
row
-----------
(3,1,123)
(2,2,123)
(1,3,123)
pkg/sql/logictest/testdata/logic_test/tuple, line 10 at r2 (raw file):
---- t u (1,2,"hello",,) (t,,"(f,6.6,f)")
jordan=# SELECT (1, 2, 'hello', NULL, NULL) AS t, (true, NULL, (false, 6.6, false)) AS u;
t | u
---------------+------------------
(1,2,hello,,) | (t,,"(f,6.6,f)")
pkg/sql/sem/tree/expr.go, line 763 at r3 (raw file):
ctx.WriteByte('(') ctx.FormatNode(&node.Exprs) if len(node.Exprs) == 1 {
nit: The equivalent in DTuple has a comment explaining this - if you're going to rebase this again plz add that comment here too.
pkg/sql/sem/tree/format.go, line 87 at r2 (raw file):
// If set, strings will be formatted for being contents of ARRAYs. // Used internally in combination with FmtPgwireText defined below. pgwireFormat
Nice idea.
But this flag does more than the comment says, now. It also prints NULL as the empty string, right?
56db7a0 to
9412d4a
Compare
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/srfs, line 418 at r2 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
I think this probably is fine, but FYI this is not what postgres outputs.
jordan=# SELECT information_schema._pg_expandarray(ARRAY['c', 'b', 'a']); _pg_expandarray ----------------- (c,1) (b,2) (a,3) (3 rows)
Discussion for #28151.
My comment there: "it's ugly but it is correct".
pkg/sql/logictest/testdata/logic_test/srfs, line 545 at r2 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
jordan=# SELECT ((i.keys).*, 123) FROM (SELECT information_schema._pg_expandarray(ARRAY[3,2,1]) AS keys) AS i; row ----------- (3,1,123) (2,2,123) (1,3,123)
That's a separate bug, well spotted: #28203
pkg/sql/logictest/testdata/logic_test/tuple, line 10 at r2 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
jordan=# SELECT (1, 2, 'hello', NULL, NULL) AS t, (true, NULL, (false, 6.6, false)) AS u; t | u ---------------+------------------ (1,2,hello,,) | (t,,"(f,6.6,f)")
Discussion for #28151. Same comment as above.
pkg/sql/sem/tree/expr.go, line 763 at r3 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
nit: The equivalent in DTuple has a comment explaining this - if you're going to rebase this again plz add that comment here too.
Done.
pkg/sql/sem/tree/format.go, line 87 at r2 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
Nice idea.
But this flag does more than the comment says, now. It also prints NULL as the empty string, right?
Changed the comment. Thanks for noticing.
jordanlewis
left a comment
There was a problem hiding this comment.
Reviewed 2 of 27 files at r4, 25 of 25 files at r5.
Reviewable status:
9412d4a to
822aea1
Compare
|
I'll hold off this while Bram prepares his all-eng prez. |
28151: sql: fix the pg text representation of tuples/arrays r=knz a=knz Forked off #28143. Fixes #25522. This patch fixes the conversion of arrays and tuples to text for the purpose of emitting the arrays/tuples back to a client through pgwire. This now properly supports nested arrays, arrays containing tuples, tuples containing arrays, etc. Labels in tuples are never emitted through pgwire. Release note (bug fix): CockroachDB supports a wider range of tuple and array values in query results. Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
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. In order to achieve this it also extends CockroachDB to support empty tuples (notation: `()`), 1-valued tuples without the `row` keyword (notation: `(x,)`), and empty tuples on the RHS of `IN` comparisons. Release note (sql change): CockroachDB now supports empty tuples with the syntax `()`, 1-valued tuples with the syntax `(x,)` in addition to `row(x)`, and the ability to use `IN` with an empty tuple as right operand.
822aea1 to
c35d8e4
Compare
|
Thanks all bors r+ |
Build failed (retrying...) |
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>
Build succeeded |
28183: sql: support casting arrays and tuples to strings r=knz a=knz Fixes #24746. Forked off #28143. The previous patch to fix pgwire text encoding for tuples/arrays really defines a different conversion algorithm from array/tuples to strings. This incidentally is the general-purpose algorithm used in PostgreSQL to convert arrays and tuples to strings, not just for pgwire. This patch utilizes this observation to define the new (and now correct) casts from array and tuple types to strings based on the new algorithm, like PostgreSQL does. Release note (sql change): CockroachDB now supports converting arrays and tuples to strings for compatibility with PostgreSQL. Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
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