amitvijapur commented on PR #24409: URL: https://github.com/apache/datafusion/pull/24409#issuecomment-5468324015
Yes, LLM-assisted, and I should have said so upfront rather than when asked. I use Claude Code for implementation and review its output before pushing. That review wasn't good enough here. You caught a doc paragraph pointing at `SparkPmod::coerce_types`, which this same PR deletes; a comment claiming the computation type is wide enough by construction while also justifying overflow handling, which can't both be true; a pointer to an ANSI assertion "further down" that is actually above it; and three test cases I'd added twice in two places. Those are the unnecessary comments you're describing and none should have survived. All eleven points are applied in d336e0e2, and the diff is 43 lines smaller for it. `decimal_coercion` inlined as you suggested, since only a decimal pair can reach that branch now. On the understanding bar in the contributor guide: what I can justify is the null and coercion interaction. When you suggested `one_of` I built it, found it regressed `pmod(NULL, NULL)` and `pmod(2.5::decimal(3,1), NULL)`, and posted the failing queries rather than just accepting or rejecting the suggestion. The null result types I checked against `mod` directly rather than reasoning about them. What I'd flag as not fully mine: I can't tell you why `Numeric(2)` refuses `(Null, Int32)`. I established that `mod` behaves identically and that it predates this PR, but not the underlying cause. Happy to leave that for the followup you mentioned. I kept the `statement error` case for `pmod(NULL, 3::int)` for now since it covers real current behaviour, but happy to drop it into the followup instead if you'd rather it not sit in this PR. Going forward I'll keep comments to what I'd write myself. -- 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]
