/cockroach

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

opt: build IS OF (...) expression #31393

Merged
merged 1 commit into from Oct 16, 2018

Conversation

Reviewers

@knz knz

@rytaft rytaft

@cockroachdb/sql-rest-prs sql-rest-prs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@justinj @cockroach-teamcity @knz @rytaft
@justinj
Member

justinj commented Oct 15, 2018

This commit builds expressions of the form x IS OF (...). This is an
optbuilder-only change since the value of such an expression is always
known at plan time.

This commit also partially fixes a bug in the heuristic planner's
implementation of this feature by using Equivalent instead of
FamilyEqual to compare types, resulting in INT[] being considered
distinct from STRING[]. It also exposes a bug that I think is
difficult to fix at the moment, which is that the lack of a 1-1 mapping
between datum and column types means the following is incorrectly
reported as true:

3::INT2 IS OF (INT4)

I've added relevant test cases and added a TODO for that case.

Tagging @knz for the heuristic planner change and also to put the
remaining type-related bug on his radar.

Release note (bug fix): Fixed IS OF (...) expressions to no longer
report arrays with different element types as being the same.

@justinj justinj requested review from knz and rytaft Oct 15, 2018

@justinj justinj requested review from cockroachdb/sql-opt-prs as code owners Oct 15, 2018

@cockroach-teamcity

This comment has been minimized.

Show comment
Hide comment
@cockroach-teamcity

cockroach-teamcity Oct 15, 2018

Member

This change is Reviewable
Member

cockroach-teamcity commented Oct 15, 2018

This change is Reviewable
@rytaft

rytaft approved these changes Oct 15, 2018
View changes

:lgtm:

Maybe you should open an issue for the bug?

Reviewed 5 of 5 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

@knz

knz approved these changes Oct 15, 2018
View changes

cool thanks!

you can also mention in the commit message or comments that this is also done statically in postgres (transformAExprOf() in src/backend/parser/parse_expr.c)

Show outdated Hide outdated pkg/sql/sem/tree/eval_test.go Outdated
@justinj
opt: build IS OF (...) expression 
This commit builds expressions of the form x IS OF (...). This is an
optbuilder-only change since the value of such an expression is always
known at plan time.

This is also performed statically in Postgres, in the file
src/backend/parser/parse_expr.c in the function transformAExprOf.

This commit also partially fixes a bug in the heuristic planner's
implementation of this feature by using `Equivalent` instead of
`FamilyEqual` to compare types, resulting in `INT[]` being considered
distinct from `STRING[]`. It also exposes a bug that I think is
difficult to fix at the moment, which is that the lack of a 1-1 mapping
between datum and column types means the following is incorrectly
reported as true:

```
3::INT2 IS OF (INT4)
```

I've added relevant test cases and added a TODO for that case.

Tagging @knz for the heuristic planner change and also to put the
type-related bug on his radar.

Release note (bug fix): Fixed IS OF (...) expressions to no longer
report arrays with different element types as being the same.
Loading status checks…
a1cb08a
@justinj

This comment has been minimized.

Show comment
Hide comment
@justinj

justinj Oct 16, 2018

Member

TFTRs!

bors r+
Member

justinj commented Oct 16, 2018

TFTRs!

bors r+

craig bot pushed a commit that referenced this pull request Oct 16, 2018

craig[bot] @justinj
Merge #31393 
31393: opt: build IS OF (...) expression r=justinj a=justinj

This commit builds expressions of the form x IS OF (...). This is an
optbuilder-only change since the value of such an expression is always
known at plan time.

This commit also partially fixes a bug in the heuristic planner's
implementation of this feature by using `Equivalent` instead of
`FamilyEqual` to compare types, resulting in `INT[]` being considered
distinct from `STRING[]`. It also exposes a bug that I think is
difficult to fix at the moment, which is that the lack of a 1-1 mapping
between datum and column types means the following is incorrectly
reported as true:

```
3::INT2 IS OF (INT4)
```

I've added relevant test cases and added a TODO for that case.

Tagging @knz for the heuristic planner change and also to put the
remaining type-related bug on his radar.

Release note (bug fix): Fixed IS OF (...) expressions to no longer
report arrays with different element types as being the same.

Co-authored-by: Justin Jaffray <justin@cockroachlabs.com>
Loading status checks…
918d44b
@craig

This comment has been minimized.

Show comment
Hide comment
@craig

craig bot Oct 16, 2018

Build succeeded

craig bot commented Oct 16, 2018

Build succeeded

@craig craig bot merged commit a1cb08a into cockroachdb:master Oct 16, 2018
3 checks passed

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment