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

tables: Remove INDEX requirement for ADDITIONAL option #6104

Merged
merged 2 commits into from
Dec 10, 2019

Conversation

theopolis
Copy link
Member

We do not need an explicit INDEX column for ADDITIONAL to work. If the ADDITIONAL option is set the constraint should be passed into the virtual table context.

directionless
directionless previously approved these changes Dec 7, 2019
Copy link
Member

@directionless directionless left a comment

Choose a reason for hiding this comment

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

Seems reasonable. Feels this is very much a choice of the underlying implementation

@theopolis
Copy link
Member Author

Yeah, I’m going to take time and review the virtual table code again to make sure this does not have side effects.

@theopolis
Copy link
Member Author

IIRC tables that have the ADDITIONAL option should not implement ROWID.

@directionless
Copy link
Member

How many table implement rowid anyhow?

We do not need an explicit INDEX column for ADDITIONAL to work.
If the ADDITIONAL option is set the constraint should be passed
into the virtual table context.
@theopolis
Copy link
Member Author

All tables without a column marked index=True will implement rowid. Tables that do not always full-scan should not have a rowid. This protects them from unwanted optimization within SQLite.

Tables without a defined primary key, and an additional behavior, will now implicitly define a primary key as a unique row via defining a primary key equal to all columns in a row.

@directionless
Copy link
Member

In lieu of deeper docs, from today's office hours:

The intent of #6104 is to start cleaning this up. We think:

  1. The underlying table implementions will only get column constraints that have additional or index set
  2. index hints to sqlite that that column is indexed, and should be taken into account during join planning
  3. additional is passed to the underlying implementation, but not be taken into query planning

And for primary keys:

index columns additional columns resultant primary key
Yes No Union of index columns
Yes Yes Union of index and additional
No Yes Union of all columns
No No Internal sqlite rowid

A good example of how this plays out, is the routes table. It has an
additional column which changes what's generated. But, it's not
indexed. However, if we use the internal rowid calling routes with
the different additional options, they'll overwrite eachother. Thus,
using all columns.

Copy link
Member

@directionless directionless left a comment

Choose a reason for hiding this comment

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

Seems okay? I did not build/test though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants