Extend support for INDEX parsing

[LucaCappelletti94]

This pull request addresses issue #1706.

• Added tests reproducing issue Failure while parsing GIN Index #1706
• Added enumerations and keywords to properly represent operator classes
• Extended Index struct to be able to characterize indices such as those presented in the issue

Summary of changes:

• Added a new document operator_classes.rs containing the classes
• Added all of the necessary keywords to characterize the parsing of the operator classes
• Extended enumeration of available Indices ddl.rs
• Introduced a new struct IndexColumn
• Extended parser to properly handle the coupled parsing of different Indices and associated Operator Classes
• Updated accordingly the test suite

[iffyio]

Marking this as draft in the meantime as its no longer awaiting review, @LucaCappelletti94 please feel free to undraft and ping when ready!

[LucaCappelletti94]

At this time I am somewhat stuck as I would not know how to parse the class operator names if not as a keyword - do you have any examples that could be applied to this use case?

[iffyio]

At this time I am somewhat stuck as I would not know how to parse the class operator names if not as a keyword - do you have any examples that could be applied to this use case?

@LucaCappelletti94 (not sure I followed entirely) did you mean to parse and represent an operator class like gist_oid_ops? If so I imagine we could parse the next token and check that its an unquoted Token::Word(_) and represent the class in the AST as an opaque String for example

[LucaCappelletti94]

While things may change in the future, to the best of my knowledge some indices can have operator classes and some cannot. Since we model the different indices, is it preferable if I continue to model this semantical difference between them? Or should I remove it?

I believe that different indices which accept different parametrizations are more similar to syntax than semantics. Do let me know what is your opinion on it.

[iffyio]

Sorry im not sure I understood the question, could you clarify with a sql example if that's possible?

[LucaCappelletti94]

Sure, so:

In a GIN index, there MAY be an operator class from a provided limited set, in the following example gin_trgm_ops

CREATE INDEX documents_content_trgm_idx ON documents USING GIN (content gin_trgm_ops);

In a BRIN index, instead, to the best of my knowledge there are no such operator classes. I base this claim on having read the actual postgres source code, as unfortunately the documentation is rather scarse in regard of which operator classes are available.

CREATE INDEX logs_event_time_brin_idx ON logs USING BRIN (event_time);

One more important thing is that the actual index type, in these cases respectively GIN and BRIN, while currently modeled as an enumeration, is actually something as customizable as a table since I could always introduce a new index via a custom extension.

If the Operator classes are to be treated as generic strings, it may be the case that also index types should be treated as such, if the goal is to make the library generically future-proofed. I am not certain it is necessarily the best way forward, as it may become exceedingly generic, but most likely this is my need to validate the semantics of the SQL that sneaks in.

Summarizing, let me know your opinion of these three questions:

1. Should there be an enumeration of index types (plus maybe a CustomIndexType(String) variant? Or should that too be replaced with a simpler IndexType(String)?
2. If we keep the index enum, should there be a check on whether the provided index type is known to support Operator classes (which is how I implemented it now, with the generic dispatch)?
3. If we keep the dispatch for validating the operator classes, maybe it would be reasonable to still keep the enumeration of the operator classes PLUS a CustomOperatorClass(String) thing? Or, if we remove any of the above, switch to a Option<OperatorClass(String)> and be done with it?

[iffyio]

Ah I see thanks for clarifying!

1. I think introducing a Custom variant would be reasonable, it would be similar to the existing pattern of DataType::Custom, BinaryOperator::Custom etc that we have. The existing Hash and BTree I figure we can keep as is, represented explicitly, partily to avoid breaking changes more than necessary and also since those types are the most common index types across dialects
2. I don't think we need to check that, we can look to parse the operator class if one is present in the input.
3. I think we can keep IndexType as the enum in (1), then essentially operator class in (2) is an opaque String.

[LucaCappelletti94]

I have tried to replace the parsing of keywords with the generic String Ident as proposed, but it leads to much more complex parsing.

Primarily, I don't understand how I could distinguish an operator class from a keyword, such as: CREATE UNIQUE INDEX IF NOT EXISTS idx_name ON test USING BTREE (name gin_trgm_ops,age DESC), specifically how can you tell that gin_trgm_ops is to be parsed as an operator class and DESC should instead be a keyword? Should I try to parse it as a keyword, and it it fails I assume it is an operator class?

[LucaCappelletti94]

I have managed to do it, but I really dislike the approach I used, which peeks whether the parser will next encounter one of the expected keywords and if not, assumes that the next token is an operator class. I much preferred the previous approach that distinctively treated operator classes as keywords which was much stricter, but at some point it boils down to personal opinion.

Let me know your opinion on it.

[LucaCappelletti94]

I tried to do the merge using the GitHub tool but it was lagging immensely, I will now fix it offline.

[iffyio]

this test scenario looks identical to parse_create_users_name_trgm_index in terms of coverage?
Im thinking generally for the tests we can group them into the same test function, it would be sufficient with the AST assertion on only one of the test scenarios then for the rest we can rely only on verified_stmt(), that would keep the tests code smaller and easier to track what part of the syntax is covered.

Then could we add scenarios for all the introduced index types (i.e. BRIN, BLOOM etc)? As well as the custom type which probably would live in common I imagine?

[iffyio]

LGTM! Thanks @LucaCappelletti94!
cc @alamb