peterxcli commented on PR #5169: URL: https://github.com/apache/datafusion-comet/pull/5169#issuecomment-5444101274
Thanks for the review — worked through all four. **#5072 bullet**: removed. That was your Aug 6 ask that I only half-landed — I added the narrowed #5211 bullet but forgot to delete the old one. **Splitting the docs bullets**: the #5073 bullet was actually a bad merge resolution on my side, not an intentional addition — #5167 already removed it from main and the issue is closed, so I've deleted it here rather than splitting it out. The #5211 bullet I'd like to keep in this PR: it's the bullet you asked for on Aug 6, and it documents the residual divergence of this PR's own change, so its wording is naturally tied to this review. **`is_wide_decimal`**: done — added an `unwrap_checked` helper that loops through `CheckedBinaryExpr` layers, and the condition is now a single downcast. For the record, nesting can't occur today (every wrap site either wraps a freshly built expression or guards with `is_none()`), so this is future-proofing rather than a behavior change. **`hasExprId` and `Cast(Divide(a, b))`**: that case is safe by construction — the exprId lands on the divide's proto, while the Cast serde builds a fresh outer proto that never carries an id, so the `!hasExprId` branch always fires for the cast and it gets its own id and context. The only serdes that return an already-identified child proto are runtime no-op wrappers (`Alias`, `PromotePrecision`, `KnownFloatingPointNormalized`, `KnownNullable`/`KnownNotNull`), none of which can throw, so inheriting the child's context there is the intended semantics. And no serde returns the same child proto for two different parents — every conversion builds fresh messages, so sharing only happens vertically through those passthrough chains, never across siblings. That said, it deserved a pin: I've added "ANSI cast overflow reports the cast's own context", which runs `CAST(a / b AS DECIMAL(3, 2))` in ANSI mode — the divide child carries its own exprId and context (`a / b`), the cast itself overflows, and the test asserts error class, SQLSTATE, and context fragments all match Spark exactly. The existing `a div b` divide-by-zero test pins the complementary case where the returned proto is the synthesized outer Cast and inherits the `IntegralDivide`'s context. -- 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]
