unikdahal commented on PR #5318:
URL:
https://github.com/apache/datafusion-comet/pull/5318#issuecomment-5574146485
Thanks @andygrove for the detailed review. I’ve pushed another update
addressing the feedback and also updated the PR description with the latest
benchmark results, compatibility boundaries, and test coverage.
A few of the review points led to more substantial changes than I initially
expected, so here is a summary of what changed and the reasoning behind the
remaining decisions.
* **Spark 4.1+ native MergeRows is now intentionally disabled.**
While looking into the missing per-action metrics, I found that this is
more than a metrics-parity issue. Starting in Spark 4.1, the concrete
`MergeRowsExec` contributes the action counters used to construct
`MergeSummary`, which is then passed through the summary-aware V2
`BatchWrite.commit` path. Replacing `MergeRowsExec` without preserving that
state could therefore alter the writer commit contract even if the produced
rows are correct.
For this PR, native `MergeRowsExec` is consequently limited to **Spark
3.5.x and 4.0.x**, while 4.1+ explicitly falls back to Spark's implementation.
There is test coverage for that version boundary. Full 4.1+ support should
preserve the metrics, action context, `MergeSummary`, and writer contract
end-to-end rather than just exposing additional counters, so I’m keeping that
out of this already-large change.
* **`output_types` are now authoritative for the native output schema.**
Previously the native side could derive a structurally compatible schema
independently. That is risky for nested types/nullability because Spark's
serialized schema is the actual execution contract. The updated implementation
validates the independently derived projection schema against Spark's
`output_types` and uses the Spark declaration for the emitted schema. This
keeps the native operator aligned with Spark rather than relying on two
independently inferred schemas remaining equivalent.
* **The SQL-level coverage has been expanded substantially.**
In addition to the lower-level native tests, the updated tests exercise
MATCHED / NOT MATCHED / NOT MATCHED BY SOURCE behavior, matched
DELETE/copy-on-write paths, conditional first-match-wins behavior, explicit
native-disabled fallback, and the relevant delta/split update paths. I wanted
these at the SQL level because several of the earlier bugs were not failures of
an individual Rust expression—they were failures in the contract between
Spark's MERGE semantics, planning, and native execution.
* **Added real Iceberg MERGE coverage with native MergeRows enabled.**
The test verifies both the resulting table contents and that execution
actually engages `CometMergeRowsExec` rather than accidentally passing because
Spark fell back to the JVM path. This gives us coverage of the integration
boundary that unit-level MergeRows tests cannot provide.
* **The physical ordering difference is now documented explicitly.**
Spark's implementation happens to preserve input ordering in places where
the native vectorized implementation does not guarantee it. MERGE semantics do
not define that physical row order as part of the result contract, so I don’t
think forcing the native operator to reproduce incidental ordering is
desirable. It is, however, worth making the difference explicit so it is not
mistaken for an unnoticed behavioral difference.
* **The shim was moved under the existing `org.apache.comet.shims` layout**,
and I reduced some of the implementation comments while keeping the ones that
explain non-obvious Spark compatibility/cardinality invariants.
On the **duplicated Spark 3.5/4.0 serde sources**, I looked more closely at
whether these should be moved into a shared source root, but I decided to keep
the version-local copies intentionally.
The relevant compatibility window is specifically **3.5 + 4.0**: Spark 3.4
does not have the corresponding `MergeRowsExec`, while Spark 4.1+ now
deliberately follows a different path because of the `MergeSummary` contract
described above. Comet's compatibility code is already organized around
Spark-version-specific source roots, and there isn't currently a shared
source-set abstraction representing exactly "3.5 and 4.0 but not 3.4 or 4.1+".
Introducing a new build/source-layout convention solely to deduplicate these
small adapters would add another compatibility abstraction without eliminating
the underlying version boundary. I think keeping them version-local is simpler
and more consistent with the surrounding Comet structure.
The real concern with duplicated compatibility sources is **silent drift**,
so I addressed that directly: there is now a parity check covering the 3.5/4.0
MergeRows serde/shim implementations. As long as they are intended to be
equivalent, an accidental change to only one side will fail the test. If Spark
eventually requires the implementations to diverge, that difference will have
to be made explicit rather than happening unnoticed.
I’ve also added the benchmark results to the PR description rather than
duplicating them here.
The two larger pieces I’m intentionally keeping outside this PR are:
* **native Spark 4.1+ MergeRows**, because supporting it correctly requires
preserving the complete `MergeSummary` / V2 writer contract rather than only
adding metrics; and
* **Spark 4.2 `InsertOnlyMergeExec`**, which is a separate execution
operator and should be treated independently under the broader MERGE work.
At this point I think expanding #5318 further would make the change harder
to review without materially improving the 3.5/4.0 implementation being added
here. The current scope is therefore: make native `MergeRowsExec` correct and
well-covered where we can preserve Spark's contract, and explicitly fall back
where we currently cannot.
Thanks again for the review several of these comments helped uncover
compatibility contracts that go beyond simply producing the same output rows.
--
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]
