-
-
Notifications
You must be signed in to change notification settings - Fork 750
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
Teradata: Added date as bare function #1663
Conversation
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.
change looks good. But some requests for the test cases if you could have a look?
@@ -0,0 +1 @@ | |||
SELECT DATE; |
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.
Nit, but could you add a new line to the end of the file? We have an editorconfig file if your editor supports that (most of them do) to enforce that without you having too.
Also are there any other examples we could use here? Can DATE be used with any params? If so could you add another example or two? Remember to regenerate the YAML after.
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.
Will do.
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.
SELECT DATE (FORMAT 'MMMbdd,bYYYY') (CHAR(12), UC);
from the docs doesn't parse.
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.
What about:
SELECT DATE (FORMAT 'MMMbdd,bYYYY')
for now? Could do the next (casting?) bit at another time. Unless you wanna figure out how to do that now to as part of this PR?
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.
Specifically, it doesn't like the (CHAR(12), UC)
bit - this might be something to address in a different MR.
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.
Codecov Report
@@ Coverage Diff @@
## main #1663 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 133 133
Lines 9316 9317 +1
=========================================
+ Hits 9316 9317 +1
Continue to review full report at Codecov.
|
Thanks for those other test case. They aren’t parsing as bare_functions but that’s fine (they aren’t bare functions!), but think good to have none the less. If tests pass we can approve and merge. |
Do I need to update changelog, contributors, etc? |
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.
First PR to the project. Woo hoo and thanks!!
Hooray! Thanks for the help! |
Brief summary of the change made
Fixes #1611 . I've added DATE as a bare function (line 89).
Are there any other side effects of this change that we should be aware of?
I also removed it from unreserved and reserved keywords - please let me know if this an incorrect change and I can adjust as necessary.
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 withpython test/generate_parse_fixture_yml.py
or by runningtox
locally).test/fixtures/linter/autofix
.Added appropriate documentation for the change.
Created GitHub issues for any relevant followup/future enhancements if appropriate.