zclllyybb commented on issue #64681: URL: https://github.com/apache/doris/issues/64681#issuecomment-4766243471
Breakwater-GitHub-Analysis-Slot: slot_88614b96b47d This content is generated by AI for reference only. Initial analysis: This looks like a valid FE/Nereids constant-folding bug on current master. I checked the upstream master code path at commit `90db340515e88e177bc4e6151ba0b9a84cf04d8c`: - `NumericArithmetic.divideDecimal(DecimalLiteral first, DecimalLiteral second)` currently returns `NullLiteral` when `first` is zero. That checks the numerator, unlike `divideDouble` and `divideDecimalV3`, which check the denominator. - `ExpressionEvaluator` registers `NumericArithmetic` executable functions and selects overloads by Java parameter assignability, so `divide` with two `DecimalLiteral` children can enter this DECIMALV2 overload during folding. - If the denominator is zero and the numerator is nonzero, `BigDecimal.divide(0)` raises through reflection; `ExpressionEvaluator.invoke` catches non-`NotSupportedException` invocation failures and returns the original expression. That explains why this path is unlikely to crash FE, but the folding implementation is still inconsistent and has a wrong zero-numerator result. Expected impact: - A silent wrong folded result for DECIMALV2 constant folding when both operands are constants and the numerator is zero, for example `0 / nonzero_decimal_v2` folding to `NULL` instead of decimal zero. - I did not see the same bug in the adjacent double or DECIMALV3 folding paths, because those paths already guard on `second`. PR #64675 appears directionally correct: changing the guard to `second.getValue().compareTo(BigDecimal.ZERO) == 0` aligns DECIMALV2 with the sibling implementations and preserves `NULL` for divide by zero. The added direct unit tests for `0 / 5` and `5 / 0` cover the two edge cases in the executable function. Missing information / suggested next steps: 1. No logs or profile are needed for the initial triage; the reported behavior is directly supported by the current master code. 2. The issue does not include a concrete SQL snippet or `EXPLAIN`/folded-plan output. The code evidence is enough to validate the bug, but adding a minimal SQL-level repro would make user-facing verification easier. 3. Review and run the PR's FE unit tests/CI. Consider adding a Nereids regression or planner-level assertion if maintainers want coverage of the full fold-constant dispatch path, not only the executable helper method. 4. If older release branches still carry the same DECIMALV2 implementation, check the exact branch code before deciding whether this needs backporting. -- 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]
