Skip to content

Commit 6a55dfb

Browse files
committed
fix(semantics): stop reading a lambda parameter as an ungrouped column
The grouping check walked into lambda bodies and reported a lambda's own parameter, such as e in list_filter(ARRAY_AGG(x), lambda e: e <> 'null'), as a column that must be grouped. A parameter is bound only inside its lambda's body, so it is now exempt there, while outer columns in the body and a column of the same name outside the lambda are still checked.
1 parent 69ba284 commit 6a55dfb

2 files changed

Lines changed: 138 additions & 7 deletions

File tree

‎crates/sqlquack-semantics/src/duckdb/profile/tests/test_statement_findings.rs‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -523,6 +523,84 @@ fn given_from_items_that_read_each_other_when_resolving_names_then_each_reads_on
523523
}
524524
}
525525

526+
#[test]
527+
fn given_a_lambda_in_a_grouped_select_when_checking_grouping_then_only_outer_columns_must_be_grouped()
528+
{
529+
// Each verdict is DuckDB 2.0.0's.
530+
let test_cases = [
531+
QueryFindingsTestCase {
532+
description: "a lambda parameter, which is no column",
533+
sql: "SELECT o.customer_id, len(list_filter(ARRAY_AGG(o.status), lambda e: e <> \
534+
'null')) AS c FROM orders AS o GROUP BY o.customer_id",
535+
expected_findings: &[],
536+
},
537+
QueryFindingsTestCase {
538+
description: "a lambda parameter cast over a JSON list of an aggregate",
539+
sql: "SELECT o.customer_id, len(list_filter(CAST(to_json(ARRAY_AGG(o.status)) AS \
540+
JSON[]), lambda e: CAST(e AS VARCHAR) <> 'null')) AS c FROM orders AS o GROUP BY \
541+
o.customer_id",
542+
expected_findings: &[],
543+
},
544+
QueryFindingsTestCase {
545+
description: "a grouped outer column inside the lambda body",
546+
sql: "SELECT o.customer_id, len(list_filter(ARRAY_AGG(o.status), lambda e: e <> \
547+
CAST(o.customer_id AS VARCHAR))) AS c FROM orders AS o GROUP BY o.customer_id",
548+
expected_findings: &[],
549+
},
550+
QueryFindingsTestCase {
551+
description: "an ungrouped outer column inside the lambda body",
552+
sql: "SELECT o.customer_id, len(list_filter(ARRAY_AGG(o.status), lambda e: e <> \
553+
o.status)) AS c FROM orders AS o GROUP BY o.customer_id",
554+
expected_findings: &[(
555+
"E230",
556+
"Column must be grouped or used in an aggregate function",
557+
76,
558+
)],
559+
},
560+
QueryFindingsTestCase {
561+
description: "a parameter's name read outside its lambda is a column",
562+
sql: "SELECT o.customer_id, concat(list_filter(ARRAY_AGG(o.status), lambda status: \
563+
status <> 'x')[1], status) AS c FROM orders AS o GROUP BY o.customer_id",
564+
expected_findings: &[(
565+
"E230",
566+
"Column must be grouped or used in an aggregate function",
567+
96,
568+
)],
569+
},
570+
QueryFindingsTestCase {
571+
description: "a lambda field the FROM item of that name also has is that item's column",
572+
sql: "SELECT list_transform([{'amount': 99}], lambda o: o.amount) AS v FROM orders AS o GROUP BY o.status",
573+
expected_findings: &[(
574+
"E230",
575+
"Column must be grouped or used in an aggregate function",
576+
52,
577+
)],
578+
},
579+
QueryFindingsTestCase {
580+
description: "a lambda field only the parameter has",
581+
sql: "SELECT list_transform([{'m': 99}], lambda o: o.m) AS v FROM orders AS o GROUP BY o.status",
582+
expected_findings: &[],
583+
},
584+
QueryFindingsTestCase {
585+
description: "a JSON arrow is no lambda, so its column must be grouped",
586+
sql: "SELECT o.items -> o.status AS v FROM orders AS o GROUP BY o.status",
587+
expected_findings: &[(
588+
"E230",
589+
"Column must be grouped or used in an aggregate function",
590+
9,
591+
)],
592+
},
593+
];
594+
for test_case in test_cases {
595+
assert_eq!(
596+
duckdb_parser_findings(test_case.sql),
597+
owned_findings(test_case.expected_findings),
598+
"{}",
599+
test_case.description
600+
);
601+
}
602+
}
603+
526604
#[test]
527605
fn given_duckdbs_parser_reads_window_calls_when_checking_arity_then_every_count_duckdb_binds_is_known()
528606
{

‎crates/sqlquack-semantics/src/lineage/_helpers/checks/scope_rules.rs‎

Lines changed: 60 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1008,9 +1008,10 @@ fn grouping(
10081008
}
10091009
let mut findings: Vec<NameFinding> = Vec::new();
10101010
for (expression, named) in read {
1011-
let mut pending: Vec<&Expression> = vec![unalias(expression)];
1011+
let mut lambdas: Vec<(usize, Vec<String>)> = vec![(0, Vec::new())];
1012+
let mut pending: Vec<(&Expression, usize)> = vec![(unalias(expression), 0)];
10121013
let mut expanded: HashSet<String> = HashSet::new();
1013-
while let Some(current) = pending.pop() {
1014+
while let Some((current, frame)) = pending.pop() {
10141015
if query_start(current)
10151016
|| is_aggregate(current)
10161017
|| matches!(current, Expression::WithinGroup(group) if is_aggregate(&group.this))
@@ -1021,8 +1022,21 @@ fn grouping(
10211022
{
10221023
continue;
10231024
}
1025+
if let Expression::Lambda(lambda) = current
1026+
&& (lambda.colon || Engine::of(level.context) != Engine::DuckDB)
1027+
{
1028+
let names: Vec<String> = lambda
1029+
.parameters
1030+
.iter()
1031+
.map(|name| level.key(name))
1032+
.collect();
1033+
lambdas.push((frame, names));
1034+
pending.push((&lambda.body, lambdas.len() - 1));
1035+
continue;
1036+
}
10241037
if let Expression::Column(column) = current {
1025-
if grouped(&column_key(column, level))
1038+
if lambda_parameter(column, (&lambdas, frame), level)
1039+
|| grouped(&column_key(column, level))
10261040
|| keyword_function(column, level.scope, level.context)
10271041
{
10281042
continue;
@@ -1031,7 +1045,7 @@ fn grouping(
10311045
let name: String = level.key(&column.name);
10321046
if let Some(alias) = aliases.get(&name) {
10331047
if expanded.insert(name) {
1034-
pending.push(alias);
1048+
pending.push((alias, frame));
10351049
}
10361050
continue;
10371051
}
@@ -1040,23 +1054,62 @@ fn grouping(
10401054
break;
10411055
}
10421056
if let Expression::WindowFunction(window) = current {
1043-
pending.extend(window.this.children());
1057+
pending.extend(
1058+
window
1059+
.this
1060+
.children()
1061+
.into_iter()
1062+
.map(|child| (child, frame)),
1063+
);
10441064
pending.extend(
10451065
current
10461066
.children()
10471067
.into_iter()
1048-
.filter(|child| !std::ptr::eq(*child, &window.this)),
1068+
.filter(|child| !std::ptr::eq(*child, &window.this))
1069+
.map(|child| (child, frame)),
10491070
);
10501071
} else {
10511072
let mut children: Vec<&Expression> = current.children();
10521073
children.reverse();
1053-
pending.extend(children);
1074+
pending.extend(children.into_iter().map(|child| (child, frame)));
10541075
}
10551076
}
10561077
}
10571078
findings
10581079
}
10591080

1081+
/// Whether `column` or its qualifier names an enclosing lambda's parameter no FROM item shadows.
1082+
fn lambda_parameter(
1083+
column: &Column,
1084+
(lambdas, mut frame): (&[(usize, Vec<String>)], usize),
1085+
level: RuleLevel<'_, '_>,
1086+
) -> bool {
1087+
let name: String = level.key(column.table.as_ref().unwrap_or(&column.name));
1088+
let field: String = level.key(&column.name);
1089+
if column.table.is_some()
1090+
&& level.scope.sources.iter().any(|source| {
1091+
source
1092+
.key
1093+
.as_ref()
1094+
.is_some_and(|key| level.context.profile.same_identifier(key, &name))
1095+
&& offers(source, &field, level)
1096+
})
1097+
{
1098+
return false;
1099+
}
1100+
while frame != 0 {
1101+
let (parent, names) = &lambdas[frame];
1102+
if names
1103+
.iter()
1104+
.any(|parameter| level.context.profile.same_identifier(parameter, &name))
1105+
{
1106+
return true;
1107+
}
1108+
frame = *parent;
1109+
}
1110+
false
1111+
}
1112+
10601113
/// Snowflake's DISTINCT with ORDER BY a column it does not select.
10611114
fn distinct_order(
10621115
(node, select): (&Expression, &Select),

0 commit comments

Comments
 (0)