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 2021/04/01 11:38:56 UTC
[GitHub] [druid] chenyuzhi459 opened a new pull request #11060: fix lookup nullable
chenyuzhi459 opened a new pull request #11060:
URL: https://github.com/apache/druid/pull/11060
<!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
<!-- Please read the doc for contribution (https://github.com/apache/druid/blob/master/CONTRIBUTING.md) before making this PR. Also, once you open a PR, please _avoid using force pushes and rebasing_ since these make it difficult for reviewers to see what you've changed in response to their reviews. See [the 'If your pull request shows conflicts with master' section](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master) for more details. -->
Fixes #11038.
### Description
When we use `lookup` func in sql, we will return null value if we don't found. However, in `org.apache.druid.sql.calcite.expression.builtin.QueryLookupOperatorConversion`, it's returnType was defined as `returnTypeNonNull`, which casue a validation error in calcites.
### Solution
Change lookup func's return as nullable with `returnTypeNonNull`.
##### Key changed/added classes in this PR
* `org.apache.druid.sql.calcite.expression.builtin.QueryLookupOperatorConversion`
<hr>
This PR has:
- [x] been self-reviewed.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage]
- [x] 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.
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] clintropolis commented on a change in pull request #11060: fix lookup nullable
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11060:
URL: https://github.com/apache/druid/pull/11060#discussion_r606168284
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -17086,4 +17086,32 @@ public void testGroupingSetsWithLimitOrderByGran() throws Exception
).build()
);
}
+
+
+ @Test
+ public void testLookupWithNull() throws Exception
+ {
+ testQuery(
+ "SELECT dim2 ,lookup(dim2,'lookyloo') from foo where dim2 is null",
+ ImmutableList.of(
+ new Druids.ScanQueryBuilder()
+ .dataSource(CalciteTests.DATASOURCE1)
+ .intervals(querySegmentSpec(Filtration.eternity()))
+ .virtualColumns(
+ expressionVirtualColumn("v0", "null", ValueType.STRING)
+ )
+ .columns("v0")
+ .legacy(false)
+ .filters(new SelectorDimFilter("dim2", "", null))
+ .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST)
+ .context(QUERY_CONTEXT_DEFAULT)
+ .build()
+ ),
+ ImmutableList.<Object[]>builder().add(
+ new Object[]{"", NULL_STRING},
+ new Object[]{"", NULL_STRING},
+ new Object[]{"", NULL_STRING}
Review comment:
I restarted CI for the failing test. Looking at the new test though, I'm not sure it tests the issue anymore since the input to the lookup is no longer null? I actually think maybe the old test was better, you just needed to adjust the expected output based on whether or not it is in default null handling mode or not.e.g
```
List<Object[]> expected;
if (useDefault) {
expected = ImmutableList.<Object[]>builder().add(
...
```
like some of the other tests are doing.
--
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.
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] clintropolis merged pull request #11060: fix lookup nullable
Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #11060:
URL: https://github.com/apache/druid/pull/11060
--
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.
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] chenyuzhi459 commented on a change in pull request #11060: fix lookup nullable
Posted by GitBox <gi...@apache.org>.
chenyuzhi459 commented on a change in pull request #11060:
URL: https://github.com/apache/druid/pull/11060#discussion_r606082928
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -17086,4 +17086,32 @@ public void testGroupingSetsWithLimitOrderByGran() throws Exception
).build()
);
}
+
+
+ @Test
+ public void testLookupWithNull() throws Exception
+ {
+ testQuery(
+ "SELECT dim2 ,lookup(dim2,'lookyloo') from foo where dim2 is null",
+ ImmutableList.of(
+ new Druids.ScanQueryBuilder()
+ .dataSource(CalciteTests.DATASOURCE1)
+ .intervals(querySegmentSpec(Filtration.eternity()))
+ .virtualColumns(
+ expressionVirtualColumn("v0", "null", ValueType.STRING)
+ )
+ .columns("v0")
+ .legacy(false)
+ .filters(new SelectorDimFilter("dim2", "", null))
+ .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST)
+ .context(QUERY_CONTEXT_DEFAULT)
+ .build()
+ ),
+ ImmutableList.<Object[]>builder().add(
+ new Object[]{"", NULL_STRING},
+ new Object[]{"", NULL_STRING},
+ new Object[]{"", NULL_STRING}
Review comment:
Thanks for reminding me. I have chang dimension `dim2` to `dim1` to fix it
--
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.
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] chenyuzhi459 commented on a change in pull request #11060: fix lookup nullable
Posted by GitBox <gi...@apache.org>.
chenyuzhi459 commented on a change in pull request #11060:
URL: https://github.com/apache/druid/pull/11060#discussion_r606203079
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -17086,4 +17086,32 @@ public void testGroupingSetsWithLimitOrderByGran() throws Exception
).build()
);
}
+
+
+ @Test
+ public void testLookupWithNull() throws Exception
+ {
+ testQuery(
+ "SELECT dim2 ,lookup(dim2,'lookyloo') from foo where dim2 is null",
+ ImmutableList.of(
+ new Druids.ScanQueryBuilder()
+ .dataSource(CalciteTests.DATASOURCE1)
+ .intervals(querySegmentSpec(Filtration.eternity()))
+ .virtualColumns(
+ expressionVirtualColumn("v0", "null", ValueType.STRING)
+ )
+ .columns("v0")
+ .legacy(false)
+ .filters(new SelectorDimFilter("dim2", "", null))
+ .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST)
+ .context(QUERY_CONTEXT_DEFAULT)
+ .build()
+ ),
+ ImmutableList.<Object[]>builder().add(
+ new Object[]{"", NULL_STRING},
+ new Object[]{"", NULL_STRING},
+ new Object[]{"", NULL_STRING}
Review comment:
ok, I think you are right , we should consider specail situation.
I hava changed it to old test, and handled `null` case.
--
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.
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] chenyuzhi459 commented on a change in pull request #11060: fix lookup nullable
Posted by GitBox <gi...@apache.org>.
chenyuzhi459 commented on a change in pull request #11060:
URL: https://github.com/apache/druid/pull/11060#discussion_r606082928
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -17086,4 +17086,32 @@ public void testGroupingSetsWithLimitOrderByGran() throws Exception
).build()
);
}
+
+
+ @Test
+ public void testLookupWithNull() throws Exception
+ {
+ testQuery(
+ "SELECT dim2 ,lookup(dim2,'lookyloo') from foo where dim2 is null",
+ ImmutableList.of(
+ new Druids.ScanQueryBuilder()
+ .dataSource(CalciteTests.DATASOURCE1)
+ .intervals(querySegmentSpec(Filtration.eternity()))
+ .virtualColumns(
+ expressionVirtualColumn("v0", "null", ValueType.STRING)
+ )
+ .columns("v0")
+ .legacy(false)
+ .filters(new SelectorDimFilter("dim2", "", null))
+ .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST)
+ .context(QUERY_CONTEXT_DEFAULT)
+ .build()
+ ),
+ ImmutableList.<Object[]>builder().add(
+ new Object[]{"", NULL_STRING},
+ new Object[]{"", NULL_STRING},
+ new Object[]{"", NULL_STRING}
Review comment:
Thanks for reminding me. I have chang dimension `dim2` to `dim1` to solution this problem
--
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.
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] chenyuzhi459 commented on a change in pull request #11060: fix lookup nullable
Posted by GitBox <gi...@apache.org>.
chenyuzhi459 commented on a change in pull request #11060:
URL: https://github.com/apache/druid/pull/11060#discussion_r606082928
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -17086,4 +17086,32 @@ public void testGroupingSetsWithLimitOrderByGran() throws Exception
).build()
);
}
+
+
+ @Test
+ public void testLookupWithNull() throws Exception
+ {
+ testQuery(
+ "SELECT dim2 ,lookup(dim2,'lookyloo') from foo where dim2 is null",
+ ImmutableList.of(
+ new Druids.ScanQueryBuilder()
+ .dataSource(CalciteTests.DATASOURCE1)
+ .intervals(querySegmentSpec(Filtration.eternity()))
+ .virtualColumns(
+ expressionVirtualColumn("v0", "null", ValueType.STRING)
+ )
+ .columns("v0")
+ .legacy(false)
+ .filters(new SelectorDimFilter("dim2", "", null))
+ .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST)
+ .context(QUERY_CONTEXT_DEFAULT)
+ .build()
+ ),
+ ImmutableList.<Object[]>builder().add(
+ new Object[]{"", NULL_STRING},
+ new Object[]{"", NULL_STRING},
+ new Object[]{"", NULL_STRING}
Review comment:
Thanks for reminding me. I have changed dimension `dim2` to `dim1` to fix it.
It seems failing CI agained because of some network trouble, should I fix it ?
--
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.
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] clintropolis commented on a change in pull request #11060: fix lookup nullable
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11060:
URL: https://github.com/apache/druid/pull/11060#discussion_r605983922
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -17086,4 +17086,32 @@ public void testGroupingSetsWithLimitOrderByGran() throws Exception
).build()
);
}
+
+
+ @Test
+ public void testLookupWithNull() throws Exception
+ {
+ testQuery(
+ "SELECT dim2 ,lookup(dim2,'lookyloo') from foo where dim2 is null",
+ ImmutableList.of(
+ new Druids.ScanQueryBuilder()
+ .dataSource(CalciteTests.DATASOURCE1)
+ .intervals(querySegmentSpec(Filtration.eternity()))
+ .virtualColumns(
+ expressionVirtualColumn("v0", "null", ValueType.STRING)
+ )
+ .columns("v0")
+ .legacy(false)
+ .filters(new SelectorDimFilter("dim2", "", null))
+ .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST)
+ .context(QUERY_CONTEXT_DEFAULT)
+ .build()
+ ),
+ ImmutableList.<Object[]>builder().add(
+ new Object[]{"", NULL_STRING},
+ new Object[]{"", NULL_STRING},
+ new Object[]{"", NULL_STRING}
Review comment:
It looks like this is failing CI in SQL compatible null handling mode because the expected results are different, where there are only 2 null valued `dim2` rows instead of 3. The data for the 'foo' datasource is here: https://github.com/apache/druid/blob/master/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java#L368. In default mode (`druid.generic.useDefaultValueForNull=true`), the empty string is equivalent to null, so https://github.com/apache/druid/blob/master/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java#L390 is treated as null, but when `druid.generic.useDefaultValueForNull=false`, only https://github.com/apache/druid/blob/master/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java#L382 and https://github.com/apache/druid/blob/master/sql/src/test/java/org/apache/druid/sql/calcite/util/CalciteTests.java#L409 have null valued `dim2`. Additionally, `dim2` would be `null` instead of the empty string in the results.
--
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.
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