Repository navigation
Don't assert-crash the analyzer on a non-lambda -> operator - #8399
Conversation
_get_lambda_argument_columns treated every `->` as a lambda arrow and asserted that exactly one lambda-parameter segment preceded it. In dialects that reuse `->` as a binary operator (duckdb/trino JSON extraction, e.g. `1::json -> 'a'`) the left operand isn't a lambda parameter, so the assert fired and surfaced as an 'Unexpected exception' on AL04/AL05/AM04/RF01/RF02/RF03/ST05. Skip the operator when the pre-arrow segments aren't a single lambda parameter instead of asserting; genuine lambdas are unaffected.
Coverage Results ✅ |
peterbud
left a comment
peterbud
left a comment
There was a problem hiding this comment.
Hi @winklemad , thanks for this contribution.
The fix prevents the crash, but still misclassifies DuckDB JSON expressions like payload -> 'a' as lambdas when the left operand is a single column. This causes RF02 false negatives in multi-table queries. Please distinguish lambdas using the parsed segment types rather than operand count, and add a plain-column JSON regression case.
Counting the segments before the arrow still classified a JSON extraction like `payload -> 'a'` as a lambda whenever the left operand happened to be a single column, which made RF02 miss the unqualified reference. duckdb, trino and snowflake parse a real lambda as a lambda_function holding a lambda_arrow, so a plain `->` binary operator in those dialects is never a lambda. athena, sparksql and databricks parse lambdas as an ordinary expression and have no JSON arrow, so they keep matching on the binary operator. The count check stays as a guard so an unexpected shape returns no columns rather than raising.
|
Good catch, you're right — counting operands still called it a lambda whenever the left side happened to be one column, so RF02 quietly stopped flagging Switched to segment types in c928bf7. I checked how each of the six dialects actually parses this:
So for the first group a bare Added the plain-column case you asked for as |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Confidence score: 5/5
- In
test/fixtures/rules/std_rule_cases/RF02.yml, the twofail_strfixtures are masked by the unrelated unqualifiednviolation, so they can pass without testing the behavior described in their comments; revise the queries to isolate the intended rule condition.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/fixtures/rules/std_rule_cases/RF02.yml">
<violation number="1" location="test/fixtures/rules/std_rule_cases/RF02.yml:214">
P3: These two fail_str fixtures don't test what their comments claim. In `select list_filter(t1.l, x -> x > n) from t1 join t2 ...`, the unrelated unqualified column `n` always produces a violation, so the test passes whether or not `x` is correctly recognized as a lambda parameter. If a future change to `_get_lambda_argument_columns` stops recognizing real lambdas, `x` gets reported as unqualified but `assert_rule_fail_in_sql` still passes because `n` (line 3) is flagged. The comment says "must not be reported," but a fail_str only asserts that at least one violation exists; it can't assert which column is flagged. Make these fixtures sensitive to the invariant, e.g. scope them so the only possible violation is the lambda parameter, or assert the exact violation set/line numbers rather than relying on the always-present `n`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| core: | ||
| dialect: trino | ||
|
|
||
| test_lambda_parameters_are_still_standalone_duckdb: |
There was a problem hiding this comment.
P3: These two fail_str fixtures don't test what their comments claim. In select list_filter(t1.l, x -> x > n) from t1 join t2 ..., the unrelated unqualified column n always produces a violation, so the test passes whether or not x is correctly recognized as a lambda parameter. If a future change to _get_lambda_argument_columns stops recognizing real lambdas, x gets reported as unqualified but assert_rule_fail_in_sql still passes because n (line 3) is flagged. The comment says "must not be reported," but a fail_str only asserts that at least one violation exists; it can't assert which column is flagged. Make these fixtures sensitive to the invariant, e.g. scope them so the only possible violation is the lambda parameter, or assert the exact violation set/line numbers rather than relying on the always-present n.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/fixtures/rules/std_rule_cases/RF02.yml, line 214:
<comment>These two fail_str fixtures don't test what their comments claim. In `select list_filter(t1.l, x -> x > n) from t1 join t2 ...`, the unrelated unqualified column `n` always produces a violation, so the test passes whether or not `x` is correctly recognized as a lambda parameter. If a future change to `_get_lambda_argument_columns` stops recognizing real lambdas, `x` gets reported as unqualified but `assert_rule_fail_in_sql` still passes because `n` (line 3) is flagged. The comment says "must not be reported," but a fail_str only asserts that at least one violation exists; it can't assert which column is flagged. Make these fixtures sensitive to the invariant, e.g. scope them so the only possible violation is the lambda parameter, or assert the exact violation set/line numbers rather than relying on the always-present `n`.</comment>
<file context>
@@ -195,6 +195,37 @@ test_json_arrow_operator_is_not_a_lambda_trino:
+ core:
+ dialect: trino
+
+test_lambda_parameters_are_still_standalone_duckdb:
+ # The lambda parameter `x` is declared in place and must not be reported.
+ fail_str: |
</file context>
Add an RF02 case where the left operand of `->` is a function call rather than a single lambda parameter, exercising the guard that skips such a shape instead of crashing. Keeps the module at 100% line coverage.
|
Thanks @peterbud — and good catch on the RF02 false-negative; switching to segment types made it more robust than the operand-count check I started with. |
…399) ## Summary - Port SQLFluff #8399: `get_lambda_argument_columns` no longer asserts on the shape of the segment before a `->`. In dialects with a dedicated lambda function (duckdb, trino, snowflake) a bare `->` is JSON extraction, not a lambda, so those expressions are skipped; dialects that model lambdas as a plain `->` expression (athena, sparksql, databricks) keep the old path. An unrecognised parameter list now contributes no columns instead of crashing. - Port the RF02 fixture cases, omitting the two upstream trino JSON cases: sqruff's trino dialect does not yet parse the `->` JSON extraction operator, so the duckdb equivalents (cast operand, plain-column operand) cover the same analyzer paths. Ported from SQLFluff cc9b54e47a44b6f644b6777626b46269a68f121e sqlfluff/sqlfluff#8399 sqlfluff/sqlfluff@cc9b54e Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01A9tYxcHmKFezu3vWeSHAbv
…399) ## Summary - Port SQLFluff #8399: `get_lambda_argument_columns` no longer asserts on the shape of the segment before a `->`. In dialects with a dedicated lambda function (duckdb, trino, snowflake) a bare `->` is JSON extraction, not a lambda, so those expressions are skipped; dialects that model lambdas as a plain `->` expression (athena, sparksql, databricks) keep the old path. An unrecognised parameter list now contributes no columns instead of crashing. - Port the RF02 fixture cases, omitting the two upstream trino JSON cases: sqruff's trino dialect does not yet parse the `->` JSON extraction operator, so the duckdb equivalents (cast operand, plain-column operand) cover the same analyzer paths. Ported from SQLFluff cc9b54e47a44b6f644b6777626b46269a68f121e sqlfluff/sqlfluff#8399 sqlfluff/sqlfluff@cc9b54e Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01A9tYxcHmKFezu3vWeSHAbv
…399) ## Summary - Port SQLFluff #8399: `get_lambda_argument_columns` no longer asserts on the shape of the segment before a `->`. In dialects with a dedicated lambda function (duckdb, trino, snowflake) a bare `->` is JSON extraction, not a lambda, so those expressions are skipped; dialects that model lambdas as a plain `->` expression (athena, sparksql, databricks) keep the old path. An unrecognised parameter list now contributes no columns instead of crashing. - Port the RF02 fixture cases, omitting the two upstream trino JSON cases: sqruff's trino dialect does not yet parse the `->` JSON extraction operator, so the duckdb equivalents (cast operand, plain-column operand) cover the same analyzer paths. Ported from SQLFluff cc9b54e47a44b6f644b6777626b46269a68f121e sqlfluff/sqlfluff#8399 sqlfluff/sqlfluff@cc9b54e Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01A9tYxcHmKFezu3vWeSHAbv
Fixes #8398
_get_lambda_argument_columnstreated every->as a lambda arrow and didassert len(argument_segments) == 1. When->is used as a binary/JSON-arrow operator instead — e.g.1::json -> 'a'on duckdb/trino, where the left operand is a cast rather than a single lambda parameter —argument_segmentsis empty and the hard assert fires. Because it runs duringget_select_statement_info,BaseRule.crawlcatches it and every affected rule (AL04, AL05, AM04, RF01, RF02, RF03, ST05) reports an "Unexpected exception" on otherwise-valid SQL.This replaces the assert with a guard: when the segments before
->aren't exactly one lambda parameter, it isn't a lambda, so it contributes no lambda argument columns and we skip it instead of crashing. Genuine lambdas (x -> x + 1,(x, y) -> x + y) are unaffected.Added RF02 fixture cases for
1::json -> 'a'on duckdb and trino; they fail onmainwith theAssertionErrorand pass with this change. The existing analysis/rule suites (including the lambda cases) stay green.AI usage
I used an AI assistant (Claude Code) to help navigate the analysis code and draft the fix and fixtures. I found and reproduced the crash myself on sqlfluff 4.3.0 (the duckdb/trino
1::json -> 'a'case, and confirmed databricks does not trigger it), verified the root cause is the lambda-arrow assert over-matching a binary->, and confirmed the new cases fail onmainand pass after while the lambda tests still pass. I've reviewed every line and stand by it.Summary by cubic
Prevents the analyzer from assert-crashing when
->is a binary/JSON operator (e.g.,1::json -> 'a'on duckdb/trino) instead of a lambda arrow, and stops it from misclassifying JSON extraction as a lambda.Bug Fixes
lambda_functionholding alambda_arrow, so a plain->binary operator in those dialects is never a lambda.binary_operator.Written for commit e39bade. Summary will update on new commits.