Skip to content
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.

Sign up for GitHub

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

Jump to bottom

opt: fix inverted key column type in testcat #82547

Merged
merged 1 commit into from
Jun 9, 2022
Merged

opt: fix inverted key column type in testcat #82547

merged 1 commit into from
Jun 9, 2022

Conversation

mgartner
Copy link
Collaborator

@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 michae2, wenyihu6 and a team 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
Member

This change is Reviewable

@mgartner mgartner mentioned this pull request Jun 7, 2022
Merged
@wenyihu6
Copy link
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
Collaborator 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
Contributor

wenyihu6 commented Jun 8, 2022

Ohh true. I see now!

@wenyihu6
Copy link
Contributor

wenyihu6 commented Jun 8, 2022

:lgtm:

michae2
michae2 approved these changes Jun 8, 2022
View reviewed changes
Copy link
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
@@ -892,7 +892,7 @@ func newOptTable(

invertedSourceColOrdinal, _ := ot.lookupColumnOrdinal(invertedColumnID)

// Add a inverted column that refers to the inverted index key.
// Add an inverted column that refers to the inverted index key.
invertedCol, invertedColOrd := newColumn()

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

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
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
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
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
Collaborator 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
Collaborator 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
Collaborator Author

mgartner commented Jun 8, 2022

TFTRs!

bors r+

@craig
Copy link
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

@wenyihu6 wenyihu6 wenyihu6 approved these changes

@michae2 michae2 michae2 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