fix: preserve MERGE target qualifier bindings - #24429
Conversation
github-actions
Bot
added
documentation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24429 +/- ##
==========================================
- Coverage 81.34% 81.34% -0.01%
==========================================
Files 1117 1117
Lines 397528 397525 -3
Branches 397528 397525 -3
==========================================
- Hits 323385 323376 -9
- Misses 55225 55229 +4
- Partials 18918 18920 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
removed
the
documentation
|
@alamb @kosiew @timsaucer Could you take a look of this PR before the Datafusion 55.0.0 release. I think adding the target table's qualifer into the protobuf is a more elegant solution to make code succinct and recursively rewriting subquery. |
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. This looks good to me.
I like the approach of keeping MERGE's SQL-visible target qualifier separate from the target provider identity. Using that qualifier consistently when rebuilding the MERGE expression schema across the analyzer, optimizer, and physical planner avoids the alias canonicalization issues while preserving the correct SQL scoping semantics.
The protobuf handling also looks good. Persisting the qualifier while falling back to DmlNode.table_name for older payloads keeps the change backward compatible.
The added coverage for aliased targets, source-name collisions, correlated subqueries, qualifier shadowing, and the basic protobuf round trip gives me good confidence in the change.
One note on protobuf coverage: I think keeping the MERGE round-trip test's ON expression flat is appropriate here. A target-correlated subquery cannot currently be serialized because datafusion/proto/src/logical_plan/to_proto.rs does not support OuterReferenceColumn, Exists, or InSubquery. A MERGE round-trip test for that case would fit better with future work adding broader protobuf support for correlated subquery expressions.
Thanks again!
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks for this @wirybeaver and @kosiew
The code looks good -- I would just like to request we migrate some of these tests to use slt rather than more rust code
Keep SQL-visible target qualifiers distinct from provider identity so correlated subqueries and source-name collisions retain their intended meaning.
Remove worktree-only context and ADR files from the published change while retaining them through local excludes.
MergeIntoOp has not appeared in a release, so users do not need before-and-after upgrade guidance.
Keep only binding-specific Rust assertions while moving SQL behavior cases to the faster, more maintainable sqllogictest suite.
|
@alamb The testing code has moved to SLT. Thanks for the guide |
d359e29 to
3d19096
Compare
|
@wirybeaver |
| /// expressions. The target's catalog/provider identity remains in | ||
| /// [`DmlStatement::table_name`]. | ||
| #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)] | ||
| #[non_exhaustive] |
There was a problem hiding this comment.
Could we make sure the 55.1 migration or release note includes the upgrade path for this API change? Downstream callers using MergeIntoOp { on, clauses } will now need to switch to MergeIntoOp::new(target_qualifier, on, clauses).
cargo-semver-checks correctly reports this as a source break. Since #24462 accepts this PR as planned 55.1 content. It would be useful for the note to explain why the new argument exists: the SQL-visible target qualifier is now intentionally separate from the target provider identity.
| /// SQL-visible target qualifier. Absent in payloads written before this field | ||
| /// was introduced; readers then fall back to DmlNode.table_name. | ||
| #[prost(message, optional, tag = "3")] | ||
| pub target_qualifier: ::core::option::Option<TableReference>, |
There was a problem hiding this comment.
It would be good to cover this generated-type source break in the 55.1 migration material too. Downstream callers constructing MergeIntoOpNode with a struct literal now need to provide target_qualifier, normally as Some(...).
I would keep the existing wire-compatibility explanation alongside that note. New readers can still accept old payloads through the DmlNode.table_name fallback, while old readers cannot safely preserve the alias semantics carried by new payloads.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn merge_into_requires_boolean_conditions() { |
There was a problem hiding this comment.
Could we move this SQL-only boolean-condition case into merge_into.slt as well? The non-boolean ON and WHEN cases are already covered there, and this test does not inspect bindings, protobuf state, or physical column indices.
That would also let us remove assert_merge_physical_error. I see this as test maintenance rather than a correctness blocker.
4 participants
Which issue does this PR close?
Rationale for this change
#22988 deliberately rejected two valid MERGE forms to avoid silently changing expression meaning.
Limitation 1: target-correlated subquery with an aliased target
t.idinside the subquery becomesouter_ref(t.id). The old top-level canonicalizer only rewroteExpr::Column(t.id)totarget.id, leaving the outer reference inconsistent with the schema later rebuilt fromDmlStatement.table_name.Limitation 2: source qualifier equals the target table name
Canonicalizing target
t.idtotarget.idcollapses both operands onto the source qualifier and can turn the condition intotarget.id = target.id.A recursive string rewrite is not safe: qualifiers are scope-local, so an inner relation can legally shadow
t. It also cannot solve the second limitation because both relations would still have the same qualifier after rewriting.What changes are included in this PR?
Solution
Keep provider identity and the SQL-visible target qualifier as separate plan state:
Planning now proceeds as follows:
targetto the target provider, while retaining aliastas the visible qualifier.tonMergeIntoOp; do not canonicalize target columns or recursively rewrite subquery plans.t.*, then source fields) and use it consistently in SQL planning, analyzer/optimizer rules, physical planning, and programmatic plans.TableProvider::merge_into, where target and source columns resolve to distinct physical indices.Protobuf change and 55.0/55.1 compatibility
MergeIntoOpNodegains optionaltarget_qualifierfield 3. This field is necessary becauseDmlNode.table_namecontains provider identity and cannot also represent aliast; without it, a proto round trip loses the binding needed to resolve MERGE expressions.Compatibility is directional:
DmlNode.table_name, matching 55.0's canonicalized representation.Rust API compatibility
DataFusion 55.0 released
MergeIntoOpwith public struct-literal construction:MergeIntoOp { on, clauses }. This PR makes the struct non-exhaustive, adds private target-qualifier state, and requiresMergeIntoOp::new(target_qualifier, on, clauses). Therefore existing 55.0 downstream struct literals will not compile unchanged against 55.1. This compatibility exception should be considered explicitly for the 55.1 release.The PR also:
Are these changes tested?
cargo fmt --allcargo clippy --all-targets --all-features -- -D warnings./ci/scripts/doc_prettier_check.sh --write --allow-dirtyRUST_BACKTRACE=1 cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-cli --workspace --lib --tests --bins --features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryptionAre there any user-facing changes?
Yes. Both valid MERGE forms above now plan successfully and reach
TableProvider::merge_into. The Rust and protobuf compatibility constraints for upgrading from 55.0 to 55.1 are documented above.