gortiz commented on code in PR #19558:
URL: https://github.com/apache/pinot/pull/19558#discussion_r4015644797
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/WorkerManager.java:
##########
@@ -1785,8 +1789,10 @@ private PartitionTableInfo
calculatePartitionTableInfo(String tableName) {
String realtimeTableName =
TableNameBuilder.REALTIME.tableNameWithType(tableName);
boolean offlineRoutingExists =
_routingManager.routingExists(offlineTableName);
boolean realtimeRoutingExists =
_routingManager.routingExists(realtimeTableName);
- Preconditions.checkState(offlineRoutingExists || realtimeRoutingExists,
"Routing doesn't exist for table: %s",
- tableName);
+ if (!offlineRoutingExists && !realtimeRoutingExists) {
Review Comment:
`calculatePartitionTableInfo` has a second caller that this change breaks:
`inferTableOptions` (line ~1768) wraps it in `catch (IllegalStateException e) {
return null; }` so the implicit table-hint path degrades quietly to a shuffled
plan. That contract is documented a few lines below this hunk, in the javadoc
of `checkNoSegmentsWithInvalidPartition`:
> Throws `IllegalStateException` rather than using `Preconditions` so that
the implicit table hint path (`#inferTableOptions`) keeps degrading quietly to
a non-partitioned (shuffled) plan.
`QueryException extends PinotRuntimeException extends RuntimeException`, not
`IllegalStateException`, so the new throw walks straight out of
`inferTableOptions`, out of `PinotImplicitTableHintRule#onMatch`, out of the
trait `HepProgram`, and lands in `QueryEnvironment#optimize()` — whose only
handler is `catch (Throwable e) { throw
QueryErrorCode.QUERY_PLANNING.asException(...) }`, with no `QueryException`
passthrough (unlike `validate()`, `toRelation()` and `planQuery()`, which all
have one).
Repro on this branch, same environment as the new partitioned test but with
the `tableOptions` hint dropped and `inferPartitionHint` enabled:
```
SET inferPartitionHint=true; SELECT col2 FROM testTable WHERE col1 = 'foo'
compile() threw QueryException code=QUERY_PLANNING
msg=Error optimizing query: Routing doesn't exist for table: testTable
```
On master the same query gets past compilation and fails in `planQuery`
instead. So for brokers with `pinot.broker.multistage.infer.partition.hint`
enabled (or the per-query option), this replaces 450 with **720**, not 410 —
and 720 maps to `BAD_REQUEST` and is in `CRITICAL_ERROR_CODES`, so the caller
is blamed for it and it stays SLA-critical.
Neither new test can catch this. The partitioned one passes `partition_key`,
`partition_function` and `partition_size` together, so
`PinotImplicitTableHintRule#getHintOptionsToRewrite` returns null, `matches()`
is false and `inferTableOptions` is never called. The leaf one relies on
`EmptyTableRoutingManager#routingExists` returning `true` unconditionally,
which no real routing manager does when the routing entry is absent — with a
faithful stub it would hit this site first.
Suggested fix, ideally both halves:
1. `inferTableOptions`: `catch (IllegalStateException | QueryException e) {
return null; }`, so the degrade-quietly contract stays explicit for the new
exception type.
2. `optimize()`: add `catch (QueryException e) { throw e; }` so a typed
error raised inside a planner rule is not relabelled `QUERY_PLANNING`. That one
is a latent bug beyond this PR, but this PR is what makes it reachable.
And a third test — `inferPartitionHint=true`, no hint — asserting
`BROKER_RESOURCE_MISSING` comes out of `planQuery()`.
--
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]
