andygrove commented on PR #4653:
URL:
https://github.com/apache/datafusion-comet/pull/4653#issuecomment-5441745954
> **Note on this review:** this was generated by an LLM (Claude Code) at my
request while I worked through a review backlog. I have not verified the
individual findings myself. Please treat everything below as suggestions to
evaluate rather than as authoritative review feedback, and push back on
anything that is wrong or already handled.
The diagnosis is right and the fix is the right shape.
`DataFusionError::Context` appearing in the chain since DataFusion 53 is
exactly the kind of thing that silently degrades error fidelity, and recursing
through both `Context` and nested `External` covers it. Removing lines from all
four `dev/diffs` files, presumably re-enabling Spark tests that were excluded
because of this, is a good sign the fix is real.
Three things.
**Two copies of the same walker**
`extract_spark_error` in
`native/core/src/execution/expressions/arithmetic.rs` and
`extract_spark_payload` in `native/jni-bridge/src/errors.rs` implement the same
recursion over the same error shapes, and the former's doc comment says so
explicitly ("Mirrors `extract_spark_payload`"). Two hand-maintained copies of
an unwrapping walker will drift, and when they do the symptom is the exact bug
this PR fixes.
Could the traversal live in one place, with the JNI-bridge version layering
the `JavaException` / `WithContext` / `Bare` classification on top of a shared
"walk to the innermost non-wrapper error" helper? Even a shared `fn
innermost(err: &DataFusionError) -> &(dyn Error)` would remove the duplication.
**Are all the wrapper variants covered?**
The match handles `External` and `Context`. DataFusion also has
`Shared(Arc<DataFusionError>)` and, depending on version, `Diagnostic`. If
either can appear in a chain that carries a `SparkError`, the leak comes
straight back. Have you checked which variants DataFusion can produce around an
external error in the pinned version? Adding the missing arms now is cheaper
than rediscovering this after the next upgrade.
**One error class is tested**
`CometCodegenSuite` gains 36 lines covering divide-by-zero. The fix is
generic across every Spark error type, so it would be worth covering at least
one more, for example an `ARITHMETIC_OVERFLOW` or a cast failure, through the
same dispatched path. A single-error test passes even if the classification in
`extract_spark_payload` gets one of the three branches wrong.
**One process question**
The `dev/diffs/*.diff` files change. Were those regenerated by applying the
diff to a Spark checkout, editing the source, and re-diffing, or edited by
hand? The project convention is the former, and hand edits tend to produce
diffs that fail to apply cleanly later.
--
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]
