Add support for unary +/- operators in SQL queries#18444
Conversation
Signed-off-by: Jinesh Parakh <jineshparakh@hotmail.com>
xiangfu0
added
sql-compliance
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end support for unary prefix - and + in Pinot SQL across both the V1 (single-stage) and V2 (multi-stage) engines by fixing Calcite operator registration/lookup and ensuring unary - maps to Pinot’s negate scalar function during planning/compilation.
Changes:
- Fix Calcite operator-table name collisions by allowing multiple
SqlOperators per canonical name (e.g., binary-and unary-) inPinotOperatorTable. - Map
SqlKind.MINUS_PREFIXtoNegateScalarFunctionin both V1 (CalciteSqlParser) and V2 (RexExpressionUtils); unwrap unary+as identity in V1. - Add/extend planner, compiler, and runtime query test coverage for unary operators.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pinot-query-runtime/src/test/resources/queries/MathFuncs.json | Adds runtime query-suite sections covering unary minus/plus behavior across types and query clauses. |
| pinot-query-planner/src/test/java/org/apache/pinot/query/QueryCompilationTest.java | Adds planner test cases to ensure unary prefix operators plan successfully across many query shapes. |
| pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RexExpressionUtils.java | Ensures V2 RexCall translation maps MINUS_PREFIX to negate. |
| pinot-query-planner/src/main/java/org/apache/pinot/calcite/sql/fun/PinotOperatorTable.java | Allows multiple operators per canonical name and registers UNARY_MINUS/UNARY_PLUS. |
| pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java | Adds V1 compilation assertions for unary minus/plus across SELECT/WHERE/GROUP BY/HAVING/etc. |
| pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java | Maps MINUS_PREFIX to negate and unwraps PLUS_PREFIX during V1 compilation. |
| pinot-common/src/main/java/org/apache/pinot/common/function/scalar/arithmetic/NegateScalarFunction.java | Introduces FUNCTION_NAME constant and reuses it in getName(). |
Signed-off-by: Jinesh Parakh <jineshparakh@hotmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18444 +/- ##
============================================
+ Coverage 63.55% 63.69% +0.14%
- Complexity 1717 1735 +18
============================================
Files 3252 3254 +2
Lines 199051 199472 +421
Branches 30838 30981 +143
============================================
+ Hits 126503 127050 +547
+ Misses 62471 62270 -201
- Partials 10077 10152 +75
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
yashmayya
left a comment
yashmayya
left a comment
There was a problem hiding this comment.
Thanks @jineshparakh, this is a nice improvement!
| // -col should produce negate(col), not a binary subtraction | ||
| PinotQuery pinotQuery = compileToPinotQuery("SELECT -col1 FROM myTable"); | ||
| Expression expr = pinotQuery.getSelectList().get(0); | ||
| Assert.assertEquals(expr.getFunctionCall().getOperator(), "negate"); | ||
| Assert.assertEquals(expr.getFunctionCall().getOperands().get(0).getIdentifier().getName(), "col1"); |
There was a problem hiding this comment.
nit: maybe we can also assert that the operand list size is equal to 1?
| } | ||
| ] | ||
| }, | ||
| "unary_minus": { |
There was a problem hiding this comment.
Should we add a case for INT_MIN / LONG_MIN overflow?
There was a problem hiding this comment.
Also maybe some null input cases
|
Re-reviewed and just two small follow-ups that elaborate on my earlier unresolved comments: 1. Null input case (elaborates on this comment) — In 2. Also — your SqlSyntax analysis is correct, |
Description
Queries like
SELECT -price FROM tableorSELECT +col FROM tablepreviously 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
PinotOperatorTablestored operators in a flatMap<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 intohandleUnresolvedFunction, retrievedPINOT_MINUS(aSqlMonotonicBinaryOperator), and threw aClassCastExceptiontrying to cast it toSqlFunction.Fix: Changed the backing store from
Map<String, SqlOperator>toMap<String, List<SqlOperator>>:The new
register()method allows multiple operators with the same canonical name as long as they have differentSqlSyntaxvalues (PREFIX vs BINARY, for example).lookupOperatorOverloads()now returns all matching operators and lets Calcite disambiguate by syntax and operand count, which is exactly howSqlOperatorTablewas designed to work. The published map is deeply immutable (List.copyOfon each value, thenMap.copyOfon the outer map) so the singleton remains safe for concurrent reads.A separate
registerOverride()method handles theIS_NULL/IS_NOT_NULLreplacement 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 viagetFunctionName()) or"minusprefix"(V1 viacanonicalizeFunctionNamePreservingSpecialKey(SqlKind.MINUS_PREFIX.name())). The existingNegateScalarFunctionregisters itself as"negate". Nothing bridged this gap, so the function lookup failed.Fix: Added explicit
MINUS_PREFIXcases in both engine paths:In
CalciteSqlParser.compileFunctionExpression()(V1): mapsMINUS_PREFIXtoNegateScalarFunction.FUNCTION_NAMEand unwrapsPLUS_PREFIXto return the operand directly since unary plus is the identity operation.In
RexExpressionUtils.fromRexCall()(V2): mapsMINUS_PREFIXtoNegateScalarFunction.FUNCTION_NAME.PLUS_PREFIXis intentionally not handled here because Calcite'sStandardConvertletTablestrips 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_NAMEconstant was added toNegateScalarFunctionto 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:
Wire compatibility
The function name
"negate"is what gets sent over the wire. Older servers already haveNegateScalarFunctionregistered in theFunctionRegistry, 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.