Skip to content

Commit 72cd4e7

Browse files
zz_yclaude
andcommitted
fix(sql): re-qualify derived-table output so joins don't cross-product (#66)
A join over two derived tables (`FROM (SELECT …) a JOIN (SELECT …) b ON a.k = b.k`) silently degenerated to a cross product: the `SubqueryAlias` arm dropped the alias, so the derived output columns had no qualifier and both `a.k`/`b.k` fell back to the first bare `k` (col 0) — `k = k`, always true. Fix: the derived-table arm now re-qualifies the inner projection's output columns with the alias. A new optional `Project.qualifier` (L2 + L3) stamps the alias onto each output column during schema derivation; the common case (the SELECT list already lowered to a Projection) is retagged in place with no extra node, and the `SELECT *` / non-Projection case is wrapped in an identity re-qualifying projection built from the sub-plan's field names. Verified: `(SELECT k,v FROM m) a JOIN (SELECT k,v FROM n) b ON a.k=b.k` now lowers the predicate to `Column(0) = Column(2)` (distinct), not `Column(0) = Column(0)`. Two regression tests added (explicit-columns + `SELECT *`); existing derived-table + join tests unchanged. Full workspace suite green; clippy --all-targets clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 99635bc commit 72cd4e7

5 files changed

Lines changed: 107 additions & 11 deletions

File tree

‎crates/frontend-sql/src/sql/mod.rs‎

