jayzhan211 commented on code in PR #27244:
URL: https://github.com/apache/datafusion/pull/27244#discussion_r4550838763
##########
datafusion/optimizer/src/decorrelate.rs:
##########
@@ -1232,12 +1232,29 @@ pub(crate) fn find_join_exprs(exprs: Vec<&Expr>) ->
(Vec<Expr>, Vec<Expr>) {
pub(crate) fn build_join_filter<'a>(
join_filters: Vec<Expr>,
correlated_cols: impl IntoIterator<Item = &'a Column>,
+ subquery_schema: &DFSchema,
subquery_alias: &str,
) -> Result<Option<Expr>> {
let correlated_cols = correlated_cols
.into_iter()
.cloned()
.collect::<BTreeSet<_>>();
+
+ for col in &correlated_cols {
+ let duplicate_count = subquery_schema
+ .fields()
+ .iter()
+ .filter(|f| f.name() == &col.name)
+ .count();
+ if duplicate_count > 1 {
+ return plan_err!(
+ "Unsupported correlated subquery: inner relation column '{}'
has ambiguous duplicate references across relations under alias '{}'",
+ col.name,
+ subquery_alias
+ );
+ }
+ }
Review Comment:
This check also rejects queries that return correct rows today. Both queries
below return `1, 2` on 55.2.0 and on `main`, and now fail with this error:
```sql
SELECT t_outer.k FROM t26234 t_outer WHERE EXISTS (
SELECT * FROM t26234 one, t26234 two WHERE one.k = t_outer.k
);
SELECT t_outer.k FROM t26234 t_outer WHERE EXISTS (
SELECT one.k, two.k FROM t26234 one, t26234 two WHERE one.k = t_outer.k
);
```
The mapping is wrong only when the correlated column is not the first field
with its name: `SubqueryAlias` keeps the first `k` and renames the later ones
to `k:1`, so `__correlated_sq_1.k` is always the first. Could the check test
exactly that? The new message also names the column (`two.k`) instead of the
internal alias.
```suggestion
for col in &correlated_cols {
check_alias_keeps_column_name(subquery_schema, col)?;
}
```
```rs
/// Returns an error if `col` is not the first field of `schema` with its
name.
///
/// Wrapping `schema` in a `SubqueryAlias` keeps the first field with a given
/// name and renames later ones (`k:1`, ...), so qualifying `col` as
/// `<alias>.<name>` refers to `col` only when it is that first field.
/// See <https://github.com/apache/datafusion/issues/26234>.
pub(crate) fn check_alias_keeps_column_name(
schema: &DFSchema,
col: &Column,
) -> Result<()> {
let Some(index) = schema.maybe_index_of_column(col) else {
return Ok(());
};
let first = schema.fields().iter().position(|f| f.name() == &col.name);
if first != Some(index) {
return plan_err!(
"Unsupported correlated subquery: correlated column {col} has
the same name as an earlier column of the subquery output"
);
}
Ok(())
}
```
<details>
<summary>Tests to add: the two queries above as passing tests, the five
existing error tests with the new message, and a unit test where `one.a` passes
and `two.a` errors</summary>
In `subquery.slt`, the five `statement error` lines become:
```
statement error Unsupported correlated subquery: correlated column two.k has
the same name as an earlier column of the subquery output
```
and these go before `# Single relation correlated column works without
ambiguity`:
```
# Same-named columns are fine when only the first one is correlated
query I
SELECT t_outer.k FROM t26234 t_outer WHERE EXISTS (
SELECT * FROM t26234 one, t26234 two WHERE one.k = t_outer.k
) ORDER BY t_outer.k;
----
1
2
query I
SELECT t_outer.k FROM t26234 t_outer WHERE EXISTS (
SELECT one.k, two.k FROM t26234 one, t26234 two WHERE one.k = t_outer.k
) ORDER BY t_outer.k;
----
1
2
```
In `test_build_join_filter`, replacing the `dup_schema` part:
```rs
// Under the alias, `one.a` keeps the name `a` and `two.a` becomes `a:1`,
// so only `one.a` can be qualified as `sq.a`.
let field_a = Arc::new(arrow::datatypes::Field::new("a", DataType::Int32,
true));
let dup_schema = DFSchema::new_with_metadata(
vec![
(Some("one".into()), Arc::clone(&field_a)),
(Some("two".into()), field_a),
],
std::collections::HashMap::new(),
)?;
let one_a = Column::from("one.a");
let two_a = Column::from("two.a");
assert!(build_join_filter(vec![], [&one_a], &dup_schema, "sq").is_ok());
assert!(build_join_filter(vec![], [&two_a], &dup_schema, "sq").is_err());
```
With this applied, the full sqllogictest suite and `cargo test -p
datafusion-optimizer --lib` pass, and `cargo clippy -p datafusion-optimizer
--all-targets -- -D warnings` is clean.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
