yashmayya commented on PR #18444: URL: https://github.com/apache/pinot/pull/18444#issuecomment-4433899761
Re-reviewed and just two small follow-ups that elaborate on my earlier unresolved comments: **1. Null input case** (elaborates on [this comment](https://github.com/apache/pinot/pull/18444#discussion_r3210612800)) — In `MathFuncs.json` `unary_minus`, add a column with a null row and verify `-nullCol` propagates the null (with null handling enabled). This is the most common path users will actually hit and isn't covered by the current numeric-only inputs. **2. `INT_MIN` / `LONG_MIN` overflow** (elaborates on [this comment](https://github.com/apache/pinot/pull/18444#discussion_r3210610933)) — `intNegate`/`longNegate` use `Math.negateExact`, which throws `ArithmeticException` on `Integer.MIN_VALUE` / `Long.MIN_VALUE`. Worth a row in the test data with those values to either pin down the failure mode or document the behavior; this is the first time `-col` syntax actually reaches `NegateScalarFunction`. Also — your [SqlSyntax analysis](https://github.com/apache/pinot/pull/18444#discussion_r3224804256) is correct, `addAll` is the right call given Calcite's downstream filtering in `SqlUtil.lookupSubjectRoutinesByName` and the fact that `PINOT_PLUS`/`PINOT_MINUS` are registered as `SqlBinaryOperator` under `add`/`sub` function aliases. Resolving that thread. -- 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]
