You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/06/11 13:49:13 UTC
[GitHub] [calcite] Aaaaaaron opened a new pull request #2020: [CALCITE-4060] TypeCoercionImpl#inOperationCoercion consider "NOT_IN".
Aaaaaaron opened a new pull request #2020:
URL: https://github.com/apache/calcite/pull/2020
----------------------------------------------------------------
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
[GitHub] [calcite] Aaaaaaron edited a comment on pull request #2020: [CALCITE-4060] Supports implicit type coercion for NOT IN.
Posted by GitBox <gi...@apache.org>.
Aaaaaaron edited a comment on pull request #2020:
URL: https://github.com/apache/calcite/pull/2020#issuecomment-644532678
Execution failed for task ':elasticsearch:test'.
An unstable 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
[GitHub] [calcite] amaliujia commented on pull request #2020: [CALCITE-4060] Supports implicit type coercion for NOT IN.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2020:
URL: https://github.com/apache/calcite/pull/2020#issuecomment-644533587
Yes. You can do a force push to re-trigger tests.
----------------------------------------------------------------
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
[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2020: [CALCITE-4060] Supports implicit type coercion for NOT IN.
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on a change in pull request #2020:
URL: https://github.com/apache/calcite/pull/2020#discussion_r440580558
##########
File path: core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
##########
@@ -61,6 +61,14 @@
+ "from (values (true, true, true))");
}
+ @Test void testNotInOperation() {
+ checkPlanEquals("select\n"
+ + "4 not in ('1', '2', '3') as f0,\n"
+ + "(3, 4) not in (('1', '2')) as f1,\n"
+ + "(2, 3) not in (('1', '2'), ('3', '4')) as f2\n"
+ + "from (values (true, true, true))");
Review comment:
> Trying to understand this test case:
>
> so it tests if value is coerced and the way to check it is doing not in. For example. `4 not in ('1', '2', '3')`. 4 should be coerced to varchar then it is not in `('1', '2', '3')`.
>
> Then, I guess what is missing are negative cases. Because seems to me that even 4 is not coerced, it will still not in `('1', '2', '3')`.
>
> So for each positive case here, there could be a negative case. For example, `3 not in ('1', '2', '3')` which returns false.
Thanks for your advice, I'll modify my test 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
[GitHub] [calcite] amaliujia commented on a change in pull request #2020: [CALCITE-4060] Supports implicit type coercion for NOT IN.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2020:
URL: https://github.com/apache/calcite/pull/2020#discussion_r440575715
##########
File path: core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
##########
@@ -61,6 +61,14 @@
+ "from (values (true, true, true))");
}
+ @Test void testNotInOperation() {
+ checkPlanEquals("select\n"
+ + "4 not in ('1', '2', '3') as f0,\n"
+ + "(3, 4) not in (('1', '2')) as f1,\n"
+ + "(2, 3) not in (('1', '2'), ('3', '4')) as f2\n"
+ + "from (values (true, true, true))");
Review comment:
Trying to understand this test case:
so it tests if value is coerced and the way to check it is doing not_in. For example. `4 not in ('1', '2', '3')`. 4 should be coerced to varchar then it is not in `('1', '2', '3')`.
Then, I guess what is missing is negative cases. Because seems to me that even 4 is coerced, it will still not in ('1', '2', '3').
So for each positive case here, there could be a negative case. For example, `3 not in ('1', '2', '3')` which returns false.
----------------------------------------------------------------
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
[GitHub] [calcite] Aaaaaaron commented on a change in pull request #2020: [CALCITE-4060] Supports implicit type coercion for NOT IN.
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on a change in pull request #2020:
URL: https://github.com/apache/calcite/pull/2020#discussion_r440580558
##########
File path: core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
##########
@@ -61,6 +61,14 @@
+ "from (values (true, true, true))");
}
+ @Test void testNotInOperation() {
+ checkPlanEquals("select\n"
+ + "4 not in ('1', '2', '3') as f0,\n"
+ + "(3, 4) not in (('1', '2')) as f1,\n"
+ + "(2, 3) not in (('1', '2'), ('3', '4')) as f2\n"
+ + "from (values (true, true, true))");
Review comment:
> Trying to understand this test case:
>
> so it tests if value is coerced and the way to check it is doing not in. For example. `4 not in ('1', '2', '3')`. 4 should be coerced to varchar then it is not in `('1', '2', '3')`.
>
> Then, I guess what is missing are negative cases. Because seems to me that even 4 is not coerced, it will still not in `('1', '2', '3')`.
>
> So for each positive case here, there could be a negative case. For example, `3 not in ('1', '2', '3')` which returns false.
Thanks for your advice, I've change my test 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
[GitHub] [calcite] Aaaaaaron edited a comment on pull request #2020: [CALCITE-4060] Supports implicit type coercion for NOT IN.
Posted by GitBox <gi...@apache.org>.
Aaaaaaron edited a comment on pull request #2020:
URL: https://github.com/apache/calcite/pull/2020#issuecomment-643903627
> Thanks for the contribution, can we change the title to "Supports implicit type coercion for NOT IN" which is more readable.
@danny0405 Thanks for your advice, I've changed the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on pull request #2020: [CALCITE-4060] TypeCoercionImpl#inOperationCoercion consider "NOT_IN".
Posted by GitBox <gi...@apache.org>.
danny0405 commented on pull request #2020:
URL: https://github.com/apache/calcite/pull/2020#issuecomment-643150807
Thanks for the contribution, can we change the title to "Supports implicit type coercion for NOT IN" which is more readable.
----------------------------------------------------------------
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
[GitHub] [calcite] chunweilei merged pull request #2020: [CALCITE-4060] Supports implicit type coercion for NOT IN.
Posted by GitBox <gi...@apache.org>.
chunweilei merged pull request #2020:
URL: https://github.com/apache/calcite/pull/2020
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on a change in pull request #2020: [CALCITE-4060] Supports implicit type coercion for NOT IN.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2020:
URL: https://github.com/apache/calcite/pull/2020#discussion_r440575715
##########
File path: core/src/test/java/org/apache/calcite/test/TypeCoercionConverterTest.java
##########
@@ -61,6 +61,14 @@
+ "from (values (true, true, true))");
}
+ @Test void testNotInOperation() {
+ checkPlanEquals("select\n"
+ + "4 not in ('1', '2', '3') as f0,\n"
+ + "(3, 4) not in (('1', '2')) as f1,\n"
+ + "(2, 3) not in (('1', '2'), ('3', '4')) as f2\n"
+ + "from (values (true, true, true))");
Review comment:
Trying to understand this test case:
so it tests if value is coerced and the way to check it is doing not in. For example. `4 not in ('1', '2', '3')`. 4 should be coerced to varchar then it is not in `('1', '2', '3')`.
Then, I guess what is missing are negative cases. Because seems to me that even 4 is not coerced, it will still not in `('1', '2', '3')`.
So for each positive case here, there could be a negative case. For example, `3 not in ('1', '2', '3')` which returns false.
----------------------------------------------------------------
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
[GitHub] [calcite] amaliujia commented on pull request #2020: [CALCITE-4060] Supports implicit type coercion for NOT IN.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2020:
URL: https://github.com/apache/calcite/pull/2020#issuecomment-644531080
+1
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] Aaaaaaron commented on pull request #2020: [CALCITE-4060] Supports implicit type coercion for NOT IN.
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on pull request #2020:
URL: https://github.com/apache/calcite/pull/2020#issuecomment-644532678
Execution failed for task ':elasticsearch:test'.
Is this an unstable ut?
----------------------------------------------------------------
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
[GitHub] [calcite] Aaaaaaron commented on pull request #2020: [CALCITE-4060] TypeCoercionImpl#inOperationCoercion consider "NOT_IN".
Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on pull request #2020:
URL: https://github.com/apache/calcite/pull/2020#issuecomment-643903627
> Thanks for the contribution, can we change the title to "Supports implicit type coercion for NOT IN" which is more readable.
Thanks for your advice!
----------------------------------------------------------------
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