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

Add INDEX USING HASH support #56

Merged
merged 2 commits into from
Nov 15, 2023
Merged

Conversation

de-luca
Copy link
Contributor

@de-luca de-luca commented Nov 15, 2023

Hi,

This PR adds support for hash sharded indexes.

It adds USING HASH [WITH ( bucket_size = X )] to:

  • column constraints
  • table constraints
  • CREATE INDEX statements

Also added:

  • support for alternate STORING keyword: INCLUDE and COVERING.
  • CONCURRENTLY on CREATE INDEX

Copy link
Collaborator

@Adriel-M Adriel-M left a comment

Choose a reason for hiding this comment

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

Can you also add a migration file for the integration test for the index hashing support? No need to assert anything. Just need to make sure it actually runs.

Folder can be found: https://github.com/Faire/sqldelight-cockroachdb-dialect/tree/main/integration-testing/src/main/resources/migration/com/faire/sqldelight/dialects/cockroachdb

You can run the gradle task :integration-testing:generateMigrationFile to create a migration file.

Thanks!

@@ -134,10 +156,10 @@ blob_data_type ::= 'BYTEA' | 'BLOB' | 'BYTES' {
override = true
}

create_index_stmt ::= CREATE [ UNIQUE ] INDEX [ IF NOT EXISTS ] [ [ ansi_database_name DOT ] ansi_index_name ] ON ansi_table_name LP ansi_indexed_column ( COMMA ansi_indexed_column ) * RP ['STORING' LP ansi_indexed_column ( COMMA ansi_indexed_column ) * RP ] [ WHERE <<expr '-1'>> ] {
create_index_stmt ::= CREATE [ UNIQUE ] INDEX [ 'CONCURRENTLY' ] [ IF NOT EXISTS ] [ [ ansi_database_name DOT ] ansi_index_name ] ON ansi_table_name LP ansi_indexed_column ( COMMA ansi_indexed_column ) * RP [ index_using_hash ] [('STORING' | 'COVERING' | 'INCLUDE') LP ansi_indexed_column ( COMMA ansi_indexed_column ) * RP ] [ WHERE <<expr '-1'>> ] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a test case for CONCURRENTLY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I forgot that one, my bad

@@ -94,8 +99,10 @@ overrides ::= table_constraint
| alter_table_rules
| extension_stmt

index_using_hash ::= 'USING' 'HASH' [ storage_options ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be good to use something else than storage_options and create your own just for the hash options.

The cockroach grammar only supports one option https://www.cockroachlabs.com/docs/stable/sql-grammar#opt_hash_sharded

While storage options can support multiple.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also there should be a SqlType for USING already

Copy link
Collaborator

@Adriel-M Adriel-M left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@Adriel-M Adriel-M merged commit c2f7918 into Faire:main Nov 15, 2023
3 checks passed
@de-luca de-luca deleted the index-using-hash branch November 15, 2023 16:16
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.

2 participants