Postgres: treat GENERATE_SERIES as a value table function
#3463
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3463 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 171 171
Lines 12898 12898
=========================================
Hits 12898 12898
Continue to review full report at Codecov.
|
|
@alanmcruickshank another simple one that's good for v1.0.0 if you see this in time. |
| @@ -43,6 +43,18 @@ test_ignore_postgres_value_table_functions: | |||
| core: | |||
| dialect: postgres | |||
|
|
|||
| test_ignore_postgres_value_table_functions_generate_series: | |||
There was a problem hiding this comment.
Would it be worth adding an opposite test, like a similar query but a different function name?
There was a problem hiding this comment.
Don't think we should put a random, gibberish function in can't think of a function that you could put in a FROM clause without it being a value table function.
Maybe all functions in FROM clauses should be value table functions rather than explicitly listing them? But let's keep this change simple since we're just about to release so that's out of scope for now.
| @@ -134,7 +134,7 @@ def _has_value_table_function(table_expr, dialect): | |||
| # Other rules can increase whitespace in the function name, so use strip to | |||
| # remove | |||
| # See: https://github.com/sqlfluff/sqlfluff/issues/1304 | |||
| if function_name.raw.lower().strip() in dialect.sets("value_table_functions"): | |||
| if function_name.raw.upper().strip() in dialect.sets("value_table_functions"): | |||
There was a problem hiding this comment.
Why did this need to change? Did we have dialects using both upper and lower-case to specify value table functions?
There was a problem hiding this comment.
I noted above in the "Are there any other side effect..." section:
I uppercased value table values in common with other syntax
Basically, I personally just found it weird that other functions and keywords were in caps, but these were in lowercase.
I considered making it case insensitive but there's a (very slight) performance impact as have to create a new set, and even though the sets are small it's not worth it IMHO.
Brief summary of the change made
Fixes #3462
Are there any other side effects of this change that we should be aware of?
I uppercased value table values in common with other syntax
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:
.ymlrule test cases intest/fixtures/rules/std_rule_cases..sql/.ymlparser 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.