You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "somu-imply (via GitHub)" <gi...@apache.org> on 2023/03/22 18:31:37 UTC
[GitHub] [druid] somu-imply opened a new pull request, #13965: Planning correctly for order by queries on time which previously thre…
somu-imply opened a new pull request, #13965:
URL: https://github.com/apache/druid/pull/13965
Queries such as the one below fail with Cannot convert query parts into an actual query in and post 2022.05 Druid versions.
SELECT __time,channel FROM wikipedia
WHERE
(
cityName is not null
and channel in
(
SELECT channel FROM wikipedia
))
ORDER BY __time DESC
This bug comes from regression on #12418 should also fix the issue reported in #13163 and also modifies #13173
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] 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.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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] cheddar merged pull request #13965: Planning correctly for order by queries on time which previously thre…
Posted by "cheddar (via GitHub)" <gi...@apache.org>.
cheddar merged PR #13965:
URL: https://github.com/apache/druid/pull/13965
--
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] somu-imply commented on a diff in pull request #13965: Planning correctly for order by queries on time which previously thre…
Posted by "somu-imply (via GitHub)" <gi...@apache.org>.
somu-imply commented on code in PR #13965:
URL: https://github.com/apache/druid/pull/13965#discussion_r1145293604
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java:
##########
@@ -1448,15 +1449,6 @@ private ScanQuery toScanQuery()
);
return null;
}
- if (!dataSource.isConcrete()) {
Review Comment:
This was a change in #12418 that was reverted. Since we are using a table data source and not a query data source now the isConcrete() check is not needed and can be removed
--
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] imply-cheddar commented on a diff in pull request #13965: Planning correctly for order by queries on time which previously thre…
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13965:
URL: https://github.com/apache/druid/pull/13965#discussion_r1147040736
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -7306,21 +7306,26 @@ public void testMinMaxAvgDailyCountWithLimit()
)
)
.setInterval(querySegmentSpec(Filtration.eternity()))
- .setLimitSpec(
- new DefaultLimitSpec(
- ImmutableList.of(),
- 1
- )
- )
.setGranularity(Granularities.ALL)
.setAggregatorSpecs(
+ useDefault ?
aggregators(
new LongMaxAggregatorFactory("_a0", "a0"),
new LongMinAggregatorFactory("_a1", "a0"),
new LongSumAggregatorFactory("_a2:sum", "a0"),
new CountAggregatorFactory("_a2:count"),
new LongMaxAggregatorFactory("_a3", "d0"),
new CountAggregatorFactory("_a4")
+ ) : aggregators(
+ new LongMaxAggregatorFactory("_a0", "a0"),
+ new LongMinAggregatorFactory("_a1", "a0"),
+ new LongSumAggregatorFactory("_a2:sum", "a0"),
+ new FilteredAggregatorFactory(
+ new CountAggregatorFactory("_a2:count"),
+ not(selector("a0", null, null))
+ ),
+ new LongMaxAggregatorFactory("_a3", "d0"),
+ new CountAggregatorFactory("_a4")
Review Comment:
Why does this change generate this adjustment to the plan?
--
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] somu-imply commented on a diff in pull request #13965: Planning correctly for order by queries on time which previously thre…
Posted by "somu-imply (via GitHub)" <gi...@apache.org>.
somu-imply commented on code in PR #13965:
URL: https://github.com/apache/druid/pull/13965#discussion_r1148160452
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -7306,21 +7306,26 @@ public void testMinMaxAvgDailyCountWithLimit()
)
)
.setInterval(querySegmentSpec(Filtration.eternity()))
- .setLimitSpec(
- new DefaultLimitSpec(
- ImmutableList.of(),
- 1
- )
- )
.setGranularity(Granularities.ALL)
.setAggregatorSpecs(
+ useDefault ?
aggregators(
new LongMaxAggregatorFactory("_a0", "a0"),
new LongMinAggregatorFactory("_a1", "a0"),
new LongSumAggregatorFactory("_a2:sum", "a0"),
new CountAggregatorFactory("_a2:count"),
new LongMaxAggregatorFactory("_a3", "d0"),
new CountAggregatorFactory("_a4")
+ ) : aggregators(
+ new LongMaxAggregatorFactory("_a0", "a0"),
+ new LongMinAggregatorFactory("_a1", "a0"),
+ new LongSumAggregatorFactory("_a2:sum", "a0"),
+ new FilteredAggregatorFactory(
+ new CountAggregatorFactory("_a2:count"),
+ not(selector("a0", null, null))
+ ),
+ new LongMaxAggregatorFactory("_a3", "d0"),
+ new CountAggregatorFactory("_a4")
Review Comment:
No clue but in the case of the useDefault=false it uses the same thing but with a check of not null so thought it was kind of correct
--
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