You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "jineshparakh (via GitHub)" <gi...@apache.org> on 2026/05/08 07:09:53 UTC
[PR] Add support for unary +/- operators in SQL queries [pinot]
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "jineshparakh (via GitHub)" <gi...@apache.org>.
jineshparakh commented on PR #18444:
URL: https://github.com/apache/pinot/pull/18444#issuecomment-4505297522
@gortiz it would be great if you could review too.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18444:
URL: https://github.com/apache/pinot/pull/18444#discussion_r3207690572
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/sql/fun/PinotOperatorTable.java:
##########
@@ -332,12 +335,12 @@ public static PinotOperatorTable instance(boolean nullHandlingEnabled) {
);
//@formatter:on
- // Key is canonical name
- private final Map<String, SqlOperator> _operatorMap;
+ // Key is canonical name. Multiple operators can share the same name (e.g. binary "-" and unary "-").
+ private final Map<String, List<SqlOperator>> _operatorMap;
Review Comment:
It is `instanceof` vs `for loop`, where `instanceof` should actually be cheaper. `instancof` is usually 1-2 cpu cycle
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18444:
URL: https://github.com/apache/pinot/pull/18444#discussion_r3207138707
##########
pinot-query-runtime/src/test/resources/queries/MathFuncs.json:
##########
@@ -511,5 +511,285 @@
"sql": "SELECT longCol / 1e20 FROM {numTbl}"
}
]
+ },
+ "unary_minus": {
+ "tables": {
+ "numTbl": {
+ "schema": [
+ {"name": "intCol", "type": "INT"},
+ {"name": "longCol", "type": "LONG"},
+ {"name": "floatCol", "type": "FLOAT"},
+ {"name": "doubleCol", "type": "DOUBLE"}
+ ],
+ "inputs": [
+ [0, 0, 0.0, 0.0],
+ [1, 1, 1.5, 1.5],
+ [-1, -1, -1.5, -1.5],
+ [123, 321, 4.2, 4.2],
+ [-456, -456, -7.7, -7.7]
+ ]
+ },
+ "bigDecTbl": {
+ "schema": [
+ {"name": "bigDecCol", "type": "BIG_DECIMAL"}
+ ],
+ "inputs": [
+ ["0"],
+ ["1"],
+ ["-1"],
+ ["123456789012345678901234567890"],
+ ["-123456789012345678901234567890"]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "description": "unary minus on INT column preserves INT type",
+ "sql": "SELECT -intCol FROM {numTbl}"
+ },
+ {
+ "description": "unary minus on LONG column preserves LONG type",
+ "sql": "SELECT -longCol FROM {numTbl}"
+ },
+ {
+ "description": "unary minus on FLOAT column",
+ "sql": "SELECT -floatCol FROM {numTbl}"
+ },
+ {
+ "description": "unary minus on DOUBLE column",
+ "sql": "SELECT -doubleCol FROM {numTbl}"
+ },
+ {
+ "description": "unary minus on BIG_DECIMAL column",
+ "sql": "SELECT -bigDecCol FROM {bigDecTbl}"
+ },
+ {
+ "description": "unary minus in WHERE clause on INT",
+ "sql": "SELECT intCol FROM {numTbl} WHERE -intCol < 0"
+ },
+ {
+ "description": "unary minus in WHERE clause on DOUBLE",
+ "sql": "SELECT doubleCol FROM {numTbl} WHERE -doubleCol < 0"
+ },
+ {
+ "description": "double negation --col returns original value",
+ "sql": "SELECT -(-intCol) FROM {numTbl}"
+ },
+ {
+ "description": "double negation on LONG",
+ "sql": "SELECT -(-longCol) FROM {numTbl}"
+ },
+ {
+ "description": "unary minus on compound expression",
+ "sql": "SELECT -(intCol + longCol) FROM {numTbl}"
+ },
+ {
+ "description": "unary minus mixed with binary arithmetic",
+ "sql": "SELECT intCol - (-longCol) FROM {numTbl}"
+ },
+ {
+ "description": "unary minus on zero returns zero",
+ "sql": "SELECT -intCol FROM {numTbl} WHERE intCol = 0"
+ }
+ ]
+ },
+ "unary_plus": {
+ "tables": {
+ "numTbl": {
+ "schema": [
+ {"name": "intCol", "type": "INT"},
+ {"name": "longCol", "type": "LONG"},
+ {"name": "floatCol", "type": "FLOAT"},
+ {"name": "doubleCol", "type": "DOUBLE"}
+ ],
+ "inputs": [
+ [0, 0, 0.0, 0.0],
+ [1, 1, 1.5, 1.5],
+ [-1, -1, -1.5, -1.5],
+ [123, 321, 4.2, 4.2]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "description": "unary plus on INT is identity",
+ "sql": "SELECT +intCol FROM {numTbl}"
+ },
+ {
+ "description": "unary plus on LONG is identity",
+ "sql": "SELECT +longCol FROM {numTbl}"
+ },
+ {
+ "description": "unary plus on FLOAT is identity",
+ "sql": "SELECT +floatCol FROM {numTbl}"
+ },
+ {
+ "description": "unary plus on DOUBLE is identity",
+ "sql": "SELECT +doubleCol FROM {numTbl}"
+ },
+ {
+ "description": "unary plus combined with unary minus cancels out",
+ "sql": "SELECT +(-intCol) FROM {numTbl}"
+ },
+ {
+ "description": "unary plus in ORDER BY is identity",
+ "sql": "SELECT intCol FROM {numTbl} ORDER BY +intCol"
+ }
+ ]
+ },
+ "unary_minus_aggregations": {
+ "tables": {
+ "numTbl": {
+ "schema": [
+ {"name": "groupKey", "type": "STRING"},
+ {"name": "intCol", "type": "INT"},
+ {"name": "longCol", "type": "LONG"},
+ {"name": "doubleCol", "type": "DOUBLE"}
+ ],
+ "inputs": [
+ ["a", 1, 10, 1.5],
+ ["a", 2, 20, 2.5],
+ ["a", 3, 30, 3.5],
+ ["b", 4, 40, 4.5],
+ ["b", 5, 50, 5.5],
+ ["c", 6, 60, 6.5],
+ ["c", -7, -70, -7.5]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "description": "SUM(-col) equals -SUM(col)",
+ "sql": "SELECT SUM(-intCol), -SUM(intCol) FROM {numTbl}"
+ },
+ {
+ "description": "MAX(-col) equals -MIN(col)",
+ "sql": "SELECT MAX(-intCol), -MIN(intCol) FROM {numTbl}"
+ },
+ {
+ "description": "MIN(-col) equals -MAX(col)",
+ "sql": "SELECT MIN(-intCol), -MAX(intCol) FROM {numTbl}"
+ },
+ {
+ "description": "AVG(-col) equals -AVG(col)",
+ "sql": "SELECT AVG(-doubleCol), -AVG(doubleCol) FROM {numTbl}"
+ },
+ {
+ "description": "COUNT is unaffected by negation of argument",
+ "sql": "SELECT COUNT(-intCol) FROM {numTbl}"
+ },
+ {
+ "description": "negation of aggregate result",
+ "sql": "SELECT -SUM(intCol), -MAX(intCol), -MIN(intCol) FROM {numTbl}"
+ },
+ {
+ "description": "aggregate of compound negation",
+ "sql": "SELECT SUM(-(intCol + longCol)) FROM {numTbl}"
+ },
+ {
+ "description": "long type aggregations with negation",
+ "sql": "SELECT SUM(-longCol), MAX(-longCol), MIN(-longCol) FROM {numTbl}"
+ },
+ {
+ "description": "double type aggregations with negation",
+ "sql": "SELECT SUM(-doubleCol), MAX(-doubleCol), MIN(-doubleCol) FROM {numTbl}"
+ }
+ ]
+ },
+ "unary_minus_group_by": {
+ "tables": {
+ "numTbl": {
+ "schema": [
+ {"name": "groupKey", "type": "STRING"},
+ {"name": "intCol", "type": "INT"},
+ {"name": "longCol", "type": "LONG"}
+ ],
+ "inputs": [
+ ["a", 1, 10],
+ ["a", 2, 20],
+ ["b", 3, 30],
+ ["b", 4, 40],
+ ["c", 5, 50]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "description": "GROUP BY -col -- negation as grouping key",
+ "sql": "SELECT -intCol, COUNT(*) FROM {numTbl} GROUP BY -intCol ORDER BY -intCol"
+ },
+ {
+ "description": "GROUP BY normal key, aggregate over negation",
+ "sql": "SELECT groupKey, SUM(-intCol) FROM {numTbl} GROUP BY groupKey ORDER BY groupKey"
+ },
+ {
+ "description": "GROUP BY normal key, negation of aggregate",
+ "sql": "SELECT groupKey, -SUM(intCol) FROM {numTbl} GROUP BY groupKey ORDER BY groupKey"
+ },
+ {
+ "description": "HAVING -SUM(col) > threshold",
+ "sql": "SELECT groupKey, SUM(intCol) FROM {numTbl} GROUP BY groupKey HAVING -SUM(intCol) > -7 ORDER BY groupKey"
+ },
+ {
+ "description": "HAVING SUM(-col) < threshold",
+ "sql": "SELECT groupKey, SUM(longCol) FROM {numTbl} GROUP BY groupKey HAVING SUM(-longCol) < -20 ORDER BY groupKey"
+ },
+ {
+ "description": "DISTINCT -col",
+ "sql": "SELECT DISTINCT -intCol FROM {numTbl} ORDER BY -intCol"
+ },
+ {
+ "description": "COUNT(DISTINCT -col)",
+ "sql": "SELECT COUNT(DISTINCT -intCol) FROM {numTbl}"
+ }
+ ]
+ },
+ "unary_minus_misc": {
+ "tables": {
+ "numTbl": {
+ "schema": [
+ {"name": "intCol", "type": "INT"},
+ {"name": "longCol", "type": "LONG"},
+ {"name": "doubleCol", "type": "DOUBLE"}
+ ],
+ "inputs": [
+ [1, 10, 1.5],
+ [2, 20, 2.5],
+ [3, 30, 3.5],
+ [4, 40, 4.5],
+ [5, 50, 5.5]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "description": "unary minus inside CAST",
+ "sql": "SELECT CAST(-intCol AS DOUBLE) FROM {numTbl}"
+ },
+ {
+ "description": "negation of CAST result",
+ "sql": "SELECT -CAST(intCol AS DOUBLE) FROM {numTbl}"
+ },
+ {
+ "description": "alias on negated column, ORDER BY alias",
+ "sql": "SELECT -intCol AS neg FROM {numTbl} ORDER BY neg"
+ },
+ {
+ "description": "IN with negative literals",
+ "sql": "SELECT intCol FROM {numTbl} WHERE intCol IN (1, 2, 3) ORDER BY intCol"
Review Comment:
The query labeled "IN with negative literals" does not actually include negative literals (it uses `IN (1, 2, 3)`). This means the runtime suite isn’t exercising negative-literal parsing/execution for IN lists as intended. Update the SQL to use negative literals (e.g., `IN (-1, -2, -3)`) or adjust the description to match the query.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "jineshparakh (via GitHub)" <gi...@apache.org>.
jineshparakh commented on code in PR #18444:
URL: https://github.com/apache/pinot/pull/18444#discussion_r3242512731
##########
pinot-query-runtime/src/test/resources/queries/MathFuncs.json:
##########
@@ -511,5 +511,285 @@
"sql": "SELECT longCol / 1e20 FROM {numTbl}"
}
]
+ },
+ "unary_minus": {
Review Comment:
Will add
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "jineshparakh (via GitHub)" <gi...@apache.org>.
jineshparakh commented on code in PR #18444:
URL: https://github.com/apache/pinot/pull/18444#discussion_r3224804256
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/sql/fun/PinotOperatorTable.java:
##########
@@ -440,9 +476,9 @@ public void lookupOperatorOverloads(SqlIdentifier opName, @Nullable SqlFunctionC
return;
}
String canonicalName = FunctionRegistry.canonicalize(opName.getSimple());
- SqlOperator operator = _operatorMap.get(canonicalName);
- if (operator != null) {
- operatorList.add(operator);
+ List<SqlOperator> operators = _operatorMap.get(canonicalName);
+ if (operators != null) {
+ operatorList.addAll(operators);
Review Comment:
Calcite already post-filters downstream (`SqlUtil.lookupSubjectRoutinesByName` filters by `instanceof SqlFunction` for FUNCTION lookups and by exact syntax match otherwise), so a filter here is either redundant or actively harmful. Tried several variants and ran `QueryCompilationTest` (113 query-plan tests) on each:
| Filter | Behavior | Test result |
|--------|---------|-------------|
| No filter (current `addAll`) | Returns all operators registered under the canonical name | 113/113 pass |
| `op.getSyntax() == syntax` (strict) | Drops anything not matching the requested syntax | 17/113 fail — breaks `SELECT *`, `SELECT COUNT(*)`, all aggregates, and `ADD/SUB(<TIMESTAMP>, <TIMESTAMP>)` |
| `op.getSyntax() == syntax \|\| (syntax==FUNCTION && op instanceof SqlFunction)` (mirror of Calcite's `ReflectiveSqlOperatorTable`) | Strict, plus accepts any `SqlFunction` for FUNCTION lookups | 2/113 fail — `ADD/SUB(<TIMESTAMP>, <TIMESTAMP>)` still break because `PINOT_PLUS`/`PINOT_MINUS` are `SqlBinaryOperator`, not `SqlFunction` |
| `syntax == FUNCTION \|\| op.getSyntax() == syntax` (permissive on FUNCTION) | Strict for non-FUNCTION lookups, no filter for FUNCTION | 113/113 pass — but functionally identical to `addAll` for our data |
**Root cause:** Pinot registers `PINOT_PLUS` (BINARY) under the function aliases `"add"`/`"plus"` and `PINOT_MINUS` under `"sub"`/`"minus"`. For `ADD(ts1, ts2)`, `SqlValidatorImpl.performUnconditionalRewrites` (SqlValidatorImpl.java:1604-1619) calls `lookupOperatorOverloads("add", FUNCTION)` and rewrites the call's operator to the single result. Any filter that drops `PINOT_PLUS` here (because it's BINARY or not a SqlFunction) defeats the rewrite, the call stays as `SqlUnresolvedFunction`, then `SqlFunction.deriveType`'s strict lookup throws `No match found`.
I plan to go with with `addAll`: same behavior as the original single-operator code, just generalized to a multimap.
Does this sound reasonable or do you want me to try few more approaches?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "jineshparakh (via GitHub)" <gi...@apache.org>.
jineshparakh commented on PR #18444:
URL: https://github.com/apache/pinot/pull/18444#issuecomment-4515332695
Docs PR: https://github.com/pinot-contrib/pinot-docs/pull/818
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18444:
URL: https://github.com/apache/pinot/pull/18444#discussion_r3210963492
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/sql/fun/PinotOperatorTable.java:
##########
@@ -332,12 +335,12 @@ public static PinotOperatorTable instance(boolean nullHandlingEnabled) {
);
//@formatter:on
- // Key is canonical name
- private final Map<String, SqlOperator> _operatorMap;
+ // Key is canonical name. Multiple operators can share the same name (e.g. binary "-" and unary "-").
+ private final Map<String, List<SqlOperator>> _operatorMap;
Review Comment:
Sure. Comparing to the remaining operations, this is definitely insignificant. I feel readability won't change much thus suggested for the small performance improvement. Go ahead with the current approach
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #18444:
URL: https://github.com/apache/pinot/pull/18444#discussion_r3229111181
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/sql/fun/PinotOperatorTable.java:
##########
@@ -440,9 +476,9 @@ public void lookupOperatorOverloads(SqlIdentifier opName, @Nullable SqlFunctionC
return;
}
String canonicalName = FunctionRegistry.canonicalize(opName.getSimple());
- SqlOperator operator = _operatorMap.get(canonicalName);
- if (operator != null) {
- operatorList.add(operator);
+ List<SqlOperator> operators = _operatorMap.get(canonicalName);
+ if (operators != null) {
+ operatorList.addAll(operators);
Review Comment:
Sounds good, current approach looks okay in that case.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "jineshparakh (via GitHub)" <gi...@apache.org>.
jineshparakh commented on PR #18444:
URL: https://github.com/apache/pinot/pull/18444#issuecomment-4505286843
@yashmayya I have tried to address all review comments. Can you please re-review?
Thanks!
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya merged PR #18444:
URL: https://github.com/apache/pinot/pull/18444
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "jineshparakh (via GitHub)" <gi...@apache.org>.
jineshparakh commented on code in PR #18444:
URL: https://github.com/apache/pinot/pull/18444#discussion_r3207208190
##########
pinot-query-runtime/src/test/resources/queries/MathFuncs.json:
##########
@@ -511,5 +511,285 @@
"sql": "SELECT longCol / 1e20 FROM {numTbl}"
}
]
+ },
+ "unary_minus": {
+ "tables": {
+ "numTbl": {
+ "schema": [
+ {"name": "intCol", "type": "INT"},
+ {"name": "longCol", "type": "LONG"},
+ {"name": "floatCol", "type": "FLOAT"},
+ {"name": "doubleCol", "type": "DOUBLE"}
+ ],
+ "inputs": [
+ [0, 0, 0.0, 0.0],
+ [1, 1, 1.5, 1.5],
+ [-1, -1, -1.5, -1.5],
+ [123, 321, 4.2, 4.2],
+ [-456, -456, -7.7, -7.7]
+ ]
+ },
+ "bigDecTbl": {
+ "schema": [
+ {"name": "bigDecCol", "type": "BIG_DECIMAL"}
+ ],
+ "inputs": [
+ ["0"],
+ ["1"],
+ ["-1"],
+ ["123456789012345678901234567890"],
+ ["-123456789012345678901234567890"]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "description": "unary minus on INT column preserves INT type",
+ "sql": "SELECT -intCol FROM {numTbl}"
+ },
+ {
+ "description": "unary minus on LONG column preserves LONG type",
+ "sql": "SELECT -longCol FROM {numTbl}"
+ },
+ {
+ "description": "unary minus on FLOAT column",
+ "sql": "SELECT -floatCol FROM {numTbl}"
+ },
+ {
+ "description": "unary minus on DOUBLE column",
+ "sql": "SELECT -doubleCol FROM {numTbl}"
+ },
+ {
+ "description": "unary minus on BIG_DECIMAL column",
+ "sql": "SELECT -bigDecCol FROM {bigDecTbl}"
+ },
+ {
+ "description": "unary minus in WHERE clause on INT",
+ "sql": "SELECT intCol FROM {numTbl} WHERE -intCol < 0"
+ },
+ {
+ "description": "unary minus in WHERE clause on DOUBLE",
+ "sql": "SELECT doubleCol FROM {numTbl} WHERE -doubleCol < 0"
+ },
+ {
+ "description": "double negation --col returns original value",
+ "sql": "SELECT -(-intCol) FROM {numTbl}"
+ },
+ {
+ "description": "double negation on LONG",
+ "sql": "SELECT -(-longCol) FROM {numTbl}"
+ },
+ {
+ "description": "unary minus on compound expression",
+ "sql": "SELECT -(intCol + longCol) FROM {numTbl}"
+ },
+ {
+ "description": "unary minus mixed with binary arithmetic",
+ "sql": "SELECT intCol - (-longCol) FROM {numTbl}"
+ },
+ {
+ "description": "unary minus on zero returns zero",
+ "sql": "SELECT -intCol FROM {numTbl} WHERE intCol = 0"
+ }
+ ]
+ },
+ "unary_plus": {
+ "tables": {
+ "numTbl": {
+ "schema": [
+ {"name": "intCol", "type": "INT"},
+ {"name": "longCol", "type": "LONG"},
+ {"name": "floatCol", "type": "FLOAT"},
+ {"name": "doubleCol", "type": "DOUBLE"}
+ ],
+ "inputs": [
+ [0, 0, 0.0, 0.0],
+ [1, 1, 1.5, 1.5],
+ [-1, -1, -1.5, -1.5],
+ [123, 321, 4.2, 4.2]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "description": "unary plus on INT is identity",
+ "sql": "SELECT +intCol FROM {numTbl}"
+ },
+ {
+ "description": "unary plus on LONG is identity",
+ "sql": "SELECT +longCol FROM {numTbl}"
+ },
+ {
+ "description": "unary plus on FLOAT is identity",
+ "sql": "SELECT +floatCol FROM {numTbl}"
+ },
+ {
+ "description": "unary plus on DOUBLE is identity",
+ "sql": "SELECT +doubleCol FROM {numTbl}"
+ },
+ {
+ "description": "unary plus combined with unary minus cancels out",
+ "sql": "SELECT +(-intCol) FROM {numTbl}"
+ },
+ {
+ "description": "unary plus in ORDER BY is identity",
+ "sql": "SELECT intCol FROM {numTbl} ORDER BY +intCol"
+ }
+ ]
+ },
+ "unary_minus_aggregations": {
+ "tables": {
+ "numTbl": {
+ "schema": [
+ {"name": "groupKey", "type": "STRING"},
+ {"name": "intCol", "type": "INT"},
+ {"name": "longCol", "type": "LONG"},
+ {"name": "doubleCol", "type": "DOUBLE"}
+ ],
+ "inputs": [
+ ["a", 1, 10, 1.5],
+ ["a", 2, 20, 2.5],
+ ["a", 3, 30, 3.5],
+ ["b", 4, 40, 4.5],
+ ["b", 5, 50, 5.5],
+ ["c", 6, 60, 6.5],
+ ["c", -7, -70, -7.5]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "description": "SUM(-col) equals -SUM(col)",
+ "sql": "SELECT SUM(-intCol), -SUM(intCol) FROM {numTbl}"
+ },
+ {
+ "description": "MAX(-col) equals -MIN(col)",
+ "sql": "SELECT MAX(-intCol), -MIN(intCol) FROM {numTbl}"
+ },
+ {
+ "description": "MIN(-col) equals -MAX(col)",
+ "sql": "SELECT MIN(-intCol), -MAX(intCol) FROM {numTbl}"
+ },
+ {
+ "description": "AVG(-col) equals -AVG(col)",
+ "sql": "SELECT AVG(-doubleCol), -AVG(doubleCol) FROM {numTbl}"
+ },
+ {
+ "description": "COUNT is unaffected by negation of argument",
+ "sql": "SELECT COUNT(-intCol) FROM {numTbl}"
+ },
+ {
+ "description": "negation of aggregate result",
+ "sql": "SELECT -SUM(intCol), -MAX(intCol), -MIN(intCol) FROM {numTbl}"
+ },
+ {
+ "description": "aggregate of compound negation",
+ "sql": "SELECT SUM(-(intCol + longCol)) FROM {numTbl}"
+ },
+ {
+ "description": "long type aggregations with negation",
+ "sql": "SELECT SUM(-longCol), MAX(-longCol), MIN(-longCol) FROM {numTbl}"
+ },
+ {
+ "description": "double type aggregations with negation",
+ "sql": "SELECT SUM(-doubleCol), MAX(-doubleCol), MIN(-doubleCol) FROM {numTbl}"
+ }
+ ]
+ },
+ "unary_minus_group_by": {
+ "tables": {
+ "numTbl": {
+ "schema": [
+ {"name": "groupKey", "type": "STRING"},
+ {"name": "intCol", "type": "INT"},
+ {"name": "longCol", "type": "LONG"}
+ ],
+ "inputs": [
+ ["a", 1, 10],
+ ["a", 2, 20],
+ ["b", 3, 30],
+ ["b", 4, 40],
+ ["c", 5, 50]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "description": "GROUP BY -col -- negation as grouping key",
+ "sql": "SELECT -intCol, COUNT(*) FROM {numTbl} GROUP BY -intCol ORDER BY -intCol"
+ },
+ {
+ "description": "GROUP BY normal key, aggregate over negation",
+ "sql": "SELECT groupKey, SUM(-intCol) FROM {numTbl} GROUP BY groupKey ORDER BY groupKey"
+ },
+ {
+ "description": "GROUP BY normal key, negation of aggregate",
+ "sql": "SELECT groupKey, -SUM(intCol) FROM {numTbl} GROUP BY groupKey ORDER BY groupKey"
+ },
+ {
+ "description": "HAVING -SUM(col) > threshold",
+ "sql": "SELECT groupKey, SUM(intCol) FROM {numTbl} GROUP BY groupKey HAVING -SUM(intCol) > -7 ORDER BY groupKey"
+ },
+ {
+ "description": "HAVING SUM(-col) < threshold",
+ "sql": "SELECT groupKey, SUM(longCol) FROM {numTbl} GROUP BY groupKey HAVING SUM(-longCol) < -20 ORDER BY groupKey"
+ },
+ {
+ "description": "DISTINCT -col",
+ "sql": "SELECT DISTINCT -intCol FROM {numTbl} ORDER BY -intCol"
+ },
+ {
+ "description": "COUNT(DISTINCT -col)",
+ "sql": "SELECT COUNT(DISTINCT -intCol) FROM {numTbl}"
+ }
+ ]
+ },
+ "unary_minus_misc": {
+ "tables": {
+ "numTbl": {
+ "schema": [
+ {"name": "intCol", "type": "INT"},
+ {"name": "longCol", "type": "LONG"},
+ {"name": "doubleCol", "type": "DOUBLE"}
+ ],
+ "inputs": [
+ [1, 10, 1.5],
+ [2, 20, 2.5],
+ [3, 30, 3.5],
+ [4, 40, 4.5],
+ [5, 50, 5.5]
+ ]
+ }
+ },
+ "queries": [
+ {
+ "description": "unary minus inside CAST",
+ "sql": "SELECT CAST(-intCol AS DOUBLE) FROM {numTbl}"
+ },
+ {
+ "description": "negation of CAST result",
+ "sql": "SELECT -CAST(intCol AS DOUBLE) FROM {numTbl}"
+ },
+ {
+ "description": "alias on negated column, ORDER BY alias",
+ "sql": "SELECT -intCol AS neg FROM {numTbl} ORDER BY neg"
+ },
+ {
+ "description": "IN with negative literals",
+ "sql": "SELECT intCol FROM {numTbl} WHERE intCol IN (1, 2, 3) ORDER BY intCol"
Review Comment:
Done. Fixed other tests too.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18444:
URL: https://github.com/apache/pinot/pull/18444#discussion_r3207461731
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/sql/fun/PinotOperatorTable.java:
##########
@@ -332,12 +335,12 @@ public static PinotOperatorTable instance(boolean nullHandlingEnabled) {
);
//@formatter:on
- // Key is canonical name
- private final Map<String, SqlOperator> _operatorMap;
+ // Key is canonical name. Multiple operators can share the same name (e.g. binary "-" and unary "-").
+ private final Map<String, List<SqlOperator>> _operatorMap;
Review Comment:
Change every value to a list could add quite some memory overhead. We can change value to `Object` which can be either `SqlOperator` or `List<SqlOperator>`. Majority of the operators should be single
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "jineshparakh (via GitHub)" <gi...@apache.org>.
jineshparakh commented on PR #18444:
URL: https://github.com/apache/pinot/pull/18444#issuecomment-4508065862
@gortiz can you please retrigger the failed job?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "jineshparakh (via GitHub)" <gi...@apache.org>.
jineshparakh commented on code in PR #18444:
URL: https://github.com/apache/pinot/pull/18444#discussion_r3279012635
##########
pinot-query-runtime/src/test/resources/queries/MathFuncs.json:
##########
@@ -511,5 +511,285 @@
"sql": "SELECT longCol / 1e20 FROM {numTbl}"
}
]
+ },
+ "unary_minus": {
Review Comment:
Done
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -1101,6 +1101,236 @@ public void testPolymorphicArithmeticScalarFunctionsCompile() {
Assert.assertEquals(selectList.get(4).getFunctionCall().getOperator(), "moduloorzero");
}
+ @Test
+ public void testUnaryMinusSyntaxCompilesAsNegate() {
+ // -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");
Review Comment:
Done
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18444:
URL: https://github.com/apache/pinot/pull/18444#issuecomment-4404979204
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18444?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `92.30769%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.70%. Comparing base ([`642f07d`](https://app.codecov.io/gh/apache/pinot/commit/642f07d340225f2d4f904527e6591bc1bb16bacf?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e79e2ee`](https://app.codecov.io/gh/apache/pinot/commit/e79e2ee06c397b88a201de2db76950567debda07?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 15 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18444?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ache/pinot/calcite/sql/fun/PinotOperatorTable.java](https://app.codecov.io/gh/apache/pinot/pull/18444?src=pr&el=tree&filepath=pinot-query-planner%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcalcite%2Fsql%2Ffun%2FPinotOperatorTable.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2FsY2l0ZS9zcWwvZnVuL1Bpbm90T3BlcmF0b3JUYWJsZS5qYXZh) | 90.90% | [0 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18444?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (642f07d) and HEAD (e79e2ee). Click for more details.
>
> <details><summary>HEAD has 16 uploads less than BASE</summary>
>
>| Flag | BASE (642f07d) | HEAD (e79e2ee) |
>|------|------|------|
>|java-21|5|1|
>|unittests|2|1|
>|temurin|5|1|
>|unittests2|1|0|
>|integration|3|0|
>|integration1|1|0|
>|integration2|1|0|
>|custom-integration1|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18444 +/- ##
============================================
- Coverage 63.55% 55.70% -7.85%
+ Complexity 1717 866 -851
============================================
Files 3252 2543 -709
Lines 199051 147993 -51058
Branches 30838 23923 -6915
============================================
- Hits 126503 82442 -44061
+ Misses 62471 58481 -3990
+ Partials 10077 7070 -3007
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18444/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/18444/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/18444/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/18444/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/18444/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18444/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.70% <92.30%> (-7.85%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18444/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.70% <92.30%> (-7.85%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18444/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.70% <92.30%> (-7.85%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18444/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.70% <92.30%> (+0.07%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18444/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/18444?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "jineshparakh (via GitHub)" <gi...@apache.org>.
jineshparakh commented on code in PR #18444:
URL: https://github.com/apache/pinot/pull/18444#discussion_r3207606066
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/sql/fun/PinotOperatorTable.java:
##########
@@ -332,12 +335,12 @@ public static PinotOperatorTable instance(boolean nullHandlingEnabled) {
);
//@formatter:on
- // Key is canonical name
- private final Map<String, SqlOperator> _operatorMap;
+ // Key is canonical name. Multiple operators can share the same name (e.g. binary "-" and unary "-").
+ private final Map<String, List<SqlOperator>> _operatorMap;
Review Comment:
We should not have a lot of operators. I'm expecting less than 1000. There are two `_operatorMap` instances per classloader which is also not a lot.
I think the overhead will be really small ~ few hundred KBs.
Changing it to `Object` affects readability and will require `instanceof` checks at every access point.
@Jackie-Jiang What do you think?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "jineshparakh (via GitHub)" <gi...@apache.org>.
jineshparakh commented on code in PR #18444:
URL: https://github.com/apache/pinot/pull/18444#discussion_r3207808692
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/sql/fun/PinotOperatorTable.java:
##########
@@ -332,12 +335,12 @@ public static PinotOperatorTable instance(boolean nullHandlingEnabled) {
);
//@formatter:on
- // Key is canonical name
- private final Map<String, SqlOperator> _operatorMap;
+ // Key is canonical name. Multiple operators can share the same name (e.g. binary "-" and unary "-").
+ private final Map<String, List<SqlOperator>> _operatorMap;
Review Comment:
Do you think it would be a significant impact?
I concerned to let go readability for a micro optimization.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "jineshparakh (via GitHub)" <gi...@apache.org>.
jineshparakh commented on code in PR #18444:
URL: https://github.com/apache/pinot/pull/18444#discussion_r3224036170
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/sql/fun/PinotOperatorTable.java:
##########
@@ -332,12 +335,12 @@ public static PinotOperatorTable instance(boolean nullHandlingEnabled) {
);
//@formatter:on
- // Key is canonical name
- private final Map<String, SqlOperator> _operatorMap;
+ // Key is canonical name. Multiple operators can share the same name (e.g. binary "-" and unary "-").
+ private final Map<String, List<SqlOperator>> _operatorMap;
Review Comment:
Okay thanks. Keeping the current approach.
Resolving this 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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add support for unary +/- operators in SQL queries [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #18444:
URL: https://github.com/apache/pinot/pull/18444#discussion_r3210557905
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -1101,6 +1101,236 @@ public void testPolymorphicArithmeticScalarFunctionsCompile() {
Assert.assertEquals(selectList.get(4).getFunctionCall().getOperator(), "moduloorzero");
}
+ @Test
+ public void testUnaryMinusSyntaxCompilesAsNegate() {
+ // -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");
Review Comment:
nit: maybe we can also assert that the operand list size is equal to 1?
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/sql/fun/PinotOperatorTable.java:
##########
@@ -332,12 +335,12 @@ public static PinotOperatorTable instance(boolean nullHandlingEnabled) {
);
//@formatter:on
- // Key is canonical name
- private final Map<String, SqlOperator> _operatorMap;
+ // Key is canonical name. Multiple operators can share the same name (e.g. binary "-" and unary "-").
+ private final Map<String, List<SqlOperator>> _operatorMap;
Review Comment:
> I think the overhead will be really small ~ few hundred KBs.
Probably even less given that Java has optimized small immutable lists that we'll be using here through `List.copyOf`
> It is instanceof vs for loop, where instanceof should actually be cheaper. instancof is usually 1-2 cpu cycle
But this isn't a per-row execution, it will be during query planning. The "for loop" over a singleton list is one iteration, which the JIT will likely inline and unroll into something indistinguishable from a single load, no? I don't think this is gonna have any measurable cost.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/sql/fun/PinotOperatorTable.java:
##########
@@ -332,12 +335,12 @@ public static PinotOperatorTable instance(boolean nullHandlingEnabled) {
);
//@formatter:on
- // Key is canonical name
- private final Map<String, SqlOperator> _operatorMap;
+ // Key is canonical name. Multiple operators can share the same name (e.g. binary "-" and unary "-").
+ private final Map<String, List<SqlOperator>> _operatorMap;
Review Comment:
+1 to Jinesh's point, this seems like a pretty big hit on readability for a fairly insignificant gain given the number of operators.
##########
pinot-query-runtime/src/test/resources/queries/MathFuncs.json:
##########
@@ -511,5 +511,285 @@
"sql": "SELECT longCol / 1e20 FROM {numTbl}"
}
]
+ },
+ "unary_minus": {
Review Comment:
Also maybe some null input cases
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/sql/fun/PinotOperatorTable.java:
##########
@@ -440,9 +476,9 @@ public void lookupOperatorOverloads(SqlIdentifier opName, @Nullable SqlFunctionC
return;
}
String canonicalName = FunctionRegistry.canonicalize(opName.getSimple());
- SqlOperator operator = _operatorMap.get(canonicalName);
- if (operator != null) {
- operatorList.add(operator);
+ List<SqlOperator> operators = _operatorMap.get(canonicalName);
+ if (operators != null) {
+ operatorList.addAll(operators);
Review Comment:
Shouldn't we honor the requested `SqlSyntax`?
##########
pinot-query-runtime/src/test/resources/queries/MathFuncs.json:
##########
@@ -511,5 +511,285 @@
"sql": "SELECT longCol / 1e20 FROM {numTbl}"
}
]
+ },
+ "unary_minus": {
Review Comment:
Should we add a case for `INT_MIN` / `LONG_MIN` overflow?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org