You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "xuzifu666 (via GitHub)" <gi...@apache.org> on 2025/02/27 12:59:11 UTC
[PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
xuzifu666 opened a new pull request, #4218:
URL: https://github.com/apache/calcite/pull/4218
As description in JIRA: https://issues.apache.org/jira/browse/CALCITE-6867
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4218:
URL: https://github.com/apache/calcite/pull/4218#discussion_r1973563495
##########
druid/src/main/java/org/apache/calcite/adapter/druid/DruidJsonFilter.java:
##########
@@ -268,14 +268,11 @@ abstract class DruidJsonFilter implements DruidJson {
private static @Nullable DruidJsonFilter toInKindDruidFilter(RexNode e, RelDataType rowType,
DruidQuery druidQuery) {
- switch (e.getKind()) {
Review Comment:
This seems an equivalent rewrite? In that case please revert, we try to keep the diff as minimal as possible unless there is a compelling reason for the change.
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on PR #4218:
URL: https://github.com/apache/calcite/pull/4218#issuecomment-2691656450
> @xuzifu666 for completeness let's add an additional test in `DruidAdapter2IT` which hightlights the issue starting from an SQL query if possible.
>
> In [this](https://github.com/apache/calcite/pull/4221/commits/c29985fa82daca3dd62f625868b2114dcd702cc4) commit you have an example of what I have in mind.
>
> Unfortunately one of the Druid images has problems on my Apple M1, so I can't run it locally, CI is green but there are 6 tests skipped in that class and sadly it doesn't detail which ones they are.
>
> If you happen to have a machine where the Druid docker images can run you can check https://github.com/zabetak/calcite-druid-dataset to spin the containers up and then run the Druid tests.
>
> In a few days I will get a Linux box where I can run the tests and check.
Thanks you very much! I would have a try~@asolimando
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4218:
URL: https://github.com/apache/calcite/pull/4218#discussion_r1973574818
##########
druid/src/main/java/org/apache/calcite/adapter/druid/DruidJsonFilter.java:
##########
@@ -294,11 +291,11 @@ abstract class DruidJsonFilter implements DruidJson {
if (columnName == null) {
return null;
}
- if (e.getKind() != SqlKind.NOT_IN) {
+ if (e.getKind() == SqlKind.DRUID_IN) {
return new DruidJsonFilter.JsonInFilter(columnName, listBuilder.build(), extractionFunction);
} else {
- return toNotDruidFilter(
- new DruidJsonFilter.JsonInFilter(columnName, listBuilder.build(), extractionFunction));
+ return new DruidJsonFilter.JsonNotInFilter(columnName,
Review Comment:
I am not too familiar with Druid, but you haven't provided enough evidence the original behaviour is incorrect, and that your proposed changes are the correct ones.
`DruidAdapterIT` and `DruidAdapter2IT` seem to be the right places to add such a test.
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adapter transforms an SQL NOT_IN filter to a Druid IN filter [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on PR #4218:
URL: https://github.com/apache/calcite/pull/4218#issuecomment-2698338265
Merged via [0ff9fcf](https://github.com/apache/calcite/commit/0ff9fcf1e7763434b49aa215c071adcfe28ea097)
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4218:
URL: https://github.com/apache/calcite/pull/4218#issuecomment-2687976183
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4218) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4218&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4218&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4218&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [95.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4218&metric=new_coverage&view=list)
 [29.3% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4218&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4218)
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4218:
URL: https://github.com/apache/calcite/pull/4218#discussion_r1974981227
##########
druid/src/main/java/org/apache/calcite/adapter/druid/DruidJsonFilter.java:
##########
@@ -294,11 +291,11 @@ abstract class DruidJsonFilter implements DruidJson {
if (columnName == null) {
return null;
}
- if (e.getKind() != SqlKind.NOT_IN) {
+ if (e.getKind() == SqlKind.DRUID_IN) {
return new DruidJsonFilter.JsonInFilter(columnName, listBuilder.build(), extractionFunction);
} else {
- return toNotDruidFilter(
- new DruidJsonFilter.JsonInFilter(columnName, listBuilder.build(), extractionFunction));
+ return new DruidJsonFilter.JsonNotInFilter(columnName,
Review Comment:
We need at least one test to show precisely the issue. The test should not pass before your change, and it should pass after it.
In this way we highlight what the change does, what problem it was there, and we protect the change against future possible regressions.
EDIT: my bad, I see you have added such a test, LGTM
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4218:
URL: https://github.com/apache/calcite/pull/4218#discussion_r1973558182
##########
druid/src/test/java/org/apache/calcite/adapter/druid/DruidQueryFilterTest.java:
##########
@@ -87,6 +87,30 @@ class DruidQueryFilterTest {
+ "\"values\":[\"1\",\"5\",\"value1\"]}"));
}
+ @Test void testNotInFilter() throws IOException {
+ final Fixture f = new Fixture();
+ final List<? extends RexNode> listRexNodes =
+ ImmutableList.of(f.rexBuilder.makeInputRef(f.varcharRowType, 0),
+ f.rexBuilder.makeExactLiteral(BigDecimal.valueOf(1)),
+ f.rexBuilder.makeExactLiteral(BigDecimal.valueOf(5)),
+ f.rexBuilder.makeLiteral("value1"));
+
+ RexNode inRexNode =
Review Comment:
Copy-paste typo
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4218:
URL: https://github.com/apache/calcite/pull/4218#discussion_r1973662188
##########
druid/src/main/java/org/apache/calcite/adapter/druid/DruidJsonFilter.java:
##########
@@ -268,14 +268,11 @@ abstract class DruidJsonFilter implements DruidJson {
private static @Nullable DruidJsonFilter toInKindDruidFilter(RexNode e, RelDataType rowType,
DruidQuery druidQuery) {
- switch (e.getKind()) {
Review Comment:
Had changed here as before.
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on PR #4218:
URL: https://github.com/apache/calcite/pull/4218#issuecomment-2692330622
> @xuzifu666 @asolimando I added the following test to DruidAdapter2IT and it returned the same results before and after the change. I agree with @xuzifu666 that the original logic here is incorrect, but I'm not familiar with the code here, and I don't know why the test wasn't affected.
>
> ```
> @Test void testNotInFilter() {
> final String sql = "select \"state_province\", \"city\",\n"
> + " \"product_name\", \"quarter\"\n"
> + "from \"foodmart\"\n"
> + "where \"product_name\" = 'High Top Dried Mushrooms'\n"
> + "and \"quarter\" not in ('Q1', 'Q4')\n"
> + "and \"state_province\" = 'WA'";
> final String druidQuery = "{'queryType':'scan',"
> + "'dataSource':'foodmart',"
> + "'intervals':['1900-01-09T00:00:00.000Z/2992-01-10T00:00:00.000Z'],"
> + "'filter':{'type':'and','fields':["
> + "{'type':'selector','dimension':'product_name','value':'High Top Dried Mushrooms'},"
> + "{'type':'not','field':{'type':'selector','dimension':'quarter','value':'Q1'}},"
> + "{'type':'not','field':{'type':'selector','dimension':'quarter','value':'Q4'}},"
> + "{'type':'selector','dimension':'state_province','value':'WA'}]},"
> + "'columns':['state_province','city','product_name','quarter'],"
> + "'resultFormat':'compactedList'}";
> final String explain = "PLAN=EnumerableInterpreter\n"
> + " DruidQuery(table=[[foodmart, foodmart]], "
> + "intervals=[[1900-01-09T00:00:00.000Z/2992-01-10T00:00:00.000Z]], "
> + "filter=[AND("
> + "=($3, 'High Top Dried Mushrooms'), "
> + "SEARCH($87, Sarg[(-∞..'Q1':VARCHAR), ('Q1':VARCHAR..'Q4':VARCHAR), ('Q4':VARCHAR..+∞)]:VARCHAR), "
> + "=($30, 'WA'))], "
> + "projects=[[$30, $29, $3, $87]])\n";
> sql(sql)
> .queryContains(new DruidChecker(druidQuery))
> .explainContains(explain)
> .returnsUnordered(
> "state_province=WA; city=Bremerton; product_name=High Top Dried Mushrooms; quarter=Q3",
> "state_province=WA; city=Everett; product_name=High Top Dried Mushrooms; quarter=Q3",
> "state_province=WA; city=Kirkland; product_name=High Top Dried Mushrooms; quarter=Q3",
> "state_province=WA; city=Lynnwood; product_name=High Top Dried Mushrooms; quarter=Q3",
> "state_province=WA; city=Olympia; product_name=High Top Dried Mushrooms; quarter=Q2",
> "state_province=WA; city=Port Orchard; product_name=High Top Dried Mushrooms; quarter=Q3",
> "state_province=WA; city=Puyallup; product_name=High Top Dried Mushrooms; quarter=Q2",
> "state_province=WA; city=Puyallup; product_name=High Top Dried Mushrooms; quarter=Q3",
> "state_province=WA; city=Spokane; product_name=High Top Dried Mushrooms; quarter=Q2",
> "state_province=WA; city=Spokane; product_name=High Top Dried Mushrooms; quarter=Q3",
> "state_province=WA; city=Spokane; product_name=High Top Dried Mushrooms; quarter=Q3",
> "state_province=WA; city=Tacoma; product_name=High Top Dried Mushrooms; quarter=Q2",
> "state_province=WA; city=Yakima; product_name=High Top Dried Mushrooms; quarter=Q2",
> "state_province=WA; city=Yakima; product_name=High Top Dried Mushrooms; quarter=Q3",
> "state_province=WA; city=Yakima; product_name=High Top Dried Mushrooms; quarter=Q2");
> }
>
> @Test void testNotInFilter1() {
> final String sql = "select \"state_province\", \"city\",\n"
> + " \"product_name\", \"quarter\"\n"
> + "from \"foodmart\"\n"
> + "where \"product_name\" = 'High Top Dried Mushrooms'\n"
> + "and \"quarter\" in ('Q1', 'Q4')\n"
> + "and \"state_province\" = 'WA'";
> final String druidQuery = "{'queryType':'scan',"
> + "'dataSource':'foodmart',"
> + "'intervals':['1900-01-09T00:00:00.000Z/2992-01-10T00:00:00.000Z'],"
> + "'filter':{'type':'and','fields':["
> + "{'type':'selector','dimension':'product_name','value':'High Top Dried Mushrooms'},"
> + "{'type':'or','fields':[{'type':'selector','dimension':'quarter','value':'Q1'},{'type':'selector','dimension':'quarter','value':'Q4'}]},"
> + "{'type':'selector','dimension':'state_province','value':'WA'}]},"
> + "'columns':['state_province','city','product_name','quarter'],"
> + "'resultFormat':'compactedList'}";
> final String explain = "PLAN=EnumerableInterpreter\n"
> + " DruidQuery(table=[[foodmart, foodmart]], "
> + "intervals=[[1900-01-09T00:00:00.000Z/2992-01-10T00:00:00.000Z]], "
> + "filter=[AND("
> + "=($3, 'High Top Dried Mushrooms'), "
> + "SEARCH($87, Sarg['Q1':VARCHAR, 'Q4':VARCHAR]:VARCHAR), "
> + "=($30, 'WA'))], "
> + "projects=[[$30, $29, $3, $87]])\n";
> sql(sql)
> .queryContains(new DruidChecker(druidQuery))
> .explainContains(explain)
> .returnsUnordered(
> "state_province=WA; city=Bremerton; product_name=High Top Dried Mushrooms; quarter=Q4",
> "state_province=WA; city=Olympia; product_name=High Top Dried Mushrooms; quarter=Q4",
> "state_province=WA; city=Port Orchard; product_name=High Top Dried Mushrooms; quarter=Q1",
> "state_province=WA; city=Puyallup; product_name=High Top Dried Mushrooms; quarter=Q1",
> "state_province=WA; city=Spokane; product_name=High Top Dried Mushrooms; quarter=Q1",
> "state_province=WA; city=Spokane; product_name=High Top Dried Mushrooms; quarter=Q4",
> "state_province=WA; city=Tacoma; product_name=High Top Dried Mushrooms; quarter=Q4",
> "state_province=WA; city=Yakima; product_name=High Top Dried Mushrooms; quarter=Q4");
> }
> ```
>
> change:
>
> ```
> --- a/druid/src/main/java/org/apache/calcite/adapter/druid/DruidJsonFilter.java
> +++ b/druid/src/main/java/org/apache/calcite/adapter/druid/DruidJsonFilter.java
> @@ -294,7 +294,7 @@ abstract class DruidJsonFilter implements DruidJson {
> if (columnName == null) {
> return null;
> }
> - if (e.getKind() != SqlKind.NOT_IN) {
> + if (e.getKind() == SqlKind.DRUID_IN) {
> return new DruidJsonFilter.JsonInFilter(columnName, listBuilder.build(), extractionFunction);
> } else {
> return toNotDruidFilter(
> ```
Thanks for your kindle help for the pr，from logic plan seems this sql not transform to in predicate，maybe can test with only IN or NOT_IN filter.
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on PR #4218:
URL: https://github.com/apache/calcite/pull/4218#issuecomment-2691243756
@xuzifu666 for completeness let's add an additional test in `DruidAdapter2IT` which hightlights the issue starting from an SQL query if possible.
In [this](https://github.com/apache/calcite/pull/4221/commits/c29985fa82daca3dd62f625868b2114dcd702cc4) commit you have an example of what I have in mind.
Unfortunately one of the Druid images has problems on my Apple M1, so I can't run it locally, CI is green but there are 6 tests skipped in that class and sadly it doesn't detail which ones they are.
If you happen to have a machine where the Druid docker images can run you can check https://github.com/zabetak/calcite-druid-dataset to spin the containers up and then run the Druid tests.
In a few days I will get a Linux box where I can run the tests and check.
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4218:
URL: https://github.com/apache/calcite/pull/4218#discussion_r1973664184
##########
druid/src/main/java/org/apache/calcite/adapter/druid/DruidJsonFilter.java:
##########
@@ -294,11 +291,11 @@ abstract class DruidJsonFilter implements DruidJson {
if (columnName == null) {
return null;
}
- if (e.getKind() != SqlKind.NOT_IN) {
+ if (e.getKind() == SqlKind.DRUID_IN) {
return new DruidJsonFilter.JsonInFilter(columnName, listBuilder.build(), extractionFunction);
} else {
- return toNotDruidFilter(
- new DruidJsonFilter.JsonInFilter(columnName, listBuilder.build(), extractionFunction));
+ return new DruidJsonFilter.JsonNotInFilter(columnName,
Review Comment:
I had remove JsonNotInFilter class, and as you suggesstion I thought not need this new class.
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4218:
URL: https://github.com/apache/calcite/pull/4218#discussion_r1973664543
##########
druid/src/main/java/org/apache/calcite/adapter/druid/DruidJsonFilter.java:
##########
@@ -627,6 +625,34 @@ protected JsonInFilter(String dimension, List<String> values,
}
}
+ /**
Review Comment:
Had removed it.
##########
druid/src/test/java/org/apache/calcite/adapter/druid/DruidQueryFilterTest.java:
##########
@@ -87,6 +87,30 @@ class DruidQueryFilterTest {
+ "\"values\":[\"1\",\"5\",\"value1\"]}"));
}
+ @Test void testNotInFilter() throws IOException {
+ final Fixture f = new Fixture();
+ final List<? extends RexNode> listRexNodes =
+ ImmutableList.of(f.rexBuilder.makeInputRef(f.varcharRowType, 0),
+ f.rexBuilder.makeExactLiteral(BigDecimal.valueOf(1)),
+ f.rexBuilder.makeExactLiteral(BigDecimal.valueOf(5)),
+ f.rexBuilder.makeLiteral("value1"));
+
+ RexNode inRexNode =
Review Comment:
Fixed.
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on PR #4218:
URL: https://github.com/apache/calcite/pull/4218#issuecomment-2698145236
> Thanks @xuzifu666 and @suibianwanwank for your investigations, I managed to launch the Druid tests locally and I arrived to the same conclusion, filters are converted to `Sarg`, which is then expanded [here](https://github.com/apache/calcite/blob/91acb99b67dedfbae125509b1a62e2fdf5f81e6a/druid/src/main/java/org/apache/calcite/adapter/druid/DruidJsonFilter.java#L384), which makes it hard to cover the change starting from an SQL query.
>
> Since we have a unit test already covering the change and that the change is trivial I am fine merging as-is.
>
> @xuzifu666 can you squash the commits into a single one and make sure it matches the title of the associated Jira?
Done, thanks for your review~ @asolimando
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on PR #4218:
URL: https://github.com/apache/calcite/pull/4218#issuecomment-2688112551
> At the moment I am not sure there is a problem as `IS_NULL`/`IS_NOT_NULL` is handled exactly the same way (look at `toIsNullKindDruidFilter`), either we have a problem in both or they are correct as-is.
>
> Can you please verify with a test actually running against Druid for both cases?
@asolimando Very thanks for your attention on this pr，I thought again here just a logic error before，due to code here would only be DRUID_IN or DRUID_NOT_IN, so here should judge for SqlKind.DRUID_IN not SqlKind.IN. I had change the logic in PR and add a test for NOT IN case. PTAL~
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4218:
URL: https://github.com/apache/calcite/pull/4218#discussion_r1974981227
##########
druid/src/main/java/org/apache/calcite/adapter/druid/DruidJsonFilter.java:
##########
@@ -294,11 +291,11 @@ abstract class DruidJsonFilter implements DruidJson {
if (columnName == null) {
return null;
}
- if (e.getKind() != SqlKind.NOT_IN) {
+ if (e.getKind() == SqlKind.DRUID_IN) {
return new DruidJsonFilter.JsonInFilter(columnName, listBuilder.build(), extractionFunction);
} else {
- return toNotDruidFilter(
- new DruidJsonFilter.JsonInFilter(columnName, listBuilder.build(), extractionFunction));
+ return new DruidJsonFilter.JsonNotInFilter(columnName,
Review Comment:
We need at least one test to show precisely the issue. The test should not pass before your change, and it should pass after it.
In this way we highlight what the change does, what problem it was there, and we protect the change against future possible regressions.
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adapter transforms an SQL NOT_IN filter to a Druid IN filter [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando closed pull request #4218: [CALCITE-6867] Druid Adapter transforms an SQL NOT_IN filter to a Druid IN filter
URL: https://github.com/apache/calcite/pull/4218
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on PR #4218:
URL: https://github.com/apache/calcite/pull/4218#issuecomment-2692325081
@xuzifu666 @asolimando I added the following test to DruidAdapter2IT and it returned the same results before and after the change. I agree with @xuzifu666 that the original logic here is incorrect, but I'm not too familiar with the code here, and I don't know why the test wasn't affected.
```
@Test void testNotInFilter() {
final String sql = "select \"state_province\", \"city\",\n"
+ " \"product_name\", \"quarter\"\n"
+ "from \"foodmart\"\n"
+ "where \"product_name\" = 'High Top Dried Mushrooms'\n"
+ "and \"quarter\" not in ('Q1', 'Q4')\n"
+ "and \"state_province\" = 'WA'";
final String druidQuery = "{'queryType':'scan',"
+ "'dataSource':'foodmart',"
+ "'intervals':['1900-01-09T00:00:00.000Z/2992-01-10T00:00:00.000Z'],"
+ "'filter':{'type':'and','fields':["
+ "{'type':'selector','dimension':'product_name','value':'High Top Dried Mushrooms'},"
+ "{'type':'not','field':{'type':'selector','dimension':'quarter','value':'Q1'}},"
+ "{'type':'not','field':{'type':'selector','dimension':'quarter','value':'Q4'}},"
+ "{'type':'selector','dimension':'state_province','value':'WA'}]},"
+ "'columns':['state_province','city','product_name','quarter'],"
+ "'resultFormat':'compactedList'}";
final String explain = "PLAN=EnumerableInterpreter\n"
+ " DruidQuery(table=[[foodmart, foodmart]], "
+ "intervals=[[1900-01-09T00:00:00.000Z/2992-01-10T00:00:00.000Z]], "
+ "filter=[AND("
+ "=($3, 'High Top Dried Mushrooms'), "
+ "SEARCH($87, Sarg[(-∞..'Q1':VARCHAR), ('Q1':VARCHAR..'Q4':VARCHAR), ('Q4':VARCHAR..+∞)]:VARCHAR), "
+ "=($30, 'WA'))], "
+ "projects=[[$30, $29, $3, $87]])\n";
sql(sql)
.queryContains(new DruidChecker(druidQuery))
.explainContains(explain)
.returnsUnordered(
"state_province=WA; city=Bremerton; product_name=High Top Dried Mushrooms; quarter=Q3",
"state_province=WA; city=Everett; product_name=High Top Dried Mushrooms; quarter=Q3",
"state_province=WA; city=Kirkland; product_name=High Top Dried Mushrooms; quarter=Q3",
"state_province=WA; city=Lynnwood; product_name=High Top Dried Mushrooms; quarter=Q3",
"state_province=WA; city=Olympia; product_name=High Top Dried Mushrooms; quarter=Q2",
"state_province=WA; city=Port Orchard; product_name=High Top Dried Mushrooms; quarter=Q3",
"state_province=WA; city=Puyallup; product_name=High Top Dried Mushrooms; quarter=Q2",
"state_province=WA; city=Puyallup; product_name=High Top Dried Mushrooms; quarter=Q3",
"state_province=WA; city=Spokane; product_name=High Top Dried Mushrooms; quarter=Q2",
"state_province=WA; city=Spokane; product_name=High Top Dried Mushrooms; quarter=Q3",
"state_province=WA; city=Spokane; product_name=High Top Dried Mushrooms; quarter=Q3",
"state_province=WA; city=Tacoma; product_name=High Top Dried Mushrooms; quarter=Q2",
"state_province=WA; city=Yakima; product_name=High Top Dried Mushrooms; quarter=Q2",
"state_province=WA; city=Yakima; product_name=High Top Dried Mushrooms; quarter=Q3",
"state_province=WA; city=Yakima; product_name=High Top Dried Mushrooms; quarter=Q2");
}
@Test void testNotInFilter1() {
final String sql = "select \"state_province\", \"city\",\n"
+ " \"product_name\", \"quarter\"\n"
+ "from \"foodmart\"\n"
+ "where \"product_name\" = 'High Top Dried Mushrooms'\n"
+ "and \"quarter\" in ('Q1', 'Q4')\n"
+ "and \"state_province\" = 'WA'";
final String druidQuery = "{'queryType':'scan',"
+ "'dataSource':'foodmart',"
+ "'intervals':['1900-01-09T00:00:00.000Z/2992-01-10T00:00:00.000Z'],"
+ "'filter':{'type':'and','fields':["
+ "{'type':'selector','dimension':'product_name','value':'High Top Dried Mushrooms'},"
+ "{'type':'or','fields':[{'type':'selector','dimension':'quarter','value':'Q1'},{'type':'selector','dimension':'quarter','value':'Q4'}]},"
+ "{'type':'selector','dimension':'state_province','value':'WA'}]},"
+ "'columns':['state_province','city','product_name','quarter'],"
+ "'resultFormat':'compactedList'}";
final String explain = "PLAN=EnumerableInterpreter\n"
+ " DruidQuery(table=[[foodmart, foodmart]], "
+ "intervals=[[1900-01-09T00:00:00.000Z/2992-01-10T00:00:00.000Z]], "
+ "filter=[AND("
+ "=($3, 'High Top Dried Mushrooms'), "
+ "SEARCH($87, Sarg['Q1':VARCHAR, 'Q4':VARCHAR]:VARCHAR), "
+ "=($30, 'WA'))], "
+ "projects=[[$30, $29, $3, $87]])\n";
sql(sql)
.queryContains(new DruidChecker(druidQuery))
.explainContains(explain)
.returnsUnordered(
"state_province=WA; city=Bremerton; product_name=High Top Dried Mushrooms; quarter=Q4",
"state_province=WA; city=Olympia; product_name=High Top Dried Mushrooms; quarter=Q4",
"state_province=WA; city=Port Orchard; product_name=High Top Dried Mushrooms; quarter=Q1",
"state_province=WA; city=Puyallup; product_name=High Top Dried Mushrooms; quarter=Q1",
"state_province=WA; city=Spokane; product_name=High Top Dried Mushrooms; quarter=Q1",
"state_province=WA; city=Spokane; product_name=High Top Dried Mushrooms; quarter=Q4",
"state_province=WA; city=Tacoma; product_name=High Top Dried Mushrooms; quarter=Q4",
"state_province=WA; city=Yakima; product_name=High Top Dried Mushrooms; quarter=Q4");
}
```
change:
```
--- a/druid/src/main/java/org/apache/calcite/adapter/druid/DruidJsonFilter.java
+++ b/druid/src/main/java/org/apache/calcite/adapter/druid/DruidJsonFilter.java
@@ -294,7 +294,7 @@ abstract class DruidJsonFilter implements DruidJson {
if (columnName == null) {
return null;
}
- if (e.getKind() != SqlKind.NOT_IN) {
+ if (e.getKind() == SqlKind.DRUID_IN) {
return new DruidJsonFilter.JsonInFilter(columnName, listBuilder.build(), extractionFunction);
} else {
return toNotDruidFilter(
```
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on PR #4218:
URL: https://github.com/apache/calcite/pull/4218#issuecomment-2692660995
I had check all ut about seems not Druid IT case had related to test in or not in.
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4218:
URL: https://github.com/apache/calcite/pull/4218#discussion_r1973561154
##########
druid/src/main/java/org/apache/calcite/adapter/druid/DruidJsonFilter.java:
##########
@@ -627,6 +625,34 @@ protected JsonInFilter(String dimension, List<String> values,
}
}
+ /**
Review Comment:
The two classes are almost identical, I'd suggest to either parameterize a single one or have a common parent class to host the shared parts
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4218:
URL: https://github.com/apache/calcite/pull/4218#issuecomment-2687977217
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4218) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4218&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4218&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4218&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [95.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4218&metric=new_coverage&view=list)
 [35.3% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4218&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4218)
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adaper transform NOT IN filter correctly [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on PR #4218:
URL: https://github.com/apache/calcite/pull/4218#issuecomment-2688097102
@asolimando Very thanks for your attention on this pr，I thought again here just a logic error before，due to code here would only be DRUID_IN or DRUID_NOT_IN, so here should judge for these two type. I had change the logic in 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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6867] Druid Adapter transforms an SQL NOT_IN filter to a Druid IN filter [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4218:
URL: https://github.com/apache/calcite/pull/4218#issuecomment-2698189961
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4218) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4218&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4218&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4218&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4218&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4218&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4218)
--
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@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org