jineshparakh opened a new pull request, #18444: URL: https://github.com/apache/pinot/pull/18444
## Description Queries like `SELECT -price FROM table` or `SELECT +col FROM table` previously failed at execution time in both the V1 (single stage) and V2 (multi stage) engines. This was caused by two separate bugs working together. ### Bug 1: Operator table collision `PinotOperatorTable` stored operators in a flat `Map<String, SqlOperator>`, meaning only one operator could exist per canonical name. Binary minus (`PINOT_MINUS`) and unary minus (`UNARY_MINUS`) both canonicalize to `"-"`, so only the binary variant survived registration. When Calcite tried to validate `-col`, it could not find a prefix operator for `"-"`, fell into `handleUnresolvedFunction`, retrieved `PINOT_MINUS` (a `SqlMonotonicBinaryOperator`), and threw a `ClassCastException` trying to cast it to `SqlFunction`. **Fix:** Changed the backing store from `Map<String, SqlOperator>` to `Map<String, List<SqlOperator>>`: The new `register()` method allows multiple operators with the same canonical name as long as they have different `SqlSyntax` values (PREFIX vs BINARY, for example). `lookupOperatorOverloads()` now returns all matching operators and lets Calcite disambiguate by syntax and operand count, which is exactly how `SqlOperatorTable` was designed to work. The published map is deeply immutable (`List.copyOf` on each value, then `Map.copyOf`on the outer map) so the singleton remains safe for concurrent reads. A separate `registerOverride()` method handles the `IS_NULL` / `IS_NOT_NULL` replacement case, where Pinot intentionally force replaces a standard operator. This keeps the intent explicit and avoids conflating "add an overload" with "replace the existing operator." ### Bug 2: Function name mismatch Even after validation succeeds, the downstream code produces the function name `"MINUS_PREFIX"` (V2 via `getFunctionName()`) or `"minusprefix"` (V1 via `canonicalizeFunctionNamePreservingSpecialKey(SqlKind.MINUS_PREFIX.name())`). The existing `NegateScalarFunction` registers itself as `"negate"`. Nothing bridged this gap, so the function lookup failed. **Fix:** Added explicit `MINUS_PREFIX` cases in both engine paths: In `CalciteSqlParser.compileFunctionExpression()` (V1): maps `MINUS_PREFIX` to `NegateScalarFunction.FUNCTION_NAME` and unwraps `PLUS_PREFIX` to return the operand directly since unary plus is the identity operation. In `RexExpressionUtils.fromRexCall()` (V2): maps `MINUS_PREFIX` to `NegateScalarFunction.FUNCTION_NAME`. `PLUS_PREFIX` is intentionally not handled here because Calcite's `StandardConvertletTable` strips unary plus during the SqlNode to RexNode conversion phase, so it never reaches this switch. PinotConvertletTable delegates to StandardConvertletTable for any operator it does not override, and PLUS_PREFIX is not overridden. A `FUNCTION_NAME` constant was added to `NegateScalarFunction` to eliminate the hardcoded `"negate"` string across the codebase. ### Test coverage **V1 parser tests** (CalciteSqlCompilerTest): 14 new test methods covering unary minus in SELECT, WHERE, ORDER BY, GROUP BY, HAVING, aggregations, CAST, CASE, DISTINCT, JOIN conditions, IN lists, aliases, binary/unary mixed expressions, and negative literals. Also verifies that unary plus is stripped to a bare identifier with no function wrapper. **V2 planner tests** (QueryCompilationTest): around 50 data provider entries in `provideUnaryOperatorQueries()` covering all numeric column types (INT, LONG, BIG_DECIMAL), GROUP BY, aggregations, HAVING, aliases, CAST, JOIN, window PARTITION BY, DISTINCT, IN/BETWEEN, and subqueries. **V2 runtime tests** (MathFuncs.json): 5 new test sections with roughly 30 queries verified against H2 reference results. Covers basic negation, unary plus identity, aggregations with unary minus, GROUP BY with unary minus, and miscellaneous cases like double negation, ORDER BY, and WHERE clauses. ### Regression validation Full test suite passed across all affected modules with 0 failures: | Module | Tests | |--------|-------| | pinot-common | 2652 | | pinot-query-planner | 697 | | pinot-query-runtime | 3307 | | pinot-core | 311 | | pinot-broker | 164 | | **Total** | **7131** | ### Wire compatibility The function name `"negate"` is what gets sent over the wire. Older servers already have `NegateScalarFunction` registered in the `FunctionRegistry`, so queries from a new broker to an old server work without issues. Old broker to new server is also fine since those brokers never produced unary operator queries in the first place. -- 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]
