andygrove commented on PR #5169:
URL:
https://github.com/apache/datafusion-comet/pull/5169#issuecomment-5441561413
> **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.
Turning these into typed `SparkError` variants is clearly the right
direction, and the tests that assert on error class, SQLSTATE, and query
context rather than just "it threw" are the right level of assertion for this
kind of fix.
Four things.
**The #5072 bullet is still in the compatibility docs**
The PR closes #5072, but `compatibility/index.md` keeps the bullet saying
wide-decimal overflow, decimal divide-by-zero, and decimal-to-decimal cast
overflow "raise raw Arrow errors that bypass `SparkErrorConverter`". That is
exactly what this PR fixes, so the bullet should be deleted rather than left
describing behavior that no longer exists. Otherwise users read a docs page
that is wrong the moment this merges.
**Two unrelated divergences added to the same docs file**
The #5211 and #5073 bullets are new known-divergence entries for things this
PR does not touch. Documenting them is good, but landing them here makes the
diff harder to reason about and ties their wording to this PR's review. Would
you split them into a docs-only PR?
**`is_wide_decimal` peels exactly one wrapper**
```rust
let is_wide_decimal = child.downcast_ref::<WideDecimalBinaryExpr>().is_some()
|| child.downcast_ref::<CheckedBinaryExpr>()
.is_some_and(|checked|
checked.child().downcast_ref::<WideDecimalBinaryExpr>().is_some());
```
This hardcodes the assumption that at most one `CheckedBinaryExpr` sits
between `CheckOverflow` and the wide-decimal expression. If a third wrapper
ever appears, the match silently fails and the redundant overflow check comes
back, which is a quiet performance regression with no test that would notice.
Would a small helper, something like `fn unwrap_checked(expr: &Arc<dyn
PhysicalExpr>) -> &Arc<dyn PhysicalExpr>` that loops until it finds a
non-`CheckedBinaryExpr`, be more durable? Then the condition is a single
downcast on the unwrapped expression.
**The `hasExprId` change is broader than the PR title suggests**
```scala
if (!protoExpr.hasExprId) {
builder.setExprId(nextExprId())
extractQueryContext(expr).foreach { ctx => builder.setQueryContext(ctx) }
}
```
This changes context assignment for every expression in the tree, not just
decimal ones. The reasoning about passthrough serdes like `Alias` is sound, but
the same code path now also skips assigning a context to any expression whose
serde happens to return an already-identified child, including ones that can
themselves throw. `Cast(Divide(a, b))` is the case I would want pinned: the
divide sets an id, so the cast no longer gets its own context, and a cast
overflow would then report the divide's context.
The description says outer-`Cast` context is preserved, so presumably
something handles this. Could you point at the test? And is there anywhere a
serde returns the *same* child proto for two different parent expressions,
which would now share one exprId?
--
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]
