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: [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]
