- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 756
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
Postgres: CREATE COLLATION
support
#3571
Conversation
@@ -925,18 +925,21 @@ def get_keywords(keyword_list, keyword_type): | |||
("DATE", "non-reserved"), | |||
("DEPTH", "non-reserved"), | |||
("DESCRIBE", "non-reserved"), | |||
("DETERMINISTIC", "non-reserved"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone can explain what goes into postgres_docs_keywords
vs postgres_nondocs_keywords
, it would be great to document that in the code. I just guessed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing from the name that postgres_nondocs_keywords
refers to keywords that appear to be keywords but are not listed in https://www.postgresql.org/docs/current/sql-keywords-appendix.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the distinction even matter? Does it matter if I've put in the "wrong" place for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it doesn’t presently. Type is important but not which list it appears in.
Ref("ColumnConstraintSegment", optional=True) | ||
OneOf( | ||
Ref("ColumnConstraintSegment", optional=True), | ||
Sequence( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I read this correctly, this allows any number of COLLATE
s, e.g.:
COLLATE a COLLATE b COLLATE c
Is this the intent? I don't see any tests doing this. Should we add one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From reading the docs, I think only one COLLATE
clause is permitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it's not possible to do multiple COLLATE
. I did it this way because COLLATE numeric NOT NULL
and NOT NULL COLLATE numeric
are both legal.
It could instead be ...
Sequence(
"COLLATE",
Ref("ObjectReferenceSegment"),
optional=True,
),
AnyNumberOf(
Ref("ColumnConstraintSegment", optional=True)
),
Sequence(
"COLLATE",
Ref("ObjectReferenceSegment"),
optional=True,
),
... so the collate sequence can go either before or after constraints. Or is there a better way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to the test I put, just ran this in Postgres and it worked:
gregoryfinley=# CREATE TABLE orders3
(
order_number text NOT NULL COLLATE numeric,
order_number2 text COLLATE numeric NOT NULL
);
CREATE TABLE
gregoryfinley=#
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, and it should be okay. The dialect is often more permissive/flexible than what is actually allowed (or makes sense to do).
I'd like @tunetheweb to review this PR as well, since he is more the expert on dialect changes. He's on vacation right now, so I'm doing a preliminary review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this works better: 010cb8b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok but now we don’t have the CONSTRAINT
+ COLLATE
+ CONSTRAINT
example so someone could try to clean this up in future using the AnySetOf
syntax I proposed above and not realise we specifically choose NOT to use that. So I think you need all three tests for complete coverage?
Though, to be honest, think it’s pretty unlikely anyone would actually use that third type, but there’s all sorts out there!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my test is an example of CONSTRAINT
+ COLLATE
+ CONSTRAINT
, isn't it?
order_number text UNIQUE COLLATE numeric NOT NULL
UNIQUE
is a constraint, then COLLATE
, then NOT NULL
is a second constraint.
Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah you’re right. What you’re missing then is COLLATE
+ CONSTRAINT
example.
Though really getting picky now so happy enough to just merge if you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be everything? Let me know if not: 0f95ac5
Codecov Report
@@ Coverage Diff @@
## main #3571 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 174 174
Lines 13146 13149 +3
=========================================
+ Hits 13146 13149 +3
Continue to review full report at Codecov.
|
CREATE COLLATION
support
Brief summary of the change made
Closes #3494
Are there any other side effects of this change that we should be aware of?
No
Pull Request checklist
Please confirm you have completed any of the necessary steps below.
Included test cases to demonstrate any code changes, which may be one or more of the following:
.yml
rule test cases intest/fixtures/rules/std_rule_cases
..sql
/.yml
parser test cases intest/fixtures/dialects
(note YML files can be auto generated withtox -e generate-fixture-yml
).test/fixtures/linter/autofix
.Added appropriate documentation for the change.
Created GitHub issues for any relevant followup/future enhancements if appropriate.