Skip to content

sql: format tuple expressions with ROW prefix#28100

Closed
rjnn wants to merge 1 commit intocockroachdb:masterfrom
rjnn:tuples
Closed

sql: format tuple expressions with ROW prefix#28100
rjnn wants to merge 1 commit intocockroachdb:masterfrom
rjnn:tuples

Conversation

@rjnn
Copy link
Copy Markdown
Contributor

@rjnn rjnn commented Jul 31, 2018

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.

@rjnn rjnn requested review from a team and knz July 31, 2018 16:49
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@rjnn rjnn force-pushed the tuples branch 3 times, most recently from 6609052 to 3c36538 Compare July 31, 2018 16:51
@knz
Copy link
Copy Markdown
Contributor

knz commented Jul 31, 2018

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
Copy link
Copy Markdown
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: :shipit: 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.

@rjnn
Copy link
Copy Markdown
Contributor Author

rjnn commented Aug 1, 2018

Superceded by #28143.

@rjnn rjnn closed this Aug 1, 2018
craig bot pushed a commit that referenced this pull request Aug 2, 2018
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>
@rjnn rjnn deleted the tuples branch September 10, 2018 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants