Skip to content

Conversation

@RaduBerinde
Copy link
Member

opt: support Tuple and DTuple when building a scalar

Adding support for tuples, using orderedListOp.

Release Notes: None

opt: index constraints for IN

Release note: None

@RaduBerinde RaduBerinde requested review from a team and petermattis December 14, 2017 02:41
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@petermattis
Copy link
Collaborator

:lgtm: I'm having deja-vu looking at some of this code. I'm curious how much of it you are basing on the existing span generation code.


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


pkg/sql/opt/testdata/build-scalar, line 147 at r2 (raw file):


build-scalar int
(@1) IN (1, 2)

Is there a reason for the parentheses around @1? Ditto below.


pkg/sql/opt/testdata/build-scalar, line 155 at r2 (raw file):

      └── const (2) (type: int)

build-scalar,normalize int

What difference are you intending to see between this test and the previous one?


Comments from Reviewable

Adding support for tuples, using orderedListOp.

Release Notes: None
@RaduBerinde RaduBerinde force-pushed the index-constraints-tuples branch from 554fef5 to c88d03c Compare December 14, 2017 03:15
Release note: None
@RaduBerinde RaduBerinde force-pushed the index-constraints-tuples branch from c88d03c to 3efb5cd Compare December 14, 2017 03:19
@RaduBerinde
Copy link
Member Author

I'm not actively basing it on the existing code (which has gotten pretty messy over time), but I am familiar with it so I'm probably using some understanding from it. The approach is somewhat different, we generate logical spans directly from expressions, and then manipulate the spans, whereas the existing code extracts a bunch of ComparisonExprs and converts those to spans later.


Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions.


pkg/sql/opt/testdata/build-scalar, line 147 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Is there a reason for the parentheses around @1? Ditto below.

Fixed


pkg/sql/opt/testdata/build-scalar, line 155 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

What difference are you intending to see between this test and the previous one?

Removed, they were left over from an earlier version where I was normalizing orderedListOp to listOp in this case (but I removed that because there's no benefit, at least not at this time).


Comments from Reviewable

@RaduBerinde RaduBerinde merged commit d155065 into cockroachdb:master Dec 14, 2017
@RaduBerinde RaduBerinde deleted the index-constraints-tuples branch December 14, 2017 13:44
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