You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/09/28 09:41:58 UTC
[GitHub] [druid] abhishekagarwal87 opened a new pull request, #13151: Fix assertion error in sql planning for latest aggregators
abhishekagarwal87 opened a new pull request, #13151:
URL: https://github.com/apache/druid/pull/13151
Fixes https://github.com/apache/druid/issues/13091.
This PR has:
- [ ] been self-reviewed.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #13151: Fix assertion error in sql planning for latest aggregators
Posted by GitBox <gi...@apache.org>.
kfaraz commented on code in PR #13151:
URL: https://github.com/apache/druid/pull/13151#discussion_r982268975
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -998,6 +998,56 @@ public void testStringLatestGroupBy()
);
}
+ @Test
+ public void testStringLatestGroupByWithAlwaysFalseCondition()
+ {
+ 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 testStringLatestByGroupByWithAlwaysFalseCondition()
Review Comment:
Perhaps a `LATEST(dim4, "100")` but it's not essential, we can do it later.
##########
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));
Review Comment:
Thanks for the clarification, I was looking at an older doc in the original PR.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #13151: Fix assertion error in sql planning for latest aggregators
Posted by GitBox <gi...@apache.org>.
kfaraz commented on code in PR #13151:
URL: https://github.com/apache/druid/pull/13151#discussion_r982243971
##########
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));
Review Comment:
From the docs, it seems that `maxBytesPerString` is always the second argument, i.e. `rexNodes.get(1)`
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a diff in pull request #13151: Fix assertion error in sql planning for latest aggregators
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on code in PR #13151:
URL: https://github.com/apache/druid/pull/13151#discussion_r982255912
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -998,6 +998,56 @@ public void testStringLatestGroupBy()
);
}
+ @Test
+ public void testStringLatestGroupByWithAlwaysFalseCondition()
+ {
+ 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 testStringLatestByGroupByWithAlwaysFalseCondition()
Review Comment:
I was not able to figure out a query that gets me that error message. This query is not an invalid query. It just goes through a bad stage and doesn't recover before this fix. Now it can.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a diff in pull request #13151: Fix assertion error in sql planning for latest aggregators
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on code in PR #13151:
URL: https://github.com/apache/druid/pull/13151#discussion_r982283458
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -998,6 +998,56 @@ public void testStringLatestGroupBy()
);
}
+ @Test
+ public void testStringLatestGroupByWithAlwaysFalseCondition()
+ {
+ 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 testStringLatestByGroupByWithAlwaysFalseCondition()
Review Comment:
that SQL will generate a different error and fails even before reaching the SqlAggregagtor.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #13151: Fix assertion error in sql planning for latest aggregators
Posted by GitBox <gi...@apache.org>.
kfaraz commented on code in PR #13151:
URL: https://github.com/apache/druid/pull/13151#discussion_r982241333
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -998,6 +998,56 @@ public void testStringLatestGroupBy()
);
}
+ @Test
+ public void testStringLatestGroupByWithAlwaysFalseCondition()
+ {
+ 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 testStringLatestByGroupByWithAlwaysFalseCondition()
Review Comment:
Nit: Maybe also add a test case to verify error message?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #13151: Fix assertion error in sql planning for latest aggregators
Posted by GitBox <gi...@apache.org>.
kfaraz commented on code in PR #13151:
URL: https://github.com/apache/druid/pull/13151#discussion_r982238578
##########
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));
Review Comment:
The arguments to this formatted error message seem reversed.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 merged pull request #13151: Fix assertion error in sql planning for latest aggregators
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 merged PR #13151:
URL: https://github.com/apache/druid/pull/13151
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #13151: Fix assertion error in sql planning for latest aggregators
Posted by GitBox <gi...@apache.org>.
kfaraz commented on code in PR #13151:
URL: https://github.com/apache/druid/pull/13151#discussion_r982233652
##########
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));
Review Comment:
Nit:
```suggestion
plannerContext.setPlanningError("The second argument '%s' to '%s' function is not an integer", aggregateCall.getName(), rexNodes.get(1));
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a diff in pull request #13151: Fix assertion error in sql planning for latest aggregators
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on code in PR #13151:
URL: https://github.com/apache/druid/pull/13151#discussion_r982257063
##########
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));
Review Comment:
For `EARLIEST_BY/LATEST_BY`, it's the third argument. For non `_BY` aggregators, it's second argument.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #13151: Fix assertion error in sql planning for latest aggregators
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on PR #13151:
URL: https://github.com/apache/druid/pull/13151#issuecomment-1261088123
Merged since build failure is unrelated.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #13151: Fix assertion error in sql planning for latest aggregators
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on PR #13151:
URL: https://github.com/apache/druid/pull/13151#issuecomment-1260741675
Thank you @kfaraz and @cryptoe for your review. I have made the changes.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #13151: Fix assertion error in sql planning for latest aggregators
Posted by GitBox <gi...@apache.org>.
kfaraz commented on code in PR #13151:
URL: https://github.com/apache/druid/pull/13151#discussion_r982240104
##########
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));
Review Comment:
We can also safely say that the argument should be an integer (maybe also mention the name of the argument) rather than saying that it should be a "literal".
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a diff in pull request #13151: Fix assertion error in sql planning for latest aggregators
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on code in PR #13151:
URL: https://github.com/apache/druid/pull/13151#discussion_r982258082
##########
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));
Review Comment:
I fixed the error message though.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org