Skip to content

Conversation

@miuy56dc
Copy link
Contributor

@miuy56dc miuy56dc commented Jul 29, 2020

...by reusing OrderByExpr for columns in Statement::CreateIndex.

This supports SQLite's indexed-column syntax https://www.sqlite.org/syntax/indexed-column.html

MSSQL's (ON <object> ( column [ ASC | DESC ] [ ,...n ] ))
https://docs.microsoft.com/en-us/sql/t-sql/statements/create-index-transact-sql?view=sql-server-ver15

And most of PostgreSQL syntax (except for opclass):
( { column_name | ( expression ) } [ COLLATE collation ] [ opclass ] [ ASC | DESC ] [ NULLS { FIRST | LAST } ] [, ...] )
https://www.postgresql.org/docs/12/sql-createindex.html

@coveralls
Copy link

coveralls commented Jul 29, 2020

Pull Request Test Coverage Report for Build 188045566

  • 10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 91.8%

Totals Coverage Status
Change from base Build 186377189: 0.06%
Covered Lines: 4411
Relevant Lines: 4805

💛 - Coveralls

Copy link
Contributor

@nickolay nickolay left a comment

Choose a reason for hiding this comment

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

Please refer to the previous discussion in new PRs.

I still don't see why not re-use self.parse_comma_separated(Parser::parse_order_by_expr) instead of introducing another AST type. OrderByExpr should be able to express all of sqlite's options, and Postgres NULLS FIRST as well.

@nickolay
Copy link
Contributor

Thanks! Any reason you prefer indexed_columns to simply columns or is it a left-over from the previous version of the PR?

@miuy56dc
Copy link
Contributor Author

Because I think column-def and indexed-column are different, so I use indexed-columns to distinguish the difference, but it can be implied to columns because I define it inside of CreateIndex.

Copy link
Contributor

@nickolay nickolay left a comment

Choose a reason for hiding this comment

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

Great. Thanks!

@nickolay nickolay merged commit 4452f9b into apache:main Jul 30, 2020
@nickolay nickolay changed the title add IndexedColumn to support SQLite create index grammar Support specifying ASC/DESC in index columns Aug 12, 2020
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