sql: Add the ability to access a specific column in a labeled tuple #25810

BramGruneir · 2018-05-22T16:49:39Z

This work is yet another step towards #16971.

The labeled tuples introduced in #25283 can now be accessed using their labels.
It's important to note that they require an extra parentheses around them to
do so.

SELECT (((1,'2',true) AS a, b, c)).a
+---+
| a |
+---+
| 1 |
+---+

SELECT (((1,'2',true) AS a, b, c)).b
+---+
| b |
+---+
| 2 |
+---+

SELECT (((1,'2',true) AS a, b, c)).c
+------+
|  c   |
+------+
| true |
+------+

This change prompted the need to restrict the labels in the tuple to be unique;
checks and test cases for that have been added as well.

Star expansion is still not allowed.

Release note: Labeled tuples can now be accessed using their labels (e.g.
SELECT (((1,'2',true) AS a, b, c)).a but it requires an extra level of
parentheses to do so.

cockroach-teamcity · 2018-05-22T16:49:47Z

This change is

BramGruneir · 2018-05-22T16:57:47Z

pkg/sql/sem/tree/type_check.go, line 890 at r1 (raw file):

	if len(expr.Labels) > 0 {
		// Ensure that there are no repeat labels.
		uniqueLabels := map[string]struct{}{}

This worries me a bit. It uses memory and I don't think it fully safeguards against it. The alternative is to do a single pass through all the labels in the columnAccessExpr type check to see if there is a double and if so fail for being ambiguous.

Thoughts?

Comments from Reviewable

knz · 2018-05-23T09:28:45Z

This implementation is adequate because it is correct. Just a few nits below.

Meanwhile, since our discussion yesterday I have thought of an optimization. See my comment below. Let me know what you think.

Reviewed 6 of 9 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

pkg/sql/logictest/testdata/logic_test/tuple, line 797 at r2 (raw file):


# Ensure the number of labels matches the number of expressions
query error pq: the number of expressions in a labeled tuple \(2\) must match the number of labels \(1\)

This is super verbose. I'd recommend more simply: mismatch in tuple definition: %d expressions, %d labels

pkg/sql/sem/tree/eval.go, line 3398 at r2 (raw file):

// Eval implements the TypedExpr interface.
func (expr *ColumnAccessExpr) Eval(ctx *EvalContext) (Datum, error) {
	d, err := expr.Expr.(TypedExpr).Eval(ctx)

Idea for optimization: keep the error that was here originally. See my other comment below.

pkg/sql/sem/tree/expr.go, line 1496 at r2 (raw file):

	ColName  string
	Star     bool
	ColIndex int // This should only be set during type checking.

Idea for optimization: do not add this field. See my comment below.

pkg/sql/sem/tree/type_check.go, line 890 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

This worries me a bit. It uses memory and I don't think it fully safeguards against it. The alternative is to do a single pass through all the labels in the columnAccessExpr type check to see if there is a double and if so fail for being ambiguous.

Thoughts?

Yes I think the n^2 algorithm is appropriate here:

for i := range expr.Labels {
   for j := 0; j < i; j++ {
       if expr.Labels[i] == expr.Labels[j] { ... error ... }
   }
}

Make the error: "found duplicate tuple label: %q", tree.ErrString(...)

pkg/sql/sem/tree/type_check.go, line 406 at r2 (raw file):

	// Ensure that the tuple is indeed labeled.
	if len(resolvedType.(types.TTuple).Labels) == 0 {

factor the .(types.TTuple) cast so it's just performed once

pkg/sql/sem/tree/type_check.go, line 417 at r2 (raw file):

	// Go through all of the labels to find a match.
	expr.ColIndex = -1

Idea for optimization: define a local variable colIndex instead of storing in expr. Then see below.

pkg/sql/sem/tree/type_check.go, line 429 at r2 (raw file):

			"could not identify column \"%s\" in %s", expr.ColName, resolvedType)
	}

Idea for optimization, here:

return expr.Expr.(*Tuple).Exprs[colIndex], nil

And zoooooom - both the ColumnAccessExpr and the tuple evaporate in a puff of nothingness.

Comments from Reviewable

BramGruneir · 2018-05-24T21:03:56Z

All comments addressed. PTAL.

Review status: 4 of 9 files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

pkg/sql/logictest/testdata/logic_test/tuple, line 797 at r2 (raw file):

Previously, knz (kena) wrote…

This is super verbose. I'd recommend more simply: mismatch in tuple definition: %d expressions, %d labels

Done.

pkg/sql/sem/tree/eval.go, line 3398 at r2 (raw file):

Previously, knz (kena) wrote…

Idea for optimization: keep the error that was here originally. See my other comment below.

Done.

pkg/sql/sem/tree/type_check.go, line 890 at r1 (raw file):

Previously, knz (kena) wrote…

Yes I think the n^2 algorithm is appropriate here:
for i := range expr.Labels {
   for j := 0; j < i; j++ {
       if expr.Labels[i] == expr.Labels[j] { ... error ... }
   }
}
Make the error: "found duplicate tuple label: %q", tree.ErrString(...)

Done,.

I could just check for the duplicate label when pulling out the column instead of during the definition.
And only do a single pass.

pkg/sql/sem/tree/type_check.go, line 406 at r2 (raw file):

Previously, knz (kena) wrote…

factor the .(types.TTuple) cast so it's just performed once

Done.

pkg/sql/sem/tree/type_check.go, line 417 at r2 (raw file):

Previously, knz (kena) wrote…

Idea for optimization: define a local variable colIndex instead of storing in expr. Then see below.

Done.

pkg/sql/sem/tree/type_check.go, line 429 at r2 (raw file):

Previously, knz (kena) wrote…

Idea for optimization, here:

return expr.Expr.(*Tuple).Exprs[colIndex], nil

And zoooooom - both the ColumnAccessExpr and the tuple evaporate in a puff of nothingness.

Wonderful idea.

Done.

pkg/sql/sem/tree/expr.go, line 1496 at r2 (raw file):

Previously, knz (kena) wrote…

Idea for optimization: do not add this field. See my comment below.

Done.

Comments from Reviewable

knz · 2018-05-25T13:09:05Z

with a nit and a required fix. See comment below.

Reviewed 6 of 6 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

pkg/sql/sem/tree/type_check.go, line 435 at r3 (raw file):

	}

	return expr.Expr.(*Tuple).Exprs[colIndex].TypeCheck(ctx, desired)

You need to call TypeCheck here. It was already done at the beginning. Just cast the result with return ... .(TypedExpr)

pkg/sql/sem/tree/type_check.go, line 898 at r3 (raw file):

				if expr.Labels[i] == expr.Labels[j] {
					return nil, pgerror.NewErrorf(pgerror.CodeSyntaxError,
						"found duplicate tuple label: %q", ErrString(expr),

I'd recommend ErrString(expr.Labels) otherwise the message could become super verbose (imagine if the expression contains a subquery...).

Comments from Reviewable

BramGruneir · 2018-05-25T16:44:25Z

TFTR!

Review status: 6 of 9 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

pkg/sql/sem/tree/type_check.go, line 435 at r3 (raw file):

Previously, knz (kena) wrote…

You need to call TypeCheck here. It was already done at the beginning. Just cast the result with return ... .(TypedExpr)

Ah, excellent, I was annoyed by that.

pkg/sql/sem/tree/type_check.go, line 898 at r3 (raw file):

Previously, knz (kena) wrote…

I'd recommend ErrString(expr.Labels) otherwise the message could become super verbose (imagine if the expression contains a subquery...).

Done.

Comments from Reviewable

This work is yet another step towards cockroachdb#16971. The labeled tuples introduced in cockroachdb#25283 can now be accessed using their labels. It's important to note that they require an extra parentheses around them to do so. ```sql SELECT (((1,'2',true) AS a, b, c)).a +---+ | a | +---+ | 1 | +---+ SELECT (((1,'2',true) AS a, b, c)).b +---+ | b | +---+ | 2 | +---+ SELECT (((1,'2',true) AS a, b, c)).c +------+ | c | +------+ | true | +------+ ``` This change prompted the need to restrict the labels in the tuple to be unique; checks and test cases for that have been added as well. Star expansion is still not allowed. Release note: Labeled tuples can now be accessed using their labels (e.g. `SELECT (((1,'2',true) AS a, b, c)).a` but it requires an extra level of parentheses to do so.

knz · 2018-05-25T17:23:21Z

Reviewed 3 of 4 files at r4.
Review status: 8 of 9 files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable

knz · 2018-05-25T17:23:32Z

Reviewed 1 of 4 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable

BramGruneir · 2018-05-25T17:37:22Z

bors r+

…

On Fri, May 25, 2018, 13:23 kena ***@***.***> wrote: Reviewed 1 of 4 files at r4. Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful. ------------------------------ *Comments from Reviewable <https://reviewable.io/reviews/cockroachdb/cockroach/25810>* — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#25810 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABihudO0DR_XZ2I3hZXMvDB8dlWobsxeks5t2D4egaJpZM4UJBZU> .

25810: sql: Add the ability to access a specific column in a labeled tuple r=BramGruneir a=BramGruneir This work is yet another step towards #16971. The labeled tuples introduced in #25283 can now be accessed using their labels. It's important to note that they require an extra parentheses around them to do so. ```sql SELECT (((1,'2',true) AS a, b, c)).a +---+ | a | +---+ | 1 | +---+ SELECT (((1,'2',true) AS a, b, c)).b +---+ | b | +---+ | 2 | +---+ SELECT (((1,'2',true) AS a, b, c)).c +------+ | c | +------+ | true | +------+ ``` This change prompted the need to restrict the labels in the tuple to be unique; checks and test cases for that have been added as well. Star expansion is still not allowed. Release note: Labeled tuples can now be accessed using their labels (e.g. `SELECT (((1,'2',true) AS a, b, c)).a` but it requires an extra level of parentheses to do so. 26096: scripts: add Yahor to release notes r=yuzefovich a=yuzefovich This adds Yahor Yuzefovich to the roachers list. Release note: None Co-authored-by: Bram Gruneir <bram@cockroachlabs.com> Co-authored-by: yuzefovich <yahor@cockroachlabs.com>

craig · 2018-05-25T17:58:55Z

Build succeeded

GitHub CI (Cockroach)

BramGruneir requested review from knz and a team May 22, 2018 16:49

BramGruneir force-pushed the colaccexpr branch from d7705ee to c18ff54 Compare May 22, 2018 19:20

BramGruneir force-pushed the colaccexpr branch from c18ff54 to c249d45 Compare May 24, 2018 21:03

BramGruneir force-pushed the colaccexpr branch from c249d45 to f5b524a Compare May 25, 2018 16:28

BramGruneir force-pushed the colaccexpr branch from f5b524a to b29728d Compare May 25, 2018 16:49

craig bot merged commit b29728d into cockroachdb:master May 25, 2018

BramGruneir deleted the colaccexpr branch May 25, 2018 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: Add the ability to access a specific column in a labeled tuple #25810

sql: Add the ability to access a specific column in a labeled tuple #25810

BramGruneir commented May 22, 2018

cockroach-teamcity commented May 22, 2018

BramGruneir commented May 22, 2018

knz commented May 23, 2018

BramGruneir commented May 24, 2018

knz commented May 25, 2018

BramGruneir commented May 25, 2018

knz commented May 25, 2018

knz commented May 25, 2018

BramGruneir commented May 25, 2018 via email

craig bot commented May 25, 2018

sql: Add the ability to access a specific column in a labeled tuple #25810

sql: Add the ability to access a specific column in a labeled tuple #25810

Conversation

BramGruneir commented May 22, 2018

cockroach-teamcity commented May 22, 2018

BramGruneir commented May 22, 2018

knz commented May 23, 2018

BramGruneir commented May 24, 2018

knz commented May 25, 2018

BramGruneir commented May 25, 2018

knz commented May 25, 2018

knz commented May 25, 2018

BramGruneir commented May 25, 2018 via email

craig bot commented May 25, 2018

Build succeeded