yashmayya opened a new pull request, #19373:
URL: https://github.com/apache/pinot/pull/19373
`EXPLAIN PLAN FOR <query>` can return a plan for a query that cannot
actually run. The
following succeeds against `ColocatedJoinEngineQuickStart`:
```sql
EXPLAIN PLAN FOR
SELECT * FROM userAttributes /*+ tableOptions(partition_function='hashcode',
partition_key='daysSinceFirstTrip', partition_size='2') */ a
JOIN userGroups /*+ tableOptions(partition_function='hashcode',
partition_key='userUUID', partition_size='2') */ g
ON a.userUUID = g.userUUID
```
but running the same query fails with:
```
Partition key: daysSinceFirstTrip does not match partition column: userUUID
for table: userAttributes_OFFLINE
```
## Root cause
`CompiledQuery#explain` renders a plain logical EXPLAIN straight off the
optimized `RelNode`
tree and never calls `toDispatchableSubPlan`. Several classes of planning
error are only raised
further down that path:
* `PinotLogicalQueryPlanner#makePlan` (`RelNode` -> `PlanNode` conversion),
e.g. lookup-join
shape validation and `ArrayToMv` validation.
* `WorkerManager` worker assignment, e.g. `checkPartitionInfoMap`, which is
where a
`tableOptions` partition hint is compared against the table's real
partitioning.
So the logical EXPLAIN reports a plan the engine would refuse to execute.
The other two explain
branches (`EXPLAIN IMPLEMENTATION PLAN`, and `EXPLAIN PLAN WITH
IMPLEMENTATION` when
`explainAskingServers` is on) already build the dispatchable subplan and
therefore already fail
correctly — this only affects the default logical EXPLAIN.
## Fix
Build the dispatchable subplan in the logical branch too, and keep rendering
the logical plan.
Parity becomes structural instead of a list of checks that has to be kept in
sync, and all three
explain branches now go through the same pipeline.
## This was not a single bad hint
`ResourceBasedQueryPlansTest#testQueryExplainPlansWithExceptions` was
calling `explainQuery(query)`
**and then** `planQuery(query minus the EXPLAIN prefix)` inside one try
block, so its
`expectedException` fixtures were being satisfied by the second call. That
made the existing
fixtures look like EXPLAIN coverage when they were really execution coverage.
Dropping that second call turns the fixtures into genuine EXPLAIN
assertions. With this fix all
590 cases in that suite pass; without it 9 fail, spanning three unrelated
causes:
* 6 x partition-hint mismatch (`Partition key: ... does not match partition
column: ...`,
`Partition size mismatch`, `Partition function mismatch`, `Failed to find
table partition info`)
* 2 x `'ArrayToMv' is not supported`
* 1 x `Right input for lookup join must be an identifier`
## Behavior change
EXPLAIN now surfaces any planning error that execution would hit, including
ones that depend on
live routing state (for example a table whose partitions have no fully
replicated server right
now). This is the intended semantics — if the query cannot run, EXPLAIN
should say so rather than
print a plan — but it does mean EXPLAIN can start failing where it
previously returned a plan.
## Tests
* `ResourceBasedQueryPlansTest`: removed the `planQuery` call from the
exception path, so the 34
existing `expectedException` fixtures now actually assert on EXPLAIN. 9 of
them are regression
coverage for this fix.
* `QueryCompilationTest#testJoinPushTransitivePredicateLookupJoin` was
asserting a plan for a
query that cannot execute: with `SELECT *` the lookup join's right input
is a bare table scan,
and `RelToPlanNodeConverter#convertLogicalJoin` requires an
identifier-only `Project` over a
`TableScan`. Switched to selecting explicit columns, which is the shape
every other lookup-join
fixture uses. The test still checks what it was written to check — the
transitive predicate is
pushed only to the left side under the lookup hint.
--
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]
