alamb commented on code in PR #24429:
URL: https://github.com/apache/datafusion/pull/24429#discussion_r3825243409
##########
datafusion/core/tests/sql/sql_api.rs:
##########
@@ -212,72 +215,306 @@ async fn merge_into_context() -> SessionContext {
let ctx = SessionContext::new();
ctx.sql("CREATE TABLE target (id INT)").await.unwrap();
ctx.sql("CREATE TABLE source (id INT)").await.unwrap();
+ ctx.sql("CREATE TABLE \"Target\" (id INT)").await.unwrap();
+ ctx.sql("CREATE SCHEMA \"CaseSchema\"").await.unwrap();
+ ctx.sql("CREATE TABLE \"CaseSchema\".\"Target\" (id INT)")
+ .await
+ .unwrap();
ctx
}
-async fn assert_merge_sql_error(ctx: &SessionContext, sql: &str, expected:
&str) {
- let err = ctx.sql(sql).await.unwrap_err();
- assert_contains!(err.strip_backtrace(), expected);
-}
-
async fn assert_merge_physical_error(ctx: &SessionContext, sql: &str,
expected: &str) {
- let err = ctx
+ let result = ctx
.sql(sql)
.await
- .unwrap()
+ .unwrap_or_else(|error| panic!("failed to plan MERGE
SQL:\n{sql}\n{error}"))
.create_physical_plan()
- .await
- .unwrap_err();
- assert_contains!(err.strip_backtrace(), expected);
+ .await;
+ let err = match result {
+ Ok(_) => panic!("expected physical planning to fail:\n{sql}"),
+ Err(error) => error,
+ };
+ let actual = err.strip_backtrace();
+ assert!(
+ actual.contains(expected),
+ "MERGE SQL:\n{sql}\n\nExpected:\n{expected}\n\nActual:\n{actual}"
+ );
+}
+
+async fn merge_operation(ctx: &SessionContext, sql: &str) -> Box<MergeIntoOp> {
+ let plan = ctx.state().create_logical_plan(sql).await.unwrap();
+ let LogicalPlan::Dml(dml) = plan else {
+ panic!("expected MERGE DML")
+ };
+ let WriteOp::MergeInto(merge_op) = dml.op else {
+ panic!("expected MERGE operation")
+ };
+ merge_op
+}
+
+fn has_outer_reference_to(expr: &Expr, qualifier: &TableReference) -> bool {
+ let mut found = false;
+ expr.apply(|expr| {
+ let outer_refs = match expr {
+ Expr::Exists(exists) => Some(&exists.subquery.outer_ref_columns),
+ Expr::InSubquery(in_subquery) => {
+ Some(&in_subquery.subquery.outer_ref_columns)
+ }
+ Expr::SetComparison(set_comparison) => {
+ Some(&set_comparison.subquery.outer_ref_columns)
+ }
+ Expr::ScalarSubquery(subquery) =>
Some(&subquery.outer_ref_columns),
+ _ => None,
+ };
+ found = outer_refs.is_some_and(|outer_refs| {
+ outer_refs.iter().any(|expr| {
+ matches!(
+ expr,
+ Expr::OuterReferenceColumn(_, column)
+ if column.relation.as_ref() == Some(qualifier)
+ )
+ })
+ });
+ Ok(if found {
+ TreeNodeRecursion::Stop
+ } else {
+ TreeNodeRecursion::Continue
+ })
+ })
+ .unwrap();
+ found
}
#[tokio::test]
-async fn merge_into_rejects_source_alias_colliding_with_target_name() {
- // Canonicalizing `t.id` to `target.id` must not collapse it onto a source
- // that also uses `target` as its qualifier.
+async fn merge_into_distinguishes_target_alias_from_source_qualifier() {
let ctx = merge_into_context().await;
- for target_ref in ["target", "public.target", "datafusion.public.target"] {
- assert_merge_sql_error(
+ for target_ref in [
+ "target",
+ "public.target",
+ "datafusion.public.target",
+ "\"Target\"",
+ "\"CaseSchema\".\"Target\"",
+ ] {
+ assert_merge_physical_error(
&ctx,
&format!(
"MERGE INTO {target_ref} AS t USING source AS target \
ON t.id = target.id WHEN MATCHED THEN DELETE"
),
+ "MERGE INTO not supported for Base table",
+ )
+ .await;
+ }
+
+ for (target_alias, target_column) in [
+ ("T", "t.id"),
+ ("\"T\"", "\"T\".id"),
+ ("\"public.target\"", "\"public.target\".id"),
+ ] {
+ assert_merge_physical_error(
+ &ctx,
&format!(
- "MERGE source may not use the target table name '{target_ref}'
\
- as a qualifier"
+ "MERGE INTO target AS {target_alias} USING source AS target \
+ ON {target_column} = target.id WHEN MATCHED THEN DELETE"
),
+ "MERGE INTO not supported for Base table",
+ )
+ .await;
+ }
+
+ for source in [
Review Comment:
Can we please port these tests to use slt -- those are faster to compile
(they don't need to compile) and are easier to maintain over the long run
--
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]
