Skip to content

opt: fix inverted key column type in testcat#82547

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
mgartner:fix-test-cat-inverted-col-type
Jun 9, 2022
Merged

opt: fix inverted key column type in testcat#82547
craig[bot] merged 1 commit intocockroachdb:masterfrom
mgartner:fix-test-cat-inverted-col-type

Conversation

@mgartner
Copy link
Copy Markdown
Contributor

@mgartner mgartner commented Jun 7, 2022

Previously, inverted key columns in the test catalog were incorrectly
given the type of their source column. Now they have the correct type,
types.EncodedKey.

Release note: None

@mgartner mgartner requested review from a team, michae2 and wenyihu6 June 7, 2022 20:50
@mgartner mgartner requested a review from a team as a code owner June 7, 2022 20:50
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Jun 7, 2022

This change is Reviewable

@mgartner mgartner mentioned this pull request Jun 7, 2022
Merged
@wenyihu6
Copy link
Copy Markdown
Contributor

wenyihu6 commented Jun 8, 2022

a discussion (no related file):
TeamCity is complaining about the test cases in pkg/sql/opt/testutils/testcat/testdata. The expected output in the test data folder is still the source column type instead of EncodedKey.
There are two other parts where inverted column type were also referenced.

  1. here. Column kind is inverted, so the type would be EncodedKey. Should we use the type of the source column for the panic error string instead?
  2. here Should we use EncodedKey for inverted column type in scanColumnsConfig?

@mgartner mgartner force-pushed the fix-test-cat-inverted-col-type branch from 678e325 to aa5de04 Compare June 8, 2022 13:47
@mgartner
Copy link
Copy Markdown
Contributor Author

mgartner commented Jun 8, 2022

TeamCity is complaining about the test cases in pkg/sql/opt/testutils/testcat/testdata.

Thanks! Should be fixed now.

  1. ...Should we use the type of the source column for the panic error string instead?

Yes. This PR includes that change: https://github.com/cockroachdb/cockroach/pull/82547/files#diff-f5b6d3abeaf55ecb48cf9aa29ab0e311a1c4a4ede69bce28913d5b45449f6b6aR1109

  1. Should we use EncodedKey for inverted column type in scanColumnsConfig?

I believe we are using EncodedKey because col.DatumType() will return types.EncodedKey when the column is an inverted key column.

@wenyihu6
Copy link
Copy Markdown
Contributor

wenyihu6 commented Jun 8, 2022

Ohh true. I see now!

@wenyihu6
Copy link
Copy Markdown
Contributor

wenyihu6 commented Jun 8, 2022

:lgtm:

michae2
michae2 approved these changes Jun 8, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 10 of 10 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @mgartner and @wenyihu6)

wenyihu6
wenyihu6 reviewed Jun 8, 2022
View reviewed changes
pkg/sql/opt_catalog.go Outdated
// Add an inverted column that refers to the inverted index key.
invertedCol, invertedColOrd := newColumn()

// All inverted columns have type bytes.
Copy link
Copy Markdown
Contributor

@wenyihu6 wenyihu6 Jun 8, 2022

Choose a reason for hiding this comment

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

Should we change this comment to // All inverted columns have type EncodedKey.?

@wenyihu6
Copy link
Copy Markdown
Contributor

wenyihu6 commented Jun 8, 2022

This is probably unrelated to this pr change. But when I was reviewing the code for inverted column type, I was confused about the logic here. Why should the first column type be int?

@michae2
Copy link
Copy Markdown
Collaborator

michae2 commented Jun 8, 2022 •
edited
Loading

This is probably unrelated to this pr change. But when I was reviewing the code for inverted column type, I was confused about the logic here. Why should the first column type be int?

I think this is something called the cellid though I don't really know the significance. I'm guessing that normally the encoding of the inverted index key is opaque to the optimizer, but in this case we wanted the optimizer to know about the cellid for some reason.

wenyihu6
wenyihu6 approved these changes Jun 8, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@wenyihu6 wenyihu6 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Reviewed 2 of 10 files at r1.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @mgartner)

@mgartner
opt: fix inverted key column type in testcat
ec8e63c 
Previously, inverted key columns in the test catalog were incorrectly
given the type of their source column. Now they have the correct type,
`types.EncodedKey`.

Release note: None
@mgartner mgartner force-pushed the fix-test-cat-inverted-col-type branch from aa5de04 to ec8e63c Compare June 8, 2022 20:28
@mgartner
Copy link
Copy Markdown
Contributor Author

mgartner commented Jun 8, 2022

This is probably unrelated to this pr change. But when I was reviewing the code for inverted column type, I was confused about the logic here. Why should the first column type be int?

Thanks for pointing this out. I'm not sure about the int, but this looks wrong for other reasons. For example, the inverted column is not the first indexed column in multi-column geo-spatial indexes. I'll look into this separately from this PR.

@mgartner
Copy link
Copy Markdown
Contributor Author

mgartner commented Jun 8, 2022

For example, the inverted column is not the first indexed column in multi-column geo-spatial indexes. I'll look into this separately from this PR.

I've addressed the problems I could see in this PR: #82632

@mgartner
Copy link
Copy Markdown
Contributor Author

mgartner commented Jun 8, 2022

TFTRs!

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 9, 2022

Build succeeded:

@craig craig bot merged commit 30d4c9d into cockroachdb:master Jun 9, 2022
@mgartner mgartner deleted the fix-test-cat-inverted-col-type branch June 9, 2022 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michae2 michae2 michae2 approved these changes

@wenyihu6 wenyihu6 wenyihu6 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mgartner @cockroach-teamcity @wenyihu6 @michae2