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

feat!: Add complex index fields to CREATE INDEX statements #229

Merged

Conversation

dgmcdona
Copy link
Contributor

See postgresql CREATE INDEX docs

The CREATE INDEX statement should be able to support three different types of index fields:

  1. Column names
  2. Expressions (wrapped in parentheses)
  3. Function calls

This PR adds those features to the CREATE INDEX statement while preserving backwards compatibility with existing tests/queries.

There is some code duplication going on here that I couldn't figure out how to resolve; maybe someone has a suggestion for how this can be improved. Each of the three types of index fields takes a set of optional parameters, but I couldn't break them out into their own rule since they are all optional and a rule containing only optional fields would match the empty string rule and therefore be invalid. I also couldn't wrap the three types in a choice followed by the optional nodes since the optional nodes need to be children of the column node in order to not break existing tests/queries.

@dgmcdona
Copy link
Contributor Author

This works as well and is a little more concise, but changes the output slightly for the test that I added:

    _index_field: $ => seq(
      choice(
        field("column_expression", wrapped_in_parenthesis($._expression)),
        field("column_function", $.invocation),
        field("name", $._column),
      ),
      optional(seq($.keyword_collate, $.identifier)),
      optional($._operator_class),
      optional($.direction),
      optional(
        seq(
          $.keyword_nulls,
          choice(
            $.keyword_first,
            $.keyword_last
          )
        )
      ),
    ),

    index_fields: $ => wrapped_in_parenthesis(comma_list(alias($._index_field, $.column))),

    create_index: $ => seq(
      $.keyword_create,
      optional($.keyword_unique),
      $.keyword_index,
      optional($.keyword_concurrently),
      optional(
        seq(
          optional($._if_not_exists),
          field("column", $._column),
        ),
      ),
      $.keyword_on,
      optional($.keyword_only),
      seq(
        $.object_reference,
        optional(
          seq(
            $.keyword_using,
            choice(
              $.keyword_btree,
              $.keyword_hash,
              $.keyword_gist,
              $.keyword_spgist,
              $.keyword_gin,
              $.keyword_brin
            ),
          ),
        ),
        alias($.index_fields, $.ordered_columns)
      ),
      optional(
        $.where,
      ),
    ),

Copy link
Owner

@DerekStride DerekStride left a comment

Choose a reason for hiding this comment

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

I like your suggested change but I'd also make a few more. I don't think we need to worry about breaking changes to the AST being disruptive especially for the create_index node. We should still follow the conventional commit guidelines here for breaking changes though.

cc: @dmfay @matthias-Q

grammar.js Outdated Show resolved Hide resolved
grammar.js Outdated Show resolved Hide resolved
grammar.js Outdated Show resolved Hide resolved
BREAKING CHANGE: The `(ordered_columns)` node in the `create_index`
statement has been replaced with `(index_fields)`, with `(field)` child
nodes which can have `column: (_)`, `expression: (_)` or `function:
(_)` named child nodes.
@dgmcdona dgmcdona force-pushed the dgm/create_index_enhance_index_fields branch from 61e1dd6 to 97806c5 Compare November 28, 2023 04:17
@dgmcdona dgmcdona changed the title feat: Add complex index fields to CREATE INDEX statements feat!: Add complex index fields to CREATE INDEX statements Nov 28, 2023
@dgmcdona
Copy link
Contributor Author

Thanks for the feedback! I made those changes and updated the commit message/PR title to reflect breaking changes.

@DerekStride DerekStride merged commit 7148a47 into DerekStride:main Dec 7, 2023
4 checks passed
@DerekStride
Copy link
Owner

I merged this to move things forward but if there's any concerns I can follow up in another PR

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