NishthaShah opened a new pull request, #20441:
URL: https://github.com/apache/druid/pull/20441
Fixes #17407.
### Description
`druid.sql.planner.requireTimeCondition` has two defects in opposite
directions, both stemming from how `NativeQueryMaker#runQuery` resolves "the
intervals that apply to this query":
* **Pre-33**: joins of two bounded subqueries were rejected.
`DataSourceAnalysis#getBaseQuerySegmentSpec` resolved the outer
`JoinDataSource` query first, whose interval is `ETERNITY`. That is the shape
reported in #17407.
* **v33+**: non-collapsible subquery stacks are now rejected. After #17726 /
#17763 the check uses `ExecutionVertex#getEffectiveQuerySegmentSpec`, whose
traversal is pruned at `Query#mayCollapseQueryDataSource` (default `false`). So
under a `QueryDataSource`, the inner query is never visited and the outer
`ETERNITY` spec is what the check sees.
A minimal repro rejected on master today, although its only table access is
bounded:
```sql
SELECT COUNT(*) FROM (
SELECT dim1 FROM druid.foo WHERE __time >= '2000-01-01' LIMIT 10
)
```
#### Fix: resolve requireTimeCondition outside `ExecutionVertex`
`ExecutionVertex#getEffectiveQuerySegmentSpec` is load-bearing for execution
— it feeds segment selection in `CachingClusteredClient`, walker lookup in
`BaseQuery`, and interval handling in `Queries` and `BrokerQueryResource`.
Changing it would change which segments get scanned. `requireTimeCondition` is
asking a different, purely-validation question ("is there a `__time` filter
anywhere that bounds the base scan?") which is not bounded by the execution
vertex.
A new `RequireTimeConditionAnalyzer` (package
`org.apache.druid.query.planning`) walks the query tree via
`ExecutionVertexShuttle` and returns `true` iff every physical leaf datasource
is descended from an ancestor query with a bounded `QuerySegmentSpec` (i.e. not
`Intervals.ONLY_ETERNITY`). Global datasources (lookups, inline) short-circuit
as satisfied.
Key invariants preserved by the walker:
1. Descends into every `QueryDataSource` with no
`mayCollapseQueryDataSource` gate — covers non-collapsible subquery stacks.
2. Descends the left input of every `JoinDataSource` but not the right — a
`__time` filter on the right side does not bound the base table. Preserves
`testRequireTimeConditionSemiJoinNegative`.
3. Keys on query intervals, not filters — a `__time` predicate over a
subquery result stays a residual filter and never becomes a `QuerySegmentSpec`,
so it must not satisfy the guardrail
(`testRequireTimeConditionOuterFilterOverUnboundedSubqueryNegative`).
#### Scope
`NativeQueryMaker` is the only enforcement site touched.
`MSQTaskQueryMaker`, `DartQueryMaker`, and `PrePlannedDartQueryMaker` do not
currently check `requireTimeCondition` at all, so the flag is silently a no-op
on those paths. Extending the check to those makers is out of scope for this PR.
#### Release note
Fixed a bug in `druid.sql.planner.requireTimeCondition` where queries with a
`__time` filter on every physical leg were rejected as if they had no filter.
Affected shapes include joins of two bounded subqueries (pre-33 regression),
non-collapsible subquery stacks over bounded scans (33+ regression), and `UNION
ALL` of two bounded queries.
<hr>
##### Key changed/added classes in this PR
* `RequireTimeConditionAnalyzer` (new)
* `NativeQueryMaker`
* `CalciteQueryTest`
<hr>
This PR has:
- [x] been self-reviewed.
- [x] added unit tests covering new code paths (nine new tests in
`CalciteQueryTest`, spanning explicit joins, three-way joins, `UNION ALL`,
lookups on the right side of a join, non-collapsible subqueries, and outer
filters over unbounded subqueries; all four pre-existing positive and three
pre-existing negative `requireTimeCondition` tests continue to pass).
- [x] added comments explaining the "why" and the intent of the code
wherever would not be obvious for an unfamiliar reader.
--
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]
