From 32f644cd483ec8c3eec041cadffc6493e6edff1e Mon Sep 17 00:00:00 2001 From: Abhishek Agarwal <1477457+abhishekagarwal87@users.noreply.github.com> Date: Wed, 28 Sep 2022 14:31:52 +0530 Subject: [PATCH 1/4] Fix sql planning bug for latest aggregators --- .../EarliestLatestAnySqlAggregator.java | 10 +++- .../EarliestLatestBySqlAggregator.java | 10 +++- .../druid/sql/calcite/CalciteQueryTest.java | 50 +++++++++++++++++++ 3 files changed, 68 insertions(+), 2 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java index 58fd083dcabc..e3f359a7a65c 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java @@ -202,12 +202,20 @@ public Aggregation toDruidAggregation( theAggFactory = aggregatorType.createAggregatorFactory(aggregatorName, fieldName, null, outputType, -1); break; case 2: + int maxStringBytes; + try { + maxStringBytes = RexLiteral.intValue(rexNodes.get(1)); + } + catch (AssertionError ae) { + plannerContext.setPlanningError("The second argument '%s' to '%s' function is not a literal", aggregateCall.getName(), rexNodes.get(1)); + return null; + } theAggFactory = aggregatorType.createAggregatorFactory( aggregatorName, fieldName, null, outputType, - RexLiteral.intValue(rexNodes.get(1)) + maxStringBytes ); break; default: diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java index f13a918e6fa6..e6d22632da2e 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java @@ -122,12 +122,20 @@ public Aggregation toDruidAggregation( ); break; case 3: + int maxStringBytes; + try { + maxStringBytes = RexLiteral.intValue(rexNodes.get(2)); + } + catch (AssertionError ae) { + plannerContext.setPlanningError("The second argument '%s' to '%s' function is not a literal", aggregateCall.getName(), rexNodes.get(1)); + return null; + } theAggFactory = aggregatorType.createAggregatorFactory( aggregatorName, fieldName, EarliestLatestAnySqlAggregator.getColumnName(plannerContext, virtualColumnRegistry, args.get(1), rexNodes.get(1)), outputType, - RexLiteral.intValue(rexNodes.get(2)) + maxStringBytes ); break; default: 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 d260126e52bc..ef9528603ee1 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 @@ -998,6 +998,56 @@ public void testStringLatestGroupBy() ); } + @Test + public void testStringLatestGroupByWithImpossibleQuery() + { + testQuery( + "SELECT LATEST(dim4, 10),dim2 FROM numfoo WHERE (dim1 = 'something' AND dim1 IN( 'something else') ) GROUP BY dim2", + ImmutableList.of( + Druids.newScanQueryBuilder() + .dataSource(InlineDataSource.fromIterable( + ImmutableList.of(), + RowSignature.builder() + .add("EXPR$0", ColumnType.STRING) + .add("dim2", ColumnType.STRING) + .build() + )) + .intervals(querySegmentSpec(Filtration.eternity())) + .columns("EXPR$0", "dim2") + .context(QUERY_CONTEXT_DEFAULT) + .resultFormat(ResultFormat.RESULT_FORMAT_COMPACTED_LIST) + .legacy(false) + .build() + ), + ImmutableList.of() + ); + } + + @Test + public void testStringLatestByGroupByWithImpossibleQuery() + { + testQuery( + "SELECT LATEST_BY(dim4, __time, 10),dim2 FROM numfoo WHERE (dim1 = 'something' AND dim1 IN( 'something else') ) GROUP BY dim2", + ImmutableList.of( + Druids.newScanQueryBuilder() + .dataSource(InlineDataSource.fromIterable( + ImmutableList.of(), + RowSignature.builder() + .add("EXPR$0", ColumnType.STRING) + .add("dim2", ColumnType.STRING) + .build() + )) + .intervals(querySegmentSpec(Filtration.eternity())) + .columns("EXPR$0", "dim2") + .context(QUERY_CONTEXT_DEFAULT) + .resultFormat(ResultFormat.RESULT_FORMAT_COMPACTED_LIST) + .legacy(false) + .build() + ), + ImmutableList.of() + ); + } + // This test the off-heap (buffer) version of the EarliestAggregator (Double/Float/Long) @Test public void testPrimitiveEarliestInSubquery() From 13df8fe9c8eef852f63282c53c185a937da9b97d Mon Sep 17 00:00:00 2001 From: Abhishek Agarwal <1477457+abhishekagarwal87@users.noreply.github.com> Date: Wed, 28 Sep 2022 15:10:25 +0530 Subject: [PATCH 2/4] change test name --- .../java/org/apache/druid/sql/calcite/CalciteQueryTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 ef9528603ee1..bd21a5798843 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 @@ -999,7 +999,7 @@ public void testStringLatestGroupBy() } @Test - public void testStringLatestGroupByWithImpossibleQuery() + public void testStringLatestGroupByWithAlwaysFalseCondition() { testQuery( "SELECT LATEST(dim4, 10),dim2 FROM numfoo WHERE (dim1 = 'something' AND dim1 IN( 'something else') ) GROUP BY dim2", @@ -1024,7 +1024,7 @@ public void testStringLatestGroupByWithImpossibleQuery() } @Test - public void testStringLatestByGroupByWithImpossibleQuery() + public void testStringLatestByGroupByWithAlwaysFalseCondition() { testQuery( "SELECT LATEST_BY(dim4, __time, 10),dim2 FROM numfoo WHERE (dim1 = 'something' AND dim1 IN( 'something else') ) GROUP BY dim2", From 26fa7c423f2b2218b419ce7f711f3122da62c387 Mon Sep 17 00:00:00 2001 From: Abhishek Agarwal <1477457+abhishekagarwal87@users.noreply.github.com> Date: Wed, 28 Sep 2022 16:32:41 +0530 Subject: [PATCH 3/4] Fix error messages --- .../aggregation/builtin/EarliestLatestAnySqlAggregator.java | 2 +- .../aggregation/builtin/EarliestLatestBySqlAggregator.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java index e3f359a7a65c..c0f20b293913 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java @@ -207,7 +207,7 @@ public Aggregation toDruidAggregation( maxStringBytes = RexLiteral.intValue(rexNodes.get(1)); } catch (AssertionError ae) { - plannerContext.setPlanningError("The second argument '%s' to '%s' function is not a literal", aggregateCall.getName(), rexNodes.get(1)); + plannerContext.setPlanningError("The second argument '%s' to function '%s' is not a number", aggregateCall.getName(), rexNodes.get(1)); return null; } theAggFactory = aggregatorType.createAggregatorFactory( diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java index e6d22632da2e..8d9e08eb8695 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java @@ -127,7 +127,7 @@ public Aggregation toDruidAggregation( maxStringBytes = RexLiteral.intValue(rexNodes.get(2)); } catch (AssertionError ae) { - plannerContext.setPlanningError("The second argument '%s' to '%s' function is not a literal", aggregateCall.getName(), rexNodes.get(1)); + plannerContext.setPlanningError("The third argument '%s' to function '%s' is not a number", aggregateCall.getName(), rexNodes.get(2)); return null; } theAggFactory = aggregatorType.createAggregatorFactory( From a6a8f044d177deadb92c8b2698996f3a166ca330 Mon Sep 17 00:00:00 2001 From: Abhishek Agarwal <1477457+abhishekagarwal87@users.noreply.github.com> Date: Wed, 28 Sep 2022 16:44:27 +0530 Subject: [PATCH 4/4] fix error message again --- .../aggregation/builtin/EarliestLatestAnySqlAggregator.java | 2 +- .../aggregation/builtin/EarliestLatestBySqlAggregator.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java index c0f20b293913..ba061483639e 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java @@ -207,7 +207,7 @@ public Aggregation toDruidAggregation( maxStringBytes = RexLiteral.intValue(rexNodes.get(1)); } catch (AssertionError ae) { - plannerContext.setPlanningError("The second argument '%s' to function '%s' is not a number", aggregateCall.getName(), rexNodes.get(1)); + plannerContext.setPlanningError("The second argument '%s' to function '%s' is not a number", rexNodes.get(1), aggregateCall.getName()); return null; } theAggFactory = aggregatorType.createAggregatorFactory( diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java index 8d9e08eb8695..46762306a45b 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestBySqlAggregator.java @@ -127,7 +127,7 @@ public Aggregation toDruidAggregation( maxStringBytes = RexLiteral.intValue(rexNodes.get(2)); } catch (AssertionError ae) { - plannerContext.setPlanningError("The third argument '%s' to function '%s' is not a number", aggregateCall.getName(), rexNodes.get(2)); + plannerContext.setPlanningError("The third argument '%s' to function '%s' is not a number", rexNodes.get(2), aggregateCall.getName()); return null; } theAggFactory = aggregatorType.createAggregatorFactory(