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.45% 81.44% -0.01%
==========================================
Files 1120 1120
Lines 401793 401790 -3
Branches 401793 401790 -3
==========================================
- Hits 327262 327250 -12
- Misses 55356 55364 +8
- Partials 19175 19176 +1 ☔ 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 |
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.
There was a problem hiding this comment.
Done in a2377f0. I moved the remaining NULL boolean-condition case to merge_into.slt and removed the Rust test and assert_merge_physical_error helper.
Explain the Rust and generated protobuf literal changes, and move the final SQL-only boolean case into the SLT suite.
github-actions
Bot
added
the
documentation
|
It looks like this has an API change. Per our documentation https://datafusion.apache.org/contributor-guide/api-health.html and https://datafusion.apache.org/contributor-guide/release_management.html#eligible-for-backport Specifically the changes in Do others concur? |
Agreed. |
Agreed! |
|
I also think 56.0.0 would be a better version to release API change. Does that mean the PR should get merged after 55.1.0 release? |
No, we will make the 55.1.0 release from the |
| <!--- | ||
| Licensed to the Apache Software Foundation (ASF) under one | ||
| or more contributor license agreements. See the NOTICE file | ||
| distributed with this work for additional information | ||
| regarding copyright ownership. The ASF licenses this file | ||
| to you under the Apache License, Version 2.0 (the | ||
| "License"); you may not use this file except in compliance | ||
| with the License. You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, | ||
| software distributed under the License is distributed on an | ||
| "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| KIND, either express or implied. See the License for the | ||
| specific language governing permissions and limitations | ||
| under the License. | ||
| --> |
There was a problem hiding this comment.
Since this will go into 56.0.0 the content of this file needs to be moved over to the correct file.
6 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.