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