Lines changed: 43 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -114,13 +114,44 @@ impl<'a> SqlLowerer<'a> {
114114
// A *derived table* / inline view — `FROM (SELECT …) t`, the
115115
// SQL counterpart of PromQL function nesting (an aggregate
116116
// over an aggregate, a filter over a derived aggregate, …).
117-
// Lower the inner plan recursively; `lower_plan` already
118-
// handles every node a sub-`SELECT` can produce. The alias is
119-
// dropped — a qualified outer reference (`t.col`) resolves by
120-
// bare name against the derived output schema, the same
121-
// `Qualified → bare-name` fallback the converter's column
122-
// resolution already applies for joins (issue #27).
123-
other => self.lower_plan(other),
117+
// Lower the inner plan, then re-qualify its output columns
118+
// with the alias so `t.col` resolves to *this* relation — and,
119+
// critically, so a join over two derived tables disambiguates
120+
// its keys instead of both binding to the first bare-name
121+
// match (issue #66). The inner column *names* are unchanged;
122+
// only the qualifier is stamped.
123+
other => {
124+
let alias_name = alias.alias.to_string();
125+
match self.lower_plan(other)? {
126+
// The derived SELECT list already lowered to a
127+
// Projection — stamp the alias onto it, no extra node.
128+
L2::Project { cols, input, .. } => Ok(L2::Project {
129+
cols,
130+
qualifier: Some(alias_name),
131+
input,
132+
}),
133+
// Otherwise (e.g. `SELECT *` unwrapped to a scan) wrap
134+
// in an identity projection that re-qualifies each
135+
// output column. Names come from the sub-plan's schema.
136+
inner => {
137+
let cols = alias
138+
.input
139+
.schema()
140+
.fields()
141+
.iter()
142+
.map(|f| L2ProjectItem {
143+
alias: Some(f.name().clone()),
144+
expr: L2Expr::Column(ColumnRef::Named(f.name().clone())),
145+
})
146+
.collect();
147+
Ok(L2::Project {
148+
cols,
149+
qualifier: Some(alias_name),
150+
input: Box::new(inner),
151+
})
152+
}
153+
}
154+
}
124155
}
125156
}
126157
other => Err(LoweringError::UnsupportedFeature(format!(
@@ -300,7 +331,11 @@ impl<'a> SqlLowerer<'a> {
300331
_ => df_expr_to_l2(e).map(|expr| L2ProjectItem { expr, alias: None }),
301332
})
302333
.collect::<Result<Vec<_>, _>>()?;
303-
Ok(L2::Project { cols, input })
334+
Ok(L2::Project {
335+
cols,
336+
qualifier: None,
337+
input,
338+
})
304339
}
305340

306341
fn lower_aggregate(&self, agg: &logical_expr::Aggregate) -> Result<L2, LoweringError> {

‎crates/frontend-sql/tests/sql_lowering.rs‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,42 @@ async fn join_predicate_disambiguates_shared_column_name() {
327327
);
328328
}
329329

330+
#[tokio::test]
331+
async fn derived_table_join_disambiguates_via_alias() {
332+
// Issue #66: a join over two *derived tables* must bind its keys to distinct
333+
// positions. Before the fix the derived output columns lost their qualifier,
334+
// so `a.service` and `b.service` both fell back to the first bare `service`
335+
// (col 0) — `service = service`, always true → a silent cross product.
336+
// Concatenated: a[service,region] ++ b[service,region] → a.service=0, b.service=2.
337+
let qe = lower(
338+
"SELECT a.region, b.region \
339+
FROM (SELECT service, region FROM hosts) a \
340+
JOIN (SELECT service, region FROM hosts) b ON a.service = b.service",
341+
)
342+
.await;
343+
let join = find_join(&qe).expect("expected a Join in the tree");
344+
assert_eq!(
345+
join_eq_columns(join),
346+
[0, 2],
347+
"derived-table join keys must bind to distinct positions, not both to the first `service`"
348+
);
349+
}
350+
351+
#[tokio::test]
352+
async fn derived_table_select_star_join_disambiguates_via_alias() {
353+
// Same as above but `SELECT *` derived tables (the non-Projection path that
354+
// wraps the inner plan in an identity re-qualifying projection).
355+
let qe = lower(
356+
"SELECT a.region, b.region \
357+
FROM (SELECT * FROM hosts) a JOIN (SELECT * FROM hosts) b \
358+
ON a.service = b.service",
359+
)
360+
.await;
361+
let join = find_join(&qe).expect("expected a Join in the tree");
362+
let [l, r] = join_eq_columns(join);
363+
assert_ne!(l, r, "SELECT * derived-table join keys must not collapse to one column");
364+
}
365+
330366
#[tokio::test]
331367
async fn self_join_disambiguates_via_aliases() {
332368
// A self-join shares *every* column name; the alias qualifiers (`a`/`b`) are

‎crates/ir/src/intent_algebra/query_expr.rs‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,11 @@ pub enum QueryExpr {
272272
/// π — column projection.
273273
Project {
274274
cols: Vec<ProjectItem>,
275+
/// Re-qualifies every output column with this table alias (a derived
276+
/// table / inline view). `None` for an ordinary SELECT list. See
277+
/// [`relational::QueryExpr::Project`](super::relational).
278+
#[serde(default)]
279+
qualifier: Option<String>,
275280
child: Box<QueryExpr>,
276281
},
277282

@@ -515,7 +520,7 @@ impl QueryExpr {
515520
// is the explicit alias or a derived default. Projection may drop
516521
// the grouping/time columns, so unique_keys reset and time_index
517522
// is re-found by name.
518-
QueryExpr::Project { cols, child } => {
523+
QueryExpr::Project { cols, qualifier, child } => {
519524
let in_schema = child.output_schema_in(scope)?;
520525
let columns: Vec<Column> = cols
521526
.iter()
@@ -526,7 +531,14 @@ impl QueryExpr {
526531
.alias
527532
.clone()
528533
.unwrap_or_else(|| default_proj_name(&item.expr, i, &in_schema));
529-
Column::new(name, dtype, nullable)
534+
let c = Column::new(name, dtype, nullable);
535+
// A derived table re-qualifies its output columns with
536+
// its alias, so `t.col` (and a join over two derived
537+
// tables) resolves to the right relation.
538+
match qualifier {
539+
Some(q) => c.with_table(q),
540+
None => c,
541+
}
530542
})
531543
.collect();
532544
let time_index = columns.iter().position(|c| c.name == "ts");
@@ -787,6 +799,7 @@ mod tests {
787799
vec![vec![0, 1]],
788800
);
789801
let q = QueryExpr::Project {
802+
qualifier: None,
790803
cols: vec![
791804
// bare column passthrough keeps its (schema) name + type: host=col 1
792805
ProjectItem {
@@ -962,6 +975,7 @@ mod tests {
962975
vec![],
963976
);
964977
let q = QueryExpr::Project {
978+
qualifier: None,
965979
cols: vec![
966980
// value=col 1, ts=col 0
967981
ProjectItem {

‎crates/l2/src/lower.rs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,11 @@ pub fn convert(
228228

229229
// π — resolve each project item's expression to positional against the
230230
// child's schema.
231-
LQueryExpr::Project { cols, input } => {
231+
LQueryExpr::Project {
232+
cols,
233+
qualifier,
234+
input,
235+
} => {
232236
let child = convert(input, fallback, acc)?;
233237
let child_schema = child.output_schema()?;
234238
let cols = cols
@@ -242,6 +246,7 @@ pub fn convert(
242246
.collect::<Result<Vec<_>, _>>()?;
243247
CQueryExpr::Project {
244248
cols,
249+
qualifier: qualifier.clone(),
245250
child: Box::new(child),
246251
}
247252
}

‎crates/l2/src/relational.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,8 +143,14 @@ pub enum QueryExpr {
143143

144144
/// π — projection / SELECT list (SQL). Column refs in `cols` resolve by
145145
/// name against the child schema during conversion.
146+
///
147+
/// `qualifier` re-qualifies every output column with a table alias — set for
148+
/// a derived table / inline view (`FROM (SELECT …) t`), so an outer `t.col`
149+
/// reference resolves to *this* relation and a join over two derived tables
150+
/// disambiguates its keys. `None` for an ordinary SELECT list.
146151
Project {
147152
cols: Vec<L2ProjectItem>,
153+
qualifier: Option<String>,
148154
input: Box<QueryExpr>,
149155
},
150156

0 commit comments

Comments
 (0)