Skip to content

sql: version gate JSON columns on inverted indexes#105840

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
fqazi:badJsonVersionGate
Jul 6, 2023
Merged

sql: version gate JSON columns on inverted indexes#105840
craig[bot] merged 1 commit intocockroachdb:masterfrom
fqazi:badJsonVersionGate

Conversation

@fqazi
Copy link
Copy Markdown
Collaborator

@fqazi fqazi commented Jun 29, 2023

Previously, our version gate for forward JSON indexes did not correctly handle the case when they are used in inverted indexes. When these columns are not the last column in an inverted index they need to be forward indexable, requiring a similar version gate. This patch fixes version gate logic for this scenario.

Fixes: #104178, Fixes #104707
Release note: None

@fqazi fqazi requested a review from a team as a code owner June 29, 2023 18:45
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

nice fix!

@rafiss
Copy link
Copy Markdown
Collaborator

rafiss commented Jun 30, 2023

there is an issue though:

pkg/sql/logictest/testdata/logic_test/inverted_index_multi_column:326:
        expected success, but found
        (42P16) index element foo of type jsonb is not indexable in a non-inverted index
        create_index.go:348: in checkIndexColumns()
    panic.go:522: -- test log scope end --

Copy link
Copy Markdown
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

the code change is too strict, since the failing test actually is using an inverted index, but the check still fails:

# Test selecting, inserting, updating, and deleting on a table with a
# multi-column JSON inverted index.
statement ok
CREATE TABLE d (
  id INT PRIMARY KEY,
  foo JSONB,
  bar JSONB,
  INVERTED INDEX idx (foo, bar)
);

Previously, our version gate for forward JSON indexes
did not correctly handle the case when they are used
in inverted indexes. When these columns are not the last
column in an inverted index they need to be forward indexable,
requiring a similar version gate. This patch fixes version
gate logic for this scenario.

Fixes: cockroachdb#104178, cockroachdb#104707
Release note: None
@fqazi fqazi force-pushed the badJsonVersionGate branch from be2931c to 31b790e Compare July 4, 2023 15:31
@fqazi
Copy link
Copy Markdown
Collaborator Author

fqazi commented Jul 4, 2023

@rafiss That test has to be updated since it's the mixed version config. Previously attempting to make an inverted index JSON column as the non-last column would result in the same error, so I updated the test to skip over these bits in a mixed version setting.

@fqazi fqazi requested a review from rafiss July 4, 2023 15:40
Copy link
Copy Markdown
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

makes sense, thanks for explaining!

@fqazi
Copy link
Copy Markdown
Collaborator Author

fqazi commented Jul 6, 2023

@rafiss TFTR!

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 6, 2023

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 6, 2023

Build succeeded:

@craig craig bot merged commit 818aec8 into cockroachdb:master Jul 6, 2023
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.

roachtest: schemachange/mixed-versions failed roachtest: acceptance/version-upgrade failed

3 participants