L044, L045: Handle Exasol VALUES clause #2400
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2400 +/- ##
=======================================
Coverage 99.93% 99.93%
=======================================
Files 162 162
Lines 11522 11529 +7
=======================================
+ Hits 11514 11521 +7
Misses 8 8
Continue to review full report at Codecov.
|
| test_pass_exasol_values_clause_cte: | ||
| pass_str: | | ||
| WITH txt AS ( | ||
| VALUES (1) | ||
| AS t (id) | ||
| ) | ||
|
|
||
| SELECT * | ||
| FROM txt |
There was a problem hiding this comment.
This isn't valid snowflake/postgres/mysql
so maybe use
configs:
core:
dialect: exasolin these test cases
There was a problem hiding this comment.
This however is valid snowflake/postgres (although annoyingly doesn't parse in postgres atm) and also raises the same L044 issue:
-
On main branch
-
On your branch:
L044 still there and catastophic failures 😱 🔥
My guess is that L025 and L026 are also rules that use your select_crawler and this values change has messed those up (at least in this case)
There was a problem hiding this comment.
(I'll fix the postgres parsing for my example above since I know exactly where I've accidently introduced this, but that's not related to the above issue 😄 )
EDIT: Addressed in #2401
There was a problem hiding this comment.
Came up with the equivalent MySQL test case but it won't parse so I've added it into issue #2402. I have a feeling if it could parse then it would likely be solved by your fix for the postgres/snowflake example so we can leave it for a later date for now.
There was a problem hiding this comment.
Oops, I'll add the exasol config. I intended to do that, but forgot. Will also add the same test for snowflake dialect.
There was a problem hiding this comment.
I added a postgres test case as well.
There was a problem hiding this comment.
@jpy-git: Think we could create a separate issue for your SELECT * test case? I agree it should be fixed, but I think it's less likely to occur in production code. Happy to look into it, but I'd rather address it later (potentially next release).
There was a problem hiding this comment.
Hmm it's kind of the same as the existing test case just in a subquery rather than a CTE, and we'd be introducing catastrophic errors in rules L025 + L026. So don't think we should be merging this before at least the L025 + L026 errors are fixed.
Worst case this PR runs into next release so don't think we should rush.
There was a problem hiding this comment.
I'll try and take a look this afternoon. I often have a couple of hours to work, then I don't always know when I'll have time again. 👶
…github.com/barrywhart/sqlfluff into bhart-issue_l044_l045_handle_values_clause
Co-authored-by: Joseph Young <80432516+jpy-git@users.noreply.github.com>
Co-authored-by: Joseph Young <80432516+jpy-git@users.noreply.github.com>
|
@jpy-git, @tunetheweb: I believe I've addressed the PR feedback, including the additional fixes. The only failure(s) I'm aware of now are caused by issue #2406 (which I created today). That's apparently a dialect issue which I think we should address separately. No other changes to |
There was a problem hiding this comment.
@barrywhart Looks good, I've tested and can confirm those L025/L026 error have gone too 👍 LGTM 🚀 ✨
As you said I think once we get that remaining Postgres/MySQL/Exasol Values grammar parsing, they should be handled by this rule in the exact same way. I'm looking at that in #2404 so will try and add extra unit tests to that to cover these statements
Brief summary of the change made
Fixes #2362
Enhances
SelectCrawlerclass to interpretVALUESclause in a CTE as an equivalentSELECT.Are there any other side effects of this change that we should be aware of?
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.