Both bugs live in {@code SqlIntervalQualifier}: + *
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} 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) + { + if (literal instanceof SqlIntervalLiteral) { + final SqlIntervalLiteral intervalLiteral = (SqlIntervalLiteral) literal; + final SqlIntervalLiteral.IntervalValue value = + (SqlIntervalLiteral.IntervalValue) intervalLiteral.getValue(); + 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() + ); + } + } + } + } + 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) { + 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 + // ISOWEEK use a non-null timeFrameName and are handled differently in Calcite. + return qualifier != null + && qualifier.timeUnitRange == TimeUnitRange.WEEK + && 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 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/QUARTER interval grammar and + * would fail validation anyway. + */ + private static String multiplyLiteral(String intervalStr, BigDecimal factor) + { + 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(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 fcb944852453..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 @@ -6647,6 +6647,79 @@ 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} + ) + ); + } + + /** + * 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 new file mode 100644 index 000000000000..39a153f87528 --- /dev/null +++ b/sql/src/test/java/org/apache/druid/sql/calcite/planner/SqlIntervalWeekRewriteShuttleTest.java @@ -0,0 +1,318 @@ +/* + * 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, 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 +{ + 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 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. + 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); + } + + @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 UnitFinder finder = new UnitFinder(unit); + rewritten.accept(finder); + assertTrue( + !finder.found, + "Rewritten tree for [" + sql + "] should not contain a " + unit + " qualifier, but it did." + ); + } + + /** + * SqlShuttle that records whether it visited any plain + * {@link SqlIntervalQualifier} for the given unit (with null timeFrameName). + */ + 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 == targetUnit + && intervalQualifier.timeFrameName == null) { + found = true; + } + return intervalQualifier; + } + } +}