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.

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

sql: disallow DESC option in the last column of inverted indexes #84516

Merged
merged 1 commit into from
Jul 20, 2022

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Jul 15, 2022

The last column of inverted indexes cannot have the DESC direction.
Prior to this commit it was allowed, but caused internal errors. To
avoid propagating the notion that inverted indexes have a semantic
direction, we stop printing the ASC option for inverted index columns
in the output of SHOW CREATE TABLE.

Fixes #84388

Release note (sql change): The last column of an INVERTED INDEX can no
longer have the DESC option. If DESC was used in prior versions, it
could cause internal errors.

@mgartner mgartner requested review from chengxiong-ruan, michae2 and a team July 15, 2022 22:33
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan, @mgartner, and @michae2)


-- commits line 16 at r1:
I don't think this is true. There are some cases where we scan a range. For example, look at the second span here:

> EXPLAIN (opt) SELECT a FROM json_tab WHERE b @> '{"a": {}}' ORDER BY a
;
                                 info
----------------------------------------------------------------------
  sort
   └── project
        └── inverted-filter
             ├── inverted expression: /5
             │    ├── tight: true, unique: false
             │    └── union spans
             │         ├── ["7a\x00\x019", "7a\x00\x019"]
             │         └── ["7a\x00\x02\x00\xff", "7a\x00\x03")
             └── scan json_tab@foo_inv
                  └── inverted constraint: /5/1
                       └── spans
                            ├── ["7a\x00\x019", "7a\x00\x019"]
                            └── ["7a\x00\x02\x00\xff", "7a\x00\x03")

Why can't we just disallow DESC for now?

@mgartner
Copy link
Collaborator Author

-- commits line 16 at r1:

Previously, rytaft (Rebecca Taft) wrote…

I don't think this is true. There are some cases where we scan a range. For example, look at the second span here:

> EXPLAIN (opt) SELECT a FROM json_tab WHERE b @> '{"a": {}}' ORDER BY a
;
                                 info
----------------------------------------------------------------------
  sort
   └── project
        └── inverted-filter
             ├── inverted expression: /5
             │    ├── tight: true, unique: false
             │    └── union spans
             │         ├── ["7a\x00\x019", "7a\x00\x019"]
             │         └── ["7a\x00\x02\x00\xff", "7a\x00\x03")
             └── scan json_tab@foo_inv
                  └── inverted constraint: /5/1
                       └── spans
                            ├── ["7a\x00\x019", "7a\x00\x019"]
                            └── ["7a\x00\x02\x00\xff", "7a\x00\x03")

Why can't we just disallow DESC for now?

Good point. I still think it's odd to give the impression that inverted index columns have a semantic ordering. The indexed column contains only one dimension of the value, and an inverted index could never support sort-free ordering on the inverted index column.

Postgres disallows ASC on GIN indexes, which is one of the reasons I removed it:

marcus=# create table t (a int[]);
CREATE TABLE

marcus=# create index on t using gin (a asc);
ERROR:  0A000: access method "gin" does not support ASC/DESC options
LOCATION:  ComputeIndexAttrs, indexcmds.c:2033

But allowing it would be more backward compatible...

@cockroachdb/sql-experience any thoughts?

@rafiss
Copy link
Collaborator

rafiss commented Jul 18, 2022

assorted thoughts:

  • Are we able to use any form of product telemetry to determine how common using gin (a asc) expressions are?
  • What if we showed a NOTICE saying that ASC is ignored (if it is specified), rather than an error?
  • When it comes to backward compat concerns, let's involve the Product team. cc @vy-ton

@mgartner mgartner force-pushed the 84388-asc-desc-inv-idx branch from f312dc9 to 1670877 Compare July 19, 2022 22:56
@mgartner mgartner changed the title sql: disallow ASC/DESC options in the last column of inverted indexes sql: disallow DESC option in the last column of inverted indexes Jul 19, 2022
@mgartner
Copy link
Collaborator Author

I've updated this PR to only disallow DESC and stop printing ASC in the output of SHOW CREATE TABLE for inverted index columns. This should not be considered a breaking change because creating an inverted index column with DESC would cause internal errors prior to this commit.

Copy link
Collaborator

@rytaft rytaft 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 8 of 8 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @chengxiong-ruan and @michae2)

@mgartner mgartner force-pushed the 84388-asc-desc-inv-idx branch from 1670877 to 19a8d7d Compare July 20, 2022 14:12
The last column of inverted indexes cannot have the `DESC` direction.
Prior to this commit it was allowed, but caused internal errors. To
avoid propagating the notion that inverted indexes have a semantic
direction, we stop printing the `ASC` option for inverted index columns
in the output of `SHOW CREATE TABLE`.

Fixes cockroachdb#84388

Release note (sql change): The last column of an INVERTED INDEX can no
longer have the `DESC` option. If `DESC` was used in prior versions, it
could cause internal errors.
@mgartner mgartner force-pushed the 84388-asc-desc-inv-idx branch from 19a8d7d to ad1c8fb Compare July 20, 2022 16:30
@@ -1689,5 +1707,5 @@ SELECT primes FROM cb WHERE primes && numbers ORDER BY primes

# Regression test for incorrectly unwrapping a DOidWrapper (#84569).
statement ok
CREATE TABLE t84569 (name_col NAME NOT NULL, INVERTED INDEX (name_col gin_trgm_ops DESC));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've confirmed that the bug was reproducible without DESC, so this regression test remains useful.

@mgartner
Copy link
Collaborator Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 20, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jul 20, 2022

Build succeeded:

@craig craig bot merged commit 200cfaf into cockroachdb:master Jul 20, 2022
@mgartner mgartner deleted the 84388-asc-desc-inv-idx branch July 21, 2022 15:36
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.

sql: descending inverted indexes do not work
4 participants