From 094b861710864fdbd32ac32f18ffc4d8a901a787 Mon Sep 17 00:00:00 2001 From: barry3406 Date: Fri, 10 Apr 2026 23:15:39 -0700 Subject: [PATCH 1/2] fix: INTERVAL n WEEK in SQL resolves to seven days instead of one hour Calcite 1.37.0's SqlIntervalQualifier.evaluateIntervalLiteralAsWeek packages a WEEK literal as a year-month shaped int array ([sign, 0, week]) but reports the type as INTERVAL_DAY, so the downstream SqlParserUtil.intervalToMillis loop interprets the week value as the hour component. The result is that `MILLIS_TO_TIMESTAMP(0) + INTERVAL '1' WEEK` resolves to 1970-01-01T01:00:00 instead of 1970-01-08, as reported in #18665. The bug was fixed upstream in CALCITE-6391 / Calcite 1.38, but Druid is still on 1.37, so this commit works around it inside Druid's planner. A new SqlIntervalWeekRewriteShuttle walks the parsed SqlNode tree in DruidPlanner.validate and rewrites both forms of week intervals before they reach the buggy code path: - `INTERVAL '1' WEEK` (a SqlIntervalLiteral) is replaced with an equivalent SqlIntervalLiteral whose numeric value has been multiplied by seven and whose qualifier is now DAY. - `INTERVAL 1 WEEK` (a SqlCall using SqlStdOperatorTable.INTERVAL) is rewritten 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 left untouched, so this only affects the bare WEEK form that is broken in Calcite 1.37. Tests: - SqlIntervalWeekRewriteShuttleTest exercises the shuttle directly against constructed SqlIntervalLiteral and SqlCall nodes for WEEK, DAY and MONTH, and against parsed queries that use both quoted and unquoted week intervals. - CalciteQueryTest#testIntervalWeekResolution is the regression test from #18665. Without the fix it produces interval narrowing on 2000-01-01T01:00:00 / 2000-01-01T02:00:00; with the fix it correctly narrows to 2000-01-08 and 2000-01-15. --- .../sql/calcite/planner/DruidPlanner.java | 7 +- .../SqlIntervalWeekRewriteShuttle.java | 146 ++++++++++++ .../druid/sql/calcite/CalciteQueryTest.java | 36 +++ .../SqlIntervalWeekRewriteShuttleTest.java | 215 ++++++++++++++++++ 4 files changed, 403 insertions(+), 1 deletion(-) create mode 100644 sql/src/main/java/org/apache/druid/sql/calcite/planner/SqlIntervalWeekRewriteShuttle.java create mode 100644 sql/src/test/java/org/apache/druid/sql/calcite/planner/SqlIntervalWeekRewriteShuttleTest.java diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java index f361e43797b4..169171bc2027 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java @@ -128,7 +128,12 @@ public void validate() // Validate query context. engine.validateContext(plannerContext.queryContextMap()); planner.skipParse(); - final SqlNode root = rewriteParameters(plannerContext.getSqlNode()); + final SqlNode parsed = plannerContext.getSqlNode(); + // Work around CALCITE-6391 (fixed in Calcite 1.38) by rewriting any + // INTERVAL ... WEEK literals before they reach Calcite's buggy + // SqlIntervalQualifier.evaluateIntervalLiteralAsWeek path. + final SqlNode weekRewritten = parsed.accept(new SqlIntervalWeekRewriteShuttle()); + final SqlNode root = rewriteParameters(weekRewritten == null ? parsed : weekRewritten); hook.captureSqlNode(root); handler = createHandler(root); handler.validate(); diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/SqlIntervalWeekRewriteShuttle.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/SqlIntervalWeekRewriteShuttle.java new file mode 100644 index 000000000000..2d957706da4d --- /dev/null +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/SqlIntervalWeekRewriteShuttle.java @@ -0,0 +1,146 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.sql.calcite.planner; + +import org.apache.calcite.avatica.util.TimeUnit; +import org.apache.calcite.avatica.util.TimeUnitRange; +import org.apache.calcite.sql.SqlCall; +import org.apache.calcite.sql.SqlIntervalLiteral; +import org.apache.calcite.sql.SqlIntervalQualifier; +import org.apache.calcite.sql.SqlLiteral; +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import org.apache.calcite.sql.parser.SqlParserPos; +import org.apache.calcite.sql.util.SqlShuttle; + +import java.math.BigDecimal; + +/** + * Workaround for a Calcite 1.37.0 bug in which {@code INTERVAL n WEEK} is + * incorrectly converted to {@code n} hours instead of {@code n * 7} days. + * + *

