barry3406 opened a new pull request, #19298:
URL: https://github.com/apache/druid/pull/19298
Fixes #18665.
### Description
`INTERVAL '1' WEEK` (and the unquoted `INTERVAL 1 WEEK`) was resolving to
one hour instead of seven days. The repro from #18665 makes the symptom obvious:
```sql
SELECT
MILLIS_TO_TIMESTAMP(0) + INTERVAL 1 WEEK,
MILLIS_TO_TIMESTAMP(0) + INTERVAL 2 WEEK
```
was returning `1970-01-01T01:00:00`, `1970-01-01T02:00:00` (one and two
hours) instead of the expected `1970-01-08`, `1970-01-15`.
#### Root cause
The bug lives in Calcite 1.37.0.
`SqlIntervalQualifier.evaluateIntervalLiteralAsWeek` packages a WEEK literal
into a year-month shaped int array (`[sign, 0, week]`), even though
`SqlIntervalQualifier.typeName()` reports a WEEK qualifier as `INTERVAL_DAY`.
The downstream `SqlParserUtil.intervalToMillis` loop walks `ret[1..]` as `[day,
hour, minute, second, ms]`, so the week count lands in the hour slot. `INTERVAL
'1' WEEK` therefore becomes `1 * 3_600_000 ms = 1 hour`.
This was fixed upstream as
[CALCITE-6391](https://issues.apache.org/jira/browse/CALCITE-6391) in Calcite
1.38, where `evaluateIntervalLiteralAsWeek` now multiplies by seven and writes
into the day-time shaped array. Druid is still on Calcite 1.37, so we work
around it inside the planner instead of upgrading Calcite in this PR.
#### Fixed the bug ...
Added `SqlIntervalWeekRewriteShuttle`, an `SqlShuttle` that walks the parsed
`SqlNode` tree before validation and rewrites the two parsed forms of `WEEK`
intervals into equivalent `DAY` intervals:
- `INTERVAL '1' WEEK` parses to a `SqlIntervalLiteral` with a WEEK
qualifier. The shuttle replaces it with a `SqlIntervalLiteral` whose value has
been multiplied by seven and whose qualifier is now DAY.
- `INTERVAL 1 WEEK` parses to
`SqlStdOperatorTable.INTERVAL.createCall(pos, n, qualifier)`. The shuttle
rewrites the call so the numeric operand becomes `MULTIPLY(n, 7)` and the
qualifier becomes DAY.
`WEEK(SUNDAY)..WEEK(SATURDAY)` and `ISOWEEK` qualifiers carry a non-null
`timeFrameName` and are handled differently in Calcite, so the shuttle leaves
them alone and only touches the bare `WEEK` form that is broken in 1.37.
The shuttle is invoked in `DruidPlanner.validate` right before
`rewriteParameters`, alongside the existing `SqlParameterizerShuttle` pass. It
runs once per validate call and is a no-op for any tree that does not contain a
WEEK interval.
#### Tests
- `SqlIntervalWeekRewriteShuttleTest` exercises the shuttle directly
against constructed `SqlIntervalLiteral` and `SqlCall` nodes for WEEK, DAY and
MONTH (positive, negative, multi-week), and against parsed queries that use
both quoted and unquoted week intervals.
- `CalciteQueryTest#testIntervalWeekResolution` is the end-to-end
regression test from #18665. Without the fix it produces an interval narrowing
on `2000-01-01T01:00:00.001` / `2000-01-01T02:00:00.001` (the bug); with the
fix it correctly narrows to `2000-01-08` and `2000-01-15`.
I verified the bug is reachable by temporarily disabling the shuttle and
watching the regression test fail with exactly the actual-vs-expected from the
issue. Re-enabling the shuttle makes it pass.
I also re-ran the surrounding test classes to make sure the rewrite did not
regress any existing INTERVAL behavior:
- `CalciteQueryTest` (440 tests)
- `CalciteSelectQueryTest` (67 tests)
- `CalciteJoinQueryTest` (594 tests)
- `CalciteSubqueryTest` (54 tests)
- `CalciteParameterQueryTest` (20 tests)
- `CalciteInsertDmlTest` / `CalciteReplaceDmlTest` (107 tests)
- `ExpressionsTest` (54 tests)
- `DruidSqlParserUtilsTest` (57 tests)
- `SqlQuidemTest` (10 tests)
All green. `mvn -pl sql -am verify -DskipTests=true` (checkstyle,
forbiddenapis, animal-sniffer) and `mvn -pl sql -P rat verify -DskipTests=true`
(license headers) also pass.
#### Release note
Fixed an issue where `INTERVAL n WEEK` in SQL queries resolved to `n` hours
instead of `n * 7` days. `MILLIS_TO_TIMESTAMP(0) + INTERVAL '1' WEEK` now
correctly returns `1970-01-08`, matching the SQL standard and other databases.
<hr>
##### Key changed/added classes in this PR
* `SqlIntervalWeekRewriteShuttle` (new)
* `DruidPlanner` (calls the shuttle in `validate`)
* `SqlIntervalWeekRewriteShuttleTest` (new)
* `CalciteQueryTest` (regression test `testIntervalWeekResolution`)
<hr>
This PR has:
- [x] been self-reviewed.
- [x] added unit tests or modified existing tests to cover new code paths.
- [x] added comments explaining the "why" and the intent of the code
wherever would not be obvious for an unfamiliar reader.
- [x] a release note entry in the PR description.
--
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]
