rkakaTS commented on issue #17407:
URL: https://github.com/apache/druid/issues/17407#issuecomment-5819134943
@JRobTS and I assessed the change that introduced this failure. Here are our
findings
Confirming this on 37.0.0 and current `master` (38.0.0-SNAPSHOT), and adding
what I think is the
most useful missing context: **this is a regression, and I've bisected it to
33.0.0.**
## Bisect
The check in `NativeQueryMaker#runQuery` is byte-identical across all of
these releases. What
changed is the helper it calls:
| Release | Helper |
|---|---|
| 29.0.1 – 32.0.1 | `getBaseQuerySegmentSpec()` |
| **33.0.0** | **`getEffectiveQuerySegmentSpec()`** |
| 37.0.0 – master | `ExecutionVertex#getEffectiveQuerySegmentSpec()` |
First broken release: **33.0.0**. Introduced by 592d2e05b3 ("Fix
JoinDataSource to not ignore
DataSource layers silently", #17726, 21 Feb 2025).
## What the change did
**29.0.1 through 32.0.1:**
```java
private List<Interval> findBaseDataSourceIntervals(Query<?> query)
{
return query.getDataSource().getAnalysis()
.getBaseQuerySegmentSpec()
.map(QuerySegmentSpec::getIntervals)
.orElseGet(query::getIntervals);
}
```
`DataSourceAnalysis#getBaseQuerySegmentSpec()` carried this javadoc:
> Returns the `QuerySegmentSpec` that is associated with the base
datasource, if any. This only
> happens when there is an outer query datasource. In this case, the base
querySegmentSpec is the
> one associated with the **innermost subquery**.
So the check preferred the innermost subquery's interval, falling back to
the outer query only when
there wasn't one. A join whose inputs each carried a `__time` filter
resolved to a real interval and
passed.
**33.0.0:**
```java
private List<Interval> findBaseDataSourceIntervals(Query<?> query)
{
return query.getDataSourceAnalysis()
.getEffectiveQuerySegmentSpec()
.getIntervals();
}
```
`getEffectiveQuerySegmentSpec()` resolves to the *outermost* spec — no
`Optional`, no fallback. For
a join the outer spec is `ONLY_ETERNITY`, so the query is rejected
regardless of what its inputs
filter on.
That inversion, innermost-preferred to outermost-only, is the regression. It
looks like collateral
from the `JoinDataSource` refactor rather than an intentional change to this
check — #17726 doesn't
mention `requireTimeCondition`.
## Current state
#17726's commit message anticipated a follow-up separating the in-join model
from the
vertex-boundary model. That follow-up is #17763 (`ExecutionVertex`), which
shipped in 37.0.0 and is
the only commit touching `NativeQueryMaker.java` between 33.0.0 and
`master`. It preserves the
semantics:
```java
// ExecutionVertex.ExecutionVertexExplorer
protected Query<?> visitQuery(Query<?> query)
{
if (querySegmentSpec == null && isLeftLeaning()) { // first match wins
querySegmentSpec = getQuerySegmentSpec(query);
}
return query;
}
private boolean isLeftLeaning()
{
for (EVNode evNode : parents) {
if (evNode.index != null && evNode.index != 0) {
return false;
}
}
return true;
}
```
Two properties combine to produce the reported behaviour:
1. **First match wins**, and traversal starts at the outermost query — an
outer `ONLY_ETERNITY` spec
is latched immediately and nothing deeper is examined.
2. **The right side of a join is never visited** — `isLeftLeaning()` returns
`false` for any parent
with a non-zero index.
`git diff druid-37.0.0 master --
sql/src/main/java/org/apache/druid/sql/calcite/run/NativeQueryMaker.java`
is empty, so 37.0.0 and `master` behave identically here.
## Note for anyone attempting a fix
The current behaviour is asserted by two tests in
`sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java`:
- `testRequireTimeConditionSubQueryNegative`
- `testRequireTimeConditionSemiJoinNegative`
Both assert the query **is** rejected, so a fix has to change them
deliberately. Worth knowing up
front — this isn't simply missing coverage.
## Possible direction
The narrowest change preserving the guardrail's intent would be to descend
into subquery inputs when
the outer spec resolves to `ONLY_ETERNITY` rather than latching it — roughly
restoring the pre-33.0.0
precedence within the `ExecutionVertex` traversal.
Whether every leaf of a join should be required to carry a time filter is a
separate design
question. Requiring all of them would be stricter than pre-33.0.0 behaviour
but arguably closer to
the setting's stated purpose; restoring the old precedence would be a
smaller, more obviously safe
change.
--
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]