The bug is in {@code SqlIntervalQualifier.evaluateIntervalLiteralAsWeek}, + * which packages the WEEK value into a year-month shaped int array + * ({@code [sign, 0, week]}) even though {@code SqlIntervalQualifier.typeName()} + * reports WEEK intervals as {@link org.apache.calcite.sql.type.SqlTypeName#INTERVAL_DAY}. + * Downstream, {@code SqlParserUtil.intervalToMillis} interprets that array as + * {@code [sign, day, hour, minute, second, ms]}, so the week value lands in + * the hour slot. The bug was fixed upstream in Calcite 1.38.0 + * (CALCITE-6391), + * but Druid is still on Calcite 1.37.0, so we work around it by rewriting any + * {@code WEEK} interval in the parsed {@link SqlNode} tree to an equivalent + * {@code DAY} interval whose value has been multiplied by seven. + * + *

Two parsed forms reach this shuttle: + *

+ * + *

This rewrite preserves the semantics of {@code TimeUnitRange.WEEK} while + * avoiding the buggy code path inside Calcite. Druid still treats WEEK as a + * day-time (millisecond) interval in arithmetic, which matches the expectation + * documented in the issue: {@code INTERVAL '1' WEEK} should add seven days. + */ +public class SqlIntervalWeekRewriteShuttle extends SqlShuttle +{ + private static final BigDecimal SEVEN = BigDecimal.valueOf(7); + + @Override + public SqlNode visit(SqlLiteral literal) + { + if (literal instanceof SqlIntervalLiteral) { + final SqlIntervalLiteral intervalLiteral = (SqlIntervalLiteral) literal; + final SqlIntervalLiteral.IntervalValue value = + (SqlIntervalLiteral.IntervalValue) intervalLiteral.getValue(); + if (value != null && isPlainWeek(value.getIntervalQualifier())) { + final String multiplied = multiplyByWeek(value.getIntervalLiteral()); + if (multiplied != null) { + return SqlLiteral.createInterval( + value.getSign(), + multiplied, + new SqlIntervalQualifier(TimeUnit.DAY, null, value.getIntervalQualifier().getParserPosition()), + literal.getParserPosition() + ); + } + } + } + return literal; + } + + @Override + public SqlNode visit(SqlCall call) + { + if (call.getOperator() == SqlStdOperatorTable.INTERVAL && call.operandCount() == 2) { + final SqlNode qualifierNode = call.operand(1); + if (qualifierNode instanceof SqlIntervalQualifier + && isPlainWeek((SqlIntervalQualifier) qualifierNode)) { + // First, recurse into the numeric operand so any nested rewrites still happen. + final SqlNode rewrittenNumeric = call.operand(0).accept(this); + final SqlNode numeric = rewrittenNumeric == null ? call.operand(0) : rewrittenNumeric; + final SqlParserPos pos = call.getParserPosition(); + final SqlNode multipliedNumeric = SqlStdOperatorTable.MULTIPLY.createCall( + pos, + numeric, + SqlLiteral.createExactNumeric("7", pos) + ); + final SqlIntervalQualifier dayQualifier = + new SqlIntervalQualifier(TimeUnit.DAY, null, qualifierNode.getParserPosition()); + return SqlStdOperatorTable.INTERVAL.createCall(pos, multipliedNumeric, dayQualifier); + } + } + return super.visit(call); + } + + private static boolean isPlainWeek(SqlIntervalQualifier qualifier) + { + // Only the bare WEEK qualifier is affected. WEEK(SUNDAY)..WEEK(SATURDAY) and + // ISOWEEK use a non-null timeFrameName and are handled differently in Calcite. + return qualifier != null + && qualifier.timeUnitRange == TimeUnitRange.WEEK + && qualifier.timeFrameName == null; + } + + /** + * Multiplies the integer string value of a WEEK interval literal by seven. + * Returns null if the value is not a plain non-negative integer literal, + * which means it does not match Calcite's WEEK interval grammar and would + * fail validation anyway. + */ + private static String multiplyByWeek(String intervalStr) + { + if (intervalStr == null || intervalStr.isEmpty()) { + return null; + } + for (int i = 0; i < intervalStr.length(); i++) { + final char c = intervalStr.charAt(i); + if (c < '0' || c > '9') { + return null; + } + } + return new BigDecimal(intervalStr).multiply(SEVEN).toString(); + } +} diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java index fcb944852453..f7b9540434c8 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java @@ -6647,6 +6647,42 @@ public void testCountStarWithTwoPointsInTime() ); } + /** + * Regression test for #18665: + * {@code INTERVAL '1' WEEK} previously folded to one hour instead of seven days because + * Calcite 1.37.0 mishandles the WEEK qualifier. The interval narrowing below proves the + * fix lands the equality on {@code 2000-01-08} (one week after {@code 2000-01-01}) + * rather than {@code 2000-01-01T01:00:00}. + */ + @Test + public void testIntervalWeekResolution() + { + testQuery( + "SELECT COUNT(*) FROM druid.foo WHERE " + + "__time = TIMESTAMP '2000-01-01 00:00:00' OR " + + "__time = TIMESTAMP '2000-01-01 00:00:00' + INTERVAL '1' WEEK OR " + + "__time = TIMESTAMP '2000-01-01 00:00:00' + INTERVAL 2 WEEK", + ImmutableList.of( + Druids.newTimeseriesQueryBuilder() + .dataSource(CalciteTests.DATASOURCE1) + .intervals( + querySegmentSpec( + Intervals.of("2000-01-01/2000-01-01T00:00:00.001"), + Intervals.of("2000-01-08/2000-01-08T00:00:00.001"), + Intervals.of("2000-01-15/2000-01-15T00:00:00.001") + ) + ) + .granularity(Granularities.ALL) + .aggregators(aggregators(new CountAggregatorFactory("a0"))) + .context(QUERY_CONTEXT_DEFAULT) + .build() + ), + ImmutableList.of( + new Object[]{1L} + ) + ); + } + @Test public void testCountStarWithComplexDisjointTimeFilter() { diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/planner/SqlIntervalWeekRewriteShuttleTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/planner/SqlIntervalWeekRewriteShuttleTest.java new file mode 100644 index 000000000000..f456d811d366 --- /dev/null +++ b/sql/src/test/java/org/apache/druid/sql/calcite/planner/SqlIntervalWeekRewriteShuttleTest.java @@ -0,0 +1,215 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.sql.calcite.planner; + +import org.apache.calcite.avatica.util.TimeUnit; +import org.apache.calcite.avatica.util.TimeUnitRange; +import org.apache.calcite.sql.SqlBasicCall; +import org.apache.calcite.sql.SqlIntervalLiteral; +import org.apache.calcite.sql.SqlIntervalQualifier; +import org.apache.calcite.sql.SqlLiteral; +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import org.apache.calcite.sql.parser.SqlParser; +import org.apache.calcite.sql.parser.SqlParserPos; +import org.apache.druid.sql.calcite.util.CalciteTestBase; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Verifies that {@link SqlIntervalWeekRewriteShuttle} rewrites + * {@code INTERVAL ... WEEK} into the equivalent {@code INTERVAL ... DAY} + * with the value multiplied by seven, working around CALCITE-6391. + */ +public class SqlIntervalWeekRewriteShuttleTest extends CalciteTestBase +{ + private final SqlIntervalWeekRewriteShuttle shuttle = new SqlIntervalWeekRewriteShuttle(); + + @Test + public void testQuotedSingleWeekLiteralBecomesSevenDays() + { + final SqlNode original = SqlLiteral.createInterval( + 1, + "1", + new SqlIntervalQualifier(TimeUnit.WEEK, null, SqlParserPos.ZERO), + SqlParserPos.ZERO + ); + + final SqlNode rewritten = original.accept(shuttle); + assertNotSame(original, rewritten); + assertTrue(rewritten instanceof SqlIntervalLiteral); + final SqlIntervalLiteral.IntervalValue value = + (SqlIntervalLiteral.IntervalValue) ((SqlIntervalLiteral) rewritten).getValue(); + assertEquals("7", value.getIntervalLiteral()); + assertEquals(TimeUnitRange.DAY, value.getIntervalQualifier().timeUnitRange); + assertEquals(1, value.getSign()); + } + + @Test + public void testQuotedMultiWeekLiteralIsMultipliedBySeven() + { + final SqlNode original = SqlLiteral.createInterval( + 1, + "3", + new SqlIntervalQualifier(TimeUnit.WEEK, null, SqlParserPos.ZERO), + SqlParserPos.ZERO + ); + + final SqlNode rewritten = original.accept(shuttle); + final SqlIntervalLiteral.IntervalValue value = + (SqlIntervalLiteral.IntervalValue) ((SqlIntervalLiteral) rewritten).getValue(); + assertEquals("21", value.getIntervalLiteral()); + assertEquals(TimeUnitRange.DAY, value.getIntervalQualifier().timeUnitRange); + } + + @Test + public void testNegativeQuotedWeekLiteralPreservesSign() + { + final SqlNode original = SqlLiteral.createInterval( + -1, + "2", + new SqlIntervalQualifier(TimeUnit.WEEK, null, SqlParserPos.ZERO), + SqlParserPos.ZERO + ); + + final SqlNode rewritten = original.accept(shuttle); + final SqlIntervalLiteral.IntervalValue value = + (SqlIntervalLiteral.IntervalValue) ((SqlIntervalLiteral) rewritten).getValue(); + assertEquals("14", value.getIntervalLiteral()); + assertEquals(TimeUnitRange.DAY, value.getIntervalQualifier().timeUnitRange); + assertEquals(-1, value.getSign()); + } + + @Test + public void testDayLiteralIsLeftUntouched() + { + final SqlNode original = SqlLiteral.createInterval( + 1, + "1", + new SqlIntervalQualifier(TimeUnit.DAY, null, SqlParserPos.ZERO), + SqlParserPos.ZERO + ); + + final SqlNode rewritten = original.accept(shuttle); + assertSame(original, rewritten); + } + + @Test + public void testMonthLiteralIsLeftUntouched() + { + final SqlNode original = SqlLiteral.createInterval( + 1, + "5", + new SqlIntervalQualifier(TimeUnit.MONTH, null, SqlParserPos.ZERO), + SqlParserPos.ZERO + ); + + final SqlNode rewritten = original.accept(shuttle); + assertSame(original, rewritten); + } + + @Test + public void testUnquotedWeekIntervalCallBecomesMultiplyDayCall() + { + // Mirrors what Druid's parser produces for `INTERVAL 1 WEEK`: + // SqlStdOperatorTable.INTERVAL.createCall(pos, n, qualifier) + final SqlNode original = SqlStdOperatorTable.INTERVAL.createCall( + SqlParserPos.ZERO, + SqlLiteral.createExactNumeric("1", SqlParserPos.ZERO), + new SqlIntervalQualifier(TimeUnit.WEEK, null, SqlParserPos.ZERO) + ); + + final SqlNode rewritten = original.accept(shuttle); + assertNotSame(original, rewritten); + assertTrue(rewritten instanceof SqlBasicCall); + final SqlBasicCall rewrittenCall = (SqlBasicCall) rewritten; + assertSame(SqlStdOperatorTable.INTERVAL, rewrittenCall.getOperator()); + + // Operand 0 should be (numeric * 7). + final SqlNode numeric = rewrittenCall.operand(0); + assertTrue(numeric instanceof SqlBasicCall); + final SqlBasicCall numericCall = (SqlBasicCall) numeric; + assertSame(SqlStdOperatorTable.MULTIPLY, numericCall.getOperator()); + assertEquals(2, numericCall.operandCount()); + assertEquals("7", ((SqlLiteral) numericCall.operand(1)).toValue()); + + // Operand 1 should now be a DAY qualifier. + final SqlNode qualifier = rewrittenCall.operand(1); + assertTrue(qualifier instanceof SqlIntervalQualifier); + assertEquals(TimeUnitRange.DAY, ((SqlIntervalQualifier) qualifier).timeUnitRange); + } + + @Test + public void testUnquotedDayIntervalCallIsLeftUntouched() + { + final SqlNode original = SqlStdOperatorTable.INTERVAL.createCall( + SqlParserPos.ZERO, + SqlLiteral.createExactNumeric("1", SqlParserPos.ZERO), + new SqlIntervalQualifier(TimeUnit.DAY, null, SqlParserPos.ZERO) + ); + + final SqlNode rewritten = original.accept(shuttle); + assertSame(original, rewritten); + } + + @Test + public void testParsedWeekIntervalIsRewritten() throws Exception + { + // End-to-end check using the actual parser, both forms. + assertParsedWeekIsRewritten("SELECT TIMESTAMP '1970-01-01 00:00:00' + INTERVAL '1' WEEK"); + assertParsedWeekIsRewritten("SELECT TIMESTAMP '1970-01-01 00:00:00' + INTERVAL 1 WEEK"); + assertParsedWeekIsRewritten("SELECT TIMESTAMP '1970-01-01 00:00:00' + INTERVAL 2 WEEK"); + } + + private void assertParsedWeekIsRewritten(String sql) throws Exception + { + final SqlNode parsed = SqlParser.create(sql).parseQuery(); + final SqlNode rewritten = parsed.accept(shuttle); + final WeekFinder finder = new WeekFinder(); + rewritten.accept(finder); + assertTrue( + !finder.found, + "Rewritten tree for [" + sql + "] should not contain a WEEK qualifier, but it did." + ); + } + + /** + * SqlShuttle that records whether it visited any plain WEEK + * {@link SqlIntervalQualifier}. + */ + private static class WeekFinder extends org.apache.calcite.sql.util.SqlShuttle + { + boolean found; + + @Override + public SqlNode visit(SqlIntervalQualifier intervalQualifier) + { + if (intervalQualifier.timeUnitRange == TimeUnitRange.WEEK + && intervalQualifier.timeFrameName == null) { + found = true; + } + return intervalQualifier; + } + } +} From 09b625056b2011a0588afabc50d4409ed7d21274 Mon Sep 17 00:00:00 2001 From: barry3406 Date: Thu, 16 Apr 2026 11:35:50 -0700 Subject: [PATCH 2/2] Also handle QUARTER interval in shuttle (per gianm review) INTERVAL n QUARTER is broken in Calcite 1.37 in the same way as WEEK: SqlIntervalQualifier.evaluateIntervalLiteralAsQuarter packages the value into a [sign, 0, quarter] year-month array without multiplying by three, so SqlParserUtil.intervalToMonths reads `quarter` months instead of `3 * quarter` months. Extend SqlIntervalWeekRewriteShuttle to rewrite QUARTER -> MONTH * 3 alongside the existing WEEK -> DAY * 7 rewrite, covering both the quoted (SqlIntervalLiteral) and unquoted (SqlCall) parsed forms. Also update the CALCITE ticket reference from CALCITE-6391 to the correct CALCITE-6581, which is the upstream fix gianm wrote for both units. --- .../sql/calcite/planner/DruidPlanner.java | 7 +- .../SqlIntervalWeekRewriteShuttle.java | 153 ++++++++++++------ .../druid/sql/calcite/CalciteQueryTest.java | 37 +++++ .../SqlIntervalWeekRewriteShuttleTest.java | 125 ++++++++++++-- 4 files changed, 259 insertions(+), 63 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java index 169171bc2027..317246ed456d 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java @@ -129,9 +129,10 @@ public void validate() engine.validateContext(plannerContext.queryContextMap()); planner.skipParse(); final SqlNode parsed = plannerContext.getSqlNode(); - // Work around CALCITE-6391 (fixed in Calcite 1.38) by rewriting any - // INTERVAL ... WEEK literals before they reach Calcite's buggy - // SqlIntervalQualifier.evaluateIntervalLiteralAsWeek path. + // Work around CALCITE-6581 (fixed in Calcite 1.38) by rewriting any + // INTERVAL ... WEEK and INTERVAL ... QUARTER literals before they reach + // Calcite's buggy SqlIntervalQualifier.evaluateIntervalLiteralAs{Week,Quarter} + // paths. final SqlNode weekRewritten = parsed.accept(new SqlIntervalWeekRewriteShuttle()); final SqlNode root = rewriteParameters(weekRewritten == null ? parsed : weekRewritten); hook.captureSqlNode(root); diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/SqlIntervalWeekRewriteShuttle.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/SqlIntervalWeekRewriteShuttle.java index 2d957706da4d..ed5d75233097 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/SqlIntervalWeekRewriteShuttle.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/SqlIntervalWeekRewriteShuttle.java @@ -33,41 +33,61 @@ import java.math.BigDecimal; /** - * Workaround for a Calcite 1.37.0 bug in which {@code INTERVAL n WEEK} is - * incorrectly converted to {@code n} hours instead of {@code n * 7} days. + * Workaround for Calcite 1.37.0 bugs in which {@code INTERVAL n WEEK} is + * incorrectly converted to {@code n} hours instead of {@code n * 7} days, + * and {@code INTERVAL n QUARTER} is incorrectly converted to {@code n} months + * instead of {@code n * 3} months. * - *

The bug is in {@code SqlIntervalQualifier.evaluateIntervalLiteralAsWeek}, - * which packages the WEEK value into a year-month shaped int array - * ({@code [sign, 0, week]}) even though {@code SqlIntervalQualifier.typeName()} - * reports WEEK intervals as {@link org.apache.calcite.sql.type.SqlTypeName#INTERVAL_DAY}. - * Downstream, {@code SqlParserUtil.intervalToMillis} interprets that array as - * {@code [sign, day, hour, minute, second, ms]}, so the week value lands in - * the hour slot. The bug was fixed upstream in Calcite 1.38.0 - * (CALCITE-6391), - * but Druid is still on Calcite 1.37.0, so we work around it by rewriting any - * {@code WEEK} interval in the parsed {@link SqlNode} tree to an equivalent - * {@code DAY} interval whose value has been multiplied by seven. + *

Both bugs live in {@code SqlIntervalQualifier}: + *

* - *

Two parsed forms reach this shuttle: + *

Both bugs were fixed upstream in Calcite 1.38.0 + * (CALCITE-6581), + * but Druid is still on Calcite 1.37.0, so we work around them by rewriting any + * affected interval in the parsed {@link SqlNode} tree to an equivalent + * interval in a unit that Calcite 1.37 handles correctly: + *

+ * + *

Two parsed forms reach this shuttle for each affected unit: *

* - *

This rewrite preserves the semantics of {@code TimeUnitRange.WEEK} while - * avoiding the buggy code path inside Calcite. Druid still treats WEEK as a - * day-time (millisecond) interval in arithmetic, which matches the expectation - * documented in the issue: {@code INTERVAL '1' WEEK} should add seven days. + *

This rewrite preserves the semantics of {@code TimeUnitRange.WEEK} and + * {@code TimeUnitRange.QUARTER} while avoiding the buggy code paths inside + * Calcite. {@code WEEK(SUNDAY)..WEEK(SATURDAY)} and {@code ISOWEEK} qualifiers + * carry a non-null {@code timeFrameName} and are handled differently in + * Calcite, so the shuttle leaves them alone and only touches the bare + * {@code WEEK} form that is broken in 1.37. */ public class SqlIntervalWeekRewriteShuttle extends SqlShuttle { private static final BigDecimal SEVEN = BigDecimal.valueOf(7); + private static final BigDecimal THREE = BigDecimal.valueOf(3); @Override public SqlNode visit(SqlLiteral literal) @@ -76,15 +96,28 @@ public SqlNode visit(SqlLiteral literal) final SqlIntervalLiteral intervalLiteral = (SqlIntervalLiteral) literal; final SqlIntervalLiteral.IntervalValue value = (SqlIntervalLiteral.IntervalValue) intervalLiteral.getValue(); - if (value != null && isPlainWeek(value.getIntervalQualifier())) { - final String multiplied = multiplyByWeek(value.getIntervalLiteral()); - if (multiplied != null) { - return SqlLiteral.createInterval( - value.getSign(), - multiplied, - new SqlIntervalQualifier(TimeUnit.DAY, null, value.getIntervalQualifier().getParserPosition()), - literal.getParserPosition() - ); + if (value != null) { + final SqlIntervalQualifier qualifier = value.getIntervalQualifier(); + if (isPlainWeek(qualifier)) { + final String multiplied = multiplyLiteral(value.getIntervalLiteral(), SEVEN); + if (multiplied != null) { + return SqlLiteral.createInterval( + value.getSign(), + multiplied, + new SqlIntervalQualifier(TimeUnit.DAY, null, qualifier.getParserPosition()), + literal.getParserPosition() + ); + } + } else if (isPlainQuarter(qualifier)) { + final String multiplied = multiplyLiteral(value.getIntervalLiteral(), THREE); + if (multiplied != null) { + return SqlLiteral.createInterval( + value.getSign(), + multiplied, + new SqlIntervalQualifier(TimeUnit.MONTH, null, qualifier.getParserPosition()), + literal.getParserPosition() + ); + } } } } @@ -96,25 +129,39 @@ public SqlNode visit(SqlCall call) { if (call.getOperator() == SqlStdOperatorTable.INTERVAL && call.operandCount() == 2) { final SqlNode qualifierNode = call.operand(1); - if (qualifierNode instanceof SqlIntervalQualifier - && isPlainWeek((SqlIntervalQualifier) qualifierNode)) { - // First, recurse into the numeric operand so any nested rewrites still happen. - final SqlNode rewrittenNumeric = call.operand(0).accept(this); - final SqlNode numeric = rewrittenNumeric == null ? call.operand(0) : rewrittenNumeric; - final SqlParserPos pos = call.getParserPosition(); - final SqlNode multipliedNumeric = SqlStdOperatorTable.MULTIPLY.createCall( - pos, - numeric, - SqlLiteral.createExactNumeric("7", pos) - ); - final SqlIntervalQualifier dayQualifier = - new SqlIntervalQualifier(TimeUnit.DAY, null, qualifierNode.getParserPosition()); - return SqlStdOperatorTable.INTERVAL.createCall(pos, multipliedNumeric, dayQualifier); + if (qualifierNode instanceof SqlIntervalQualifier) { + final SqlIntervalQualifier qualifier = (SqlIntervalQualifier) qualifierNode; + if (isPlainWeek(qualifier)) { + return rewriteIntervalCall(call, qualifier, "7", TimeUnit.DAY); + } else if (isPlainQuarter(qualifier)) { + return rewriteIntervalCall(call, qualifier, "3", TimeUnit.MONTH); + } } } return super.visit(call); } + private SqlNode rewriteIntervalCall( + SqlCall call, + SqlIntervalQualifier qualifier, + String factor, + TimeUnit rewrittenUnit + ) + { + // First, recurse into the numeric operand so any nested rewrites still happen. + final SqlNode rewrittenNumeric = call.operand(0).accept(this); + final SqlNode numeric = rewrittenNumeric == null ? call.operand(0) : rewrittenNumeric; + final SqlParserPos pos = call.getParserPosition(); + final SqlNode multipliedNumeric = SqlStdOperatorTable.MULTIPLY.createCall( + pos, + numeric, + SqlLiteral.createExactNumeric(factor, pos) + ); + final SqlIntervalQualifier rewrittenQualifier = + new SqlIntervalQualifier(rewrittenUnit, null, qualifier.getParserPosition()); + return SqlStdOperatorTable.INTERVAL.createCall(pos, multipliedNumeric, rewrittenQualifier); + } + private static boolean isPlainWeek(SqlIntervalQualifier qualifier) { // Only the bare WEEK qualifier is affected. WEEK(SUNDAY)..WEEK(SATURDAY) and @@ -124,13 +171,21 @@ private static boolean isPlainWeek(SqlIntervalQualifier qualifier) && qualifier.timeFrameName == null; } + private static boolean isPlainQuarter(SqlIntervalQualifier qualifier) + { + // Only the bare QUARTER qualifier is affected. + return qualifier != null + && qualifier.timeUnitRange == TimeUnitRange.QUARTER + && qualifier.timeFrameName == null; + } + /** - * Multiplies the integer string value of a WEEK interval literal by seven. + * Multiplies the integer string value of an interval literal by a factor. * Returns null if the value is not a plain non-negative integer literal, - * which means it does not match Calcite's WEEK interval grammar and would - * fail validation anyway. + * which means it does not match Calcite's WEEK/QUARTER interval grammar and + * would fail validation anyway. */ - private static String multiplyByWeek(String intervalStr) + private static String multiplyLiteral(String intervalStr, BigDecimal factor) { if (intervalStr == null || intervalStr.isEmpty()) { return null; @@ -141,6 +196,6 @@ private static String multiplyByWeek(String intervalStr) return null; } } - return new BigDecimal(intervalStr).multiply(SEVEN).toString(); + return new BigDecimal(intervalStr).multiply(factor).toString(); } } diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java index f7b9540434c8..ee253c0ddaf8 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java @@ -6683,6 +6683,43 @@ public void testIntervalWeekResolution() ); } + /** + * Regression test for the companion bug to {@link #testIntervalWeekResolution()}: + * {@code INTERVAL '1' QUARTER} previously folded to one month instead of three months + * because Calcite 1.37.0 mishandles the QUARTER qualifier (packages the value into a + * year-month array without multiplying by three). The interval narrowing below proves + * the fix lands the equality on {@code 2000-04-01} (three months after + * {@code 2000-01-01}) and {@code 2000-07-01} (six months after {@code 2000-01-01}). + */ + @Test + public void testIntervalQuarterResolution() + { + testQuery( + "SELECT COUNT(*) FROM druid.foo WHERE " + + "__time = TIMESTAMP '2000-01-01 00:00:00' OR " + + "__time = TIMESTAMP '2000-01-01 00:00:00' + INTERVAL '1' QUARTER OR " + + "__time = TIMESTAMP '2000-01-01 00:00:00' + INTERVAL 2 QUARTER", + ImmutableList.of( + Druids.newTimeseriesQueryBuilder() + .dataSource(CalciteTests.DATASOURCE1) + .intervals( + querySegmentSpec( + Intervals.of("2000-01-01/2000-01-01T00:00:00.001"), + Intervals.of("2000-04-01/2000-04-01T00:00:00.001"), + Intervals.of("2000-07-01/2000-07-01T00:00:00.001") + ) + ) + .granularity(Granularities.ALL) + .aggregators(aggregators(new CountAggregatorFactory("a0"))) + .context(QUERY_CONTEXT_DEFAULT) + .build() + ), + ImmutableList.of( + new Object[]{1L} + ) + ); + } + @Test public void testCountStarWithComplexDisjointTimeFilter() { diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/planner/SqlIntervalWeekRewriteShuttleTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/planner/SqlIntervalWeekRewriteShuttleTest.java index f456d811d366..39a153f87528 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/planner/SqlIntervalWeekRewriteShuttleTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/planner/SqlIntervalWeekRewriteShuttleTest.java @@ -40,7 +40,9 @@ /** * Verifies that {@link SqlIntervalWeekRewriteShuttle} rewrites * {@code INTERVAL ... WEEK} into the equivalent {@code INTERVAL ... DAY} - * with the value multiplied by seven, working around CALCITE-6391. + * with the value multiplied by seven, and {@code INTERVAL ... QUARTER} into + * the equivalent {@code INTERVAL ... MONTH} with the value multiplied by three, + * working around CALCITE-6581. */ public class SqlIntervalWeekRewriteShuttleTest extends CalciteTestBase { @@ -173,39 +175,140 @@ public void testUnquotedDayIntervalCallIsLeftUntouched() assertSame(original, rewritten); } + @Test + public void testQuotedSingleQuarterLiteralBecomesThreeMonths() + { + final SqlNode original = SqlLiteral.createInterval( + 1, + "1", + new SqlIntervalQualifier(TimeUnit.QUARTER, null, SqlParserPos.ZERO), + SqlParserPos.ZERO + ); + + final SqlNode rewritten = original.accept(shuttle); + assertNotSame(original, rewritten); + assertTrue(rewritten instanceof SqlIntervalLiteral); + final SqlIntervalLiteral.IntervalValue value = + (SqlIntervalLiteral.IntervalValue) ((SqlIntervalLiteral) rewritten).getValue(); + assertEquals("3", value.getIntervalLiteral()); + assertEquals(TimeUnitRange.MONTH, value.getIntervalQualifier().timeUnitRange); + assertEquals(1, value.getSign()); + } + + @Test + public void testQuotedMultiQuarterLiteralIsMultipliedByThree() + { + final SqlNode original = SqlLiteral.createInterval( + 1, + "4", + new SqlIntervalQualifier(TimeUnit.QUARTER, null, SqlParserPos.ZERO), + SqlParserPos.ZERO + ); + + final SqlNode rewritten = original.accept(shuttle); + final SqlIntervalLiteral.IntervalValue value = + (SqlIntervalLiteral.IntervalValue) ((SqlIntervalLiteral) rewritten).getValue(); + assertEquals("12", value.getIntervalLiteral()); + assertEquals(TimeUnitRange.MONTH, value.getIntervalQualifier().timeUnitRange); + } + + @Test + public void testNegativeQuotedQuarterLiteralPreservesSign() + { + final SqlNode original = SqlLiteral.createInterval( + -1, + "2", + new SqlIntervalQualifier(TimeUnit.QUARTER, null, SqlParserPos.ZERO), + SqlParserPos.ZERO + ); + + final SqlNode rewritten = original.accept(shuttle); + final SqlIntervalLiteral.IntervalValue value = + (SqlIntervalLiteral.IntervalValue) ((SqlIntervalLiteral) rewritten).getValue(); + assertEquals("6", value.getIntervalLiteral()); + assertEquals(TimeUnitRange.MONTH, value.getIntervalQualifier().timeUnitRange); + assertEquals(-1, value.getSign()); + } + + @Test + public void testUnquotedQuarterIntervalCallBecomesMultiplyMonthCall() + { + // Mirrors what Druid's parser produces for `INTERVAL 1 QUARTER`: + // SqlStdOperatorTable.INTERVAL.createCall(pos, n, qualifier) + final SqlNode original = SqlStdOperatorTable.INTERVAL.createCall( + SqlParserPos.ZERO, + SqlLiteral.createExactNumeric("2", SqlParserPos.ZERO), + new SqlIntervalQualifier(TimeUnit.QUARTER, null, SqlParserPos.ZERO) + ); + + final SqlNode rewritten = original.accept(shuttle); + assertNotSame(original, rewritten); + assertTrue(rewritten instanceof SqlBasicCall); + final SqlBasicCall rewrittenCall = (SqlBasicCall) rewritten; + assertSame(SqlStdOperatorTable.INTERVAL, rewrittenCall.getOperator()); + + // Operand 0 should be (numeric * 3). + final SqlNode numeric = rewrittenCall.operand(0); + assertTrue(numeric instanceof SqlBasicCall); + final SqlBasicCall numericCall = (SqlBasicCall) numeric; + assertSame(SqlStdOperatorTable.MULTIPLY, numericCall.getOperator()); + assertEquals(2, numericCall.operandCount()); + assertEquals("3", ((SqlLiteral) numericCall.operand(1)).toValue()); + + // Operand 1 should now be a MONTH qualifier. + final SqlNode qualifier = rewrittenCall.operand(1); + assertTrue(qualifier instanceof SqlIntervalQualifier); + assertEquals(TimeUnitRange.MONTH, ((SqlIntervalQualifier) qualifier).timeUnitRange); + } + @Test public void testParsedWeekIntervalIsRewritten() throws Exception { // End-to-end check using the actual parser, both forms. - assertParsedWeekIsRewritten("SELECT TIMESTAMP '1970-01-01 00:00:00' + INTERVAL '1' WEEK"); - assertParsedWeekIsRewritten("SELECT TIMESTAMP '1970-01-01 00:00:00' + INTERVAL 1 WEEK"); - assertParsedWeekIsRewritten("SELECT TIMESTAMP '1970-01-01 00:00:00' + INTERVAL 2 WEEK"); + assertParsedUnitIsRewritten("SELECT TIMESTAMP '1970-01-01 00:00:00' + INTERVAL '1' WEEK", TimeUnitRange.WEEK); + assertParsedUnitIsRewritten("SELECT TIMESTAMP '1970-01-01 00:00:00' + INTERVAL 1 WEEK", TimeUnitRange.WEEK); + assertParsedUnitIsRewritten("SELECT TIMESTAMP '1970-01-01 00:00:00' + INTERVAL 2 WEEK", TimeUnitRange.WEEK); } - private void assertParsedWeekIsRewritten(String sql) throws Exception + @Test + public void testParsedQuarterIntervalIsRewritten() throws Exception + { + // End-to-end check using the actual parser, both forms. + assertParsedUnitIsRewritten("SELECT TIMESTAMP '1970-01-01 00:00:00' + INTERVAL '1' QUARTER", TimeUnitRange.QUARTER); + assertParsedUnitIsRewritten("SELECT TIMESTAMP '1970-01-01 00:00:00' + INTERVAL 1 QUARTER", TimeUnitRange.QUARTER); + assertParsedUnitIsRewritten("SELECT TIMESTAMP '1970-01-01 00:00:00' + INTERVAL 2 QUARTER", TimeUnitRange.QUARTER); + } + + private void assertParsedUnitIsRewritten(String sql, TimeUnitRange unit) throws Exception { final SqlNode parsed = SqlParser.create(sql).parseQuery(); final SqlNode rewritten = parsed.accept(shuttle); - final WeekFinder finder = new WeekFinder(); + final UnitFinder finder = new UnitFinder(unit); rewritten.accept(finder); assertTrue( !finder.found, - "Rewritten tree for [" + sql + "] should not contain a WEEK qualifier, but it did." + "Rewritten tree for [" + sql + "] should not contain a " + unit + " qualifier, but it did." ); } /** - * SqlShuttle that records whether it visited any plain WEEK - * {@link SqlIntervalQualifier}. + * SqlShuttle that records whether it visited any plain + * {@link SqlIntervalQualifier} for the given unit (with null timeFrameName). */ - private static class WeekFinder extends org.apache.calcite.sql.util.SqlShuttle + private static class UnitFinder extends org.apache.calcite.sql.util.SqlShuttle { + private final TimeUnitRange targetUnit; boolean found; + UnitFinder(TimeUnitRange targetUnit) + { + this.targetUnit = targetUnit; + } + @Override public SqlNode visit(SqlIntervalQualifier intervalQualifier) { - if (intervalQualifier.timeUnitRange == TimeUnitRange.WEEK + if (intervalQualifier.timeUnitRange == targetUnit && intervalQualifier.timeFrameName == null) { found = true; }