gortiz commented on code in PR #20148:
URL: https://github.com/apache/pinot/pull/20148#discussion_r4478386136
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -745,6 +746,7 @@ protected BrokerResponse doHandleRequest(long requestId,
String query, SqlNodeAn
setTableName(serverBrokerRequest, realtimeTableName);
handleExpressionOverride(serverPinotQuery,
_tableCache.getExpressionOverrideMap(realtimeTableName));
handleTimestampIndexOverride(serverPinotQuery, realtimeTableConfig);
+ _queryOptimizer.optimize(serverPinotQuery, schema);
Review Comment:
+1 to this, and I'd prefer to fix it in this PR rather than in a follow-up.
It's the same bug through a different entry point, and this fix is going to be
backported for a customer, so it's better if one cherry-pick covers both paths.
In `prepareBaseTableHybridRoute`, the offline-only and realtime-only branches
need the same `_queryOptimizer.optimize(baseBrokerRequest.getPinotQuery(),
schema)` after the overrides.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -736,6 +736,7 @@ protected BrokerResponse doHandleRequest(long requestId,
String query, SqlNodeAn
setTableName(serverBrokerRequest, offlineTableName);
handleExpressionOverride(serverPinotQuery,
_tableCache.getExpressionOverrideMap(offlineTableName));
handleTimestampIndexOverride(serverPinotQuery, offlineTableConfig);
+ _queryOptimizer.optimize(serverPinotQuery, schema);
Review Comment:
Non-blocking: with this change every OFFLINE-only and REALTIME-only query
goes through the optimizer twice, including the ones without any override. The
cost is negligible, but it changes the request shape on the most common path,
and that's what makes this risky to backport. What do you think about
re-optimizing only when an override actually changed something? For example,
`handleExpressionOverride` and `handleTimestampIndexOverride` could return a
boolean, or at least we could skip it when `getExpressionOverrideMap()` returns
null. The hybrid branch can keep the unconditional call.
##########
pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandlerTest.java:
##########
@@ -1169,6 +1169,96 @@ protected BrokerResponseNative processBrokerRequest(long
requestId,
"combined filter must retain the original WHERE clause, got literals:
" + literalValues);
}
+ /// When an expression override rewrites a nested predicate to a literal
(e.g. `REGEXP_LIKE(foo, '.*') -> TRUE`),
+ /// the broker re-runs the query optimizer for offline-only and
realtime-only tables so that the literal is removed
+ /// before routing, as it already does for hybrid tables. The override is
applied after the initial optimization, and
+ /// a leftover `TRUE` operand would reach the broker segment pruners, which
expect every filter node to be a function
+ /// call (e.g. `TimeSegmentPruner.getFilterTimeIntervals` would throw a
`NullPointerException`).
+ @Test
+ public void
testExpressionOverrideToLiteralIsOptimizedAwayForNonHybridTables()
Review Comment:
Could we add two more cases here? First, an override that rewrites the
predicate to `FALSE`. With this change the broker now returns an empty response
without contacting the servers, and that behavior isn't covered. Second, an
override under an `OR`, where `OR(x, TRUE)` has to collapse to no filter at
all. Both are cheap with the helper you already have.
--
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]
