Skip to content

Add support for explicitly selecting from an index.#2057

Merged
petermattis merged 1 commit intomasterfrom
pmattis/sql-index
Aug 11, 2015
Merged

Add support for explicitly selecting from an index.#2057
petermattis merged 1 commit intomasterfrom
pmattis/sql-index

Conversation

@petermattis
Copy link
Copy Markdown
Collaborator

Added support for "SELECT * FROM table@index". This is an extension
which allows us to more explicitly test our index scanning. The index
must exist within the table and the only columns accessible are those
the index is defined on.

See #2046.

Comment thread sql/parser/expr.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

comments for these? Especially whether they can both be true.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1. this should probably be an enum

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

alternatively, maybe it's time to split this into separate types.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Splitting into separate types is problematic due to the grammar changes. (I actually started out trying that approach and quickly gave up). Changing to an enum. It started as a single bool and then evolved.

@tamird
Copy link
Copy Markdown
Contributor

tamird commented Aug 11, 2015

Mostly LGTM. Some concern about the runtime assertions on QualifiedName

@petermattis
Copy link
Copy Markdown
Collaborator Author

@tamird The panics in QualifiedName should never happen. Seems reasonable to panic instead of returning an error up the stack.

Comment thread sql/testdata/select Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you add SELECT x FROM test.xyz@foo to exercise the error case? also nit from should be capitalized

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done and done.

@tamird
Copy link
Copy Markdown
Contributor

tamird commented Aug 11, 2015

@tamird The panics in QualifiedName should never happen. Seems reasonable to panic instead of returning an error up the stack.

Yeah, that seems fine. My comment was intended to suggest that Normalize{Table,Column}Name could have return values of type {Table,Column}Name, which would be slightly cleaner.

@tamird
Copy link
Copy Markdown
Contributor

tamird commented Aug 11, 2015

LGTM

Comment thread sql/testdata/select Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

did you mean to add the select x case i described?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, though really I just misunderstood your comment and missed that error case. Good thing to add it because it found a bug.

Added support for "SELECT * FROM table@index". This is an extension
which allows us to more explicitly test our index scanning. The index
must exist within the table and the only columns accessible are those
the index is defined on.

See #2046.
petermattis added a commit that referenced this pull request Aug 11, 2015
Add support for explicitly selecting from an index.
@petermattis petermattis merged commit 68ee67e into master Aug 11, 2015
@petermattis petermattis removed the PTAL label Aug 11, 2015
@petermattis petermattis deleted the pmattis/sql-index branch August 11, 2015 23:51
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.

3 participants