You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "YasuoStudyJava (via GitHub)" <gi...@apache.org> on 2023/06/06 08:04:14 UTC
[GitHub] [calcite] YasuoStudyJava opened a new pull request, #3247: [CALCITE-5733][core]Simplification should also simplify for array type
YasuoStudyJava opened a new pull request, #3247:
URL: https://github.com/apache/calcite/pull/3247
*Simplification seem to not take into account that the specified field is of array type. In other words，it can simplify "a = 1 AND a = 2" to "false"，but can not simplify “a = ARRAY[1,2] AND a = ARRAY[2,3]” to "false". This change can fix it.*
*Linked to https://issues.apache.org/jira/browse/FLINK-32188, when submitting an SQL task with Flink to test a customized data source connector, I specified to query an array-type field of a temporary table with a fixed-value array. For example, "select * from image-source where URL=ARRAY ['/flink. jpg', '/flink_1. jpg'] AND URL=ARRAY ['/f. jpg', '/f_1. jpg']" can obtain two predicate conditions, this is illogical. Generally speaking, simplifying this SQL condition should not result in any predicates. Changes related to “RexSimplify.java” can 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zoudan commented on a diff in pull request #3247: [CALCITE-5733] Simplify "a = ARRAY[1,2] AND a = ARRAY[2,3]" to "false"
Posted by "zoudan (via GitHub)" <gi...@apache.org>.
zoudan commented on code in PR #3247:
URL: https://github.com/apache/calcite/pull/3247#discussion_r1226169010
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -672,9 +675,28 @@ private void simplifyAndTerms(List<RexNode> terms, RexUnknownAs unknownAs) {
RexSimplify simplify = this;
for (int i = 0; i < terms.size(); i++) {
RexNode t = terms.get(i);
- if (Predicate.of(t) == null) {
+ Predicate predicate = Predicate.of(t);
+ if (predicate == null) {
continue;
}
+
+ //add for array-type
+ if (predicate instanceof Comparison) {
+ Comparison cmp = (Comparison) predicate;
+ if (cmp.rexCall != null && cmp.rexCall.getKind().equals(SqlKind.ARRAY_VALUE_CONSTRUCTOR)) {
Review Comment:
`SqlKind` is an enum type, you could use `==`
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -2660,6 +2695,24 @@ private Comparison(RexNode ref, SqlKind kind, RexLiteral literal) {
return new Comparison(left, e.getKind(), (RexLiteral) right);
}
break;
+ //add for RexCall like "CAST(ARRAY(..., ...))..."
+ case CAST:
+ final RexCall castCall = (RexCall) right;
+ final RexNode castLeft = castCall.getOperands().get(0);
+ switch (castLeft.getKind()) {
+ case ARRAY_VALUE_CONSTRUCTOR:
+ //array-type RexCall --> create a comparison of RexCall
+ if (nodePredicate.test(left)) {
+ return new Comparison(left, e.getKind(), (RexCall) castLeft);
+ }
+ }
+ break;
+ case ARRAY_VALUE_CONSTRUCTOR:
+ //array-type RexCall --> create a comparison of RexCall
Review Comment:
It is better to reuse the duplicate code.
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -2669,10 +2722,28 @@ private Comparison(RexNode ref, SqlKind kind, RexLiteral literal) {
return new Comparison(right, e.getKind().reverse(), (RexLiteral) left);
}
break;
+ //add for RexCall like "CAST(ARRAY(..., ...))..."
+ case CAST:
+ final RexCall castCall = (RexCall) left;
+ final RexNode castLeft = castCall.getOperands().get(0);
+ switch (castLeft.getKind()) {
+ case ARRAY_VALUE_CONSTRUCTOR:
+ //array-type RexCall --> create a comparison of RexCall
+ if (nodePredicate.test(right)) {
+ return new Comparison(right, e.getKind().reverse(), (RexCall) castLeft);
+ }
+ }
+ break;
+ case ARRAY_VALUE_CONSTRUCTOR:
+ //array-type RexCall --> create a comparison of RexCall
+ if (nodePredicate.test(right)) {
+ return new Comparison(right, e.getKind().reverse(), (RexCall) left);
+ }
+ break;
default:
break;
}
- break;
+ break;
Review Comment:
Do not add spaces at the end of lines
--
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
[GitHub] [calcite] YasuoStudyJava commented on pull request #3247: [CALCITE-5733] Simplify "a = ARRAY[1,2] AND a = ARRAY[2,3]" to "false"
Posted by "YasuoStudyJava (via GitHub)" <gi...@apache.org>.
YasuoStudyJava commented on PR #3247:
URL: https://github.com/apache/calcite/pull/3247#issuecomment-1614045307
>
Can you give an example?
--
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
[GitHub] [calcite] herunkang2018 commented on pull request #3247: [CALCITE-5733] Simplify "a = ARRAY[1,2] AND a = ARRAY[2,3]" to "false"
Posted by "herunkang2018 (via GitHub)" <gi...@apache.org>.
herunkang2018 commented on PR #3247:
URL: https://github.com/apache/calcite/pull/3247#issuecomment-1615485565
@YasuoStudyJava there are similar tests in `RexProgramTest`, and you could learn from that.
--
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
[GitHub] [calcite] YasuoStudyJava commented on pull request #3247: [CALCITE-5733]Simplify "a= ARRAY[1,2] AND a = ARRAY[2,3]" to "false"
Posted by "YasuoStudyJava (via GitHub)" <gi...@apache.org>.
YasuoStudyJava commented on PR #3247:
URL: https://github.com/apache/calcite/pull/3247#issuecomment-1583791696
>
OK, I changed it, could you please take a look again to see if it is standardized.
--
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
[GitHub] [calcite] ILuffZhe commented on pull request #3247: [CALCITE-5733] Simplify "a = ARRAY[1,2] AND a = ARRAY[2,3]" to "false"
Posted by "ILuffZhe (via GitHub)" <gi...@apache.org>.
ILuffZhe commented on PR #3247:
URL: https://github.com/apache/calcite/pull/3247#issuecomment-1584126536
> > > Can you also please add a unit test here? It helps reviewers to understand better.
> >
> >
> > I'll have a try to add a test.
>
> BTW, I seem to not find pom.xml in source code of calcite? @ILuffZhe
We now use Gradle in Calcite, and you can find `build.gradle.kts` and `gradle.properties` in root dir.
For more information, you can browse the [website](https://calcite.apache.org/develop/#download-source-build-and-run-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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on a diff in pull request #3247: [CALCITE-5733] Simplify "a = ARRAY[1,2] AND a = ARRAY[2,3]" to "false"
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3247:
URL: https://github.com/apache/calcite/pull/3247#discussion_r1228811712
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -2622,21 +2644,34 @@ private static class VariableCollector extends RexVisitorImpl<Void> {
/** Represents a simple Comparison.
*
- * <p>Left hand side is a {@link RexNode}, right hand side is a literal.
+ * <p>Left hand side is a {@link RexNode}, right hand side is a literal/array-call.
*/
private static class Comparison implements Predicate {
final RexNode ref;
final SqlKind kind;
+ //RexCall or RexLiteral
Review Comment:
It might be better to change the `RexLiteral literal` field to `RexNode argument`, and not create a separate `RexCall rexCall` field. If there's one field we can check that it is not null.
--
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
[GitHub] [calcite] ILuffZhe commented on pull request #3247: [CALCITE-5733][core]Simplification should also simplify for array type
Posted by "ILuffZhe (via GitHub)" <gi...@apache.org>.
ILuffZhe commented on PR #3247:
URL: https://github.com/apache/calcite/pull/3247#issuecomment-1583684754
Hi @YasuoStudyJava. The PR's headline should be formatted like "[CALCITE-5733] YOUR JIRA ISSUE SUMMARY", you can take a look at other PRs. Or the recently merged [PR](https://github.com/apache/calcite/pull/3250) is also a good help to refer to.
--
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
[GitHub] [calcite] herunkang2018 commented on pull request #3247: [CALCITE-5733] Simplify "a = ARRAY[1,2] AND a = ARRAY[2,3]" to "false"
Posted by "herunkang2018 (via GitHub)" <gi...@apache.org>.
herunkang2018 commented on PR #3247:
URL: https://github.com/apache/calcite/pull/3247#issuecomment-1608538325
@YasuoStudyJava we should consider the case that the `RexCall` in projection, which the default RexUnknownAs is UNKNOWN, not the same as predicate in where clause.
--
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
[GitHub] [calcite] YasuoStudyJava commented on a diff in pull request #3247: [CALCITE-5733] Simplify "a = ARRAY[1,2] AND a = ARRAY[2,3]" to "false"
Posted by "YasuoStudyJava (via GitHub)" <gi...@apache.org>.
YasuoStudyJava commented on code in PR #3247:
URL: https://github.com/apache/calcite/pull/3247#discussion_r1226182776
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -2660,6 +2695,24 @@ private Comparison(RexNode ref, SqlKind kind, RexLiteral literal) {
return new Comparison(left, e.getKind(), (RexLiteral) right);
}
break;
+ //add for RexCall like "CAST(ARRAY(..., ...))..."
+ case CAST:
+ final RexCall castCall = (RexCall) right;
+ final RexNode castLeft = castCall.getOperands().get(0);
+ switch (castLeft.getKind()) {
+ case ARRAY_VALUE_CONSTRUCTOR:
+ //array-type RexCall --> create a comparison of RexCall
+ if (nodePredicate.test(left)) {
+ return new Comparison(left, e.getKind(), (RexCall) castLeft);
+ }
+ }
+ break;
+ case ARRAY_VALUE_CONSTRUCTOR:
+ //array-type RexCall --> create a comparison of RexCall
Review Comment:
@zoudan OK, Thank you. I will take your suggestion later on. Now I am spending time on adding a unit test，but gradle is a bit unfamiliar to me. Maybe once I have configured the gradle, I can test the 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] YasuoStudyJava commented on pull request #3247: [CALCITE-5733] Simplify "a = ARRAY[1,2] AND a = ARRAY[2,3]" to "false"
Posted by "YasuoStudyJava (via GitHub)" <gi...@apache.org>.
YasuoStudyJava commented on PR #3247:
URL: https://github.com/apache/calcite/pull/3247#issuecomment-1616710831
> @YasuoStudyJava there are similar tests in `RexProgramTest`, and you could learn from that.
yeah，I know，but I met some problems when I pull the code and build in my IDEA. I am quite unfamiliar with ‘Gradle’, So code format correction and test cases have not started. Now the problem is:
> Task :buildSrc:javacc:compileKotlin
Could not load entry 89875aab070c1135c15229026edb1371 from remote build cache: Bucket 'calcite-gradle-cache' not found
e: file:///E:/calcite/calcite/buildSrc/subprojects/javacc/src/main/kotlin/org/apache/calcite/buildtools/javacc/JavaCCTask.kt:66:13 Unresolved reference: main
Task :buildSrc:javacc:compileKotlin FAILED
--
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
[GitHub] [calcite] YasuoStudyJava commented on a diff in pull request #3247: [CALCITE-5733] Simplify "a = ARRAY[1,2] AND a = ARRAY[2,3]" to "false"
Posted by "YasuoStudyJava (via GitHub)" <gi...@apache.org>.
YasuoStudyJava commented on code in PR #3247:
URL: https://github.com/apache/calcite/pull/3247#discussion_r1226182776
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -2660,6 +2695,24 @@ private Comparison(RexNode ref, SqlKind kind, RexLiteral literal) {
return new Comparison(left, e.getKind(), (RexLiteral) right);
}
break;
+ //add for RexCall like "CAST(ARRAY(..., ...))..."
+ case CAST:
+ final RexCall castCall = (RexCall) right;
+ final RexNode castLeft = castCall.getOperands().get(0);
+ switch (castLeft.getKind()) {
+ case ARRAY_VALUE_CONSTRUCTOR:
+ //array-type RexCall --> create a comparison of RexCall
+ if (nodePredicate.test(left)) {
+ return new Comparison(left, e.getKind(), (RexCall) castLeft);
+ }
+ }
+ break;
+ case ARRAY_VALUE_CONSTRUCTOR:
+ //array-type RexCall --> create a comparison of RexCall
Review Comment:
@zoudan Thank you. Maybe I will take your suggestion later on.
--
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
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3247: [CALCITE-5733][core]Simplification should also simplify for array type
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3247:
URL: https://github.com/apache/calcite/pull/3247#issuecomment-1578185953
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3247)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3247&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3247&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3247&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3247&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3247&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3247&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3247&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3247&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3247&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3247&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3247&resolved=false&types=CODE_SMELL) [12 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3247&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3247&metric=new_coverage&view=list) [37.3% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3247&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3247&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3247&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [calcite] herunkang2018 commented on a diff in pull request #3247: [CALCITE-5733] Simplify "a = ARRAY[1,2] AND a = ARRAY[2,3]" to "false"
Posted by "herunkang2018 (via GitHub)" <gi...@apache.org>.
herunkang2018 commented on code in PR #3247:
URL: https://github.com/apache/calcite/pull/3247#discussion_r1242994446
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -672,9 +675,28 @@ private void simplifyAndTerms(List<RexNode> terms, RexUnknownAs unknownAs) {
RexSimplify simplify = this;
for (int i = 0; i < terms.size(); i++) {
RexNode t = terms.get(i);
- if (Predicate.of(t) == null) {
+ Predicate predicate = Predicate.of(t);
+ if (predicate == null) {
continue;
}
+
+ //add for array-type
Review Comment:
Better to add a blank to `//` and `add` for line comment.
--
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
[GitHub] [calcite] YasuoStudyJava commented on a diff in pull request #3247: [CALCITE-5733] Simplify "a = ARRAY[1,2] AND a = ARRAY[2,3]" to "false"
Posted by "YasuoStudyJava (via GitHub)" <gi...@apache.org>.
YasuoStudyJava commented on code in PR #3247:
URL: https://github.com/apache/calcite/pull/3247#discussion_r1247358366
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -672,9 +675,28 @@ private void simplifyAndTerms(List<RexNode> terms, RexUnknownAs unknownAs) {
RexSimplify simplify = this;
for (int i = 0; i < terms.size(); i++) {
RexNode t = terms.get(i);
- if (Predicate.of(t) == null) {
+ Predicate predicate = Predicate.of(t);
+ if (predicate == null) {
continue;
}
+
+ //add for array-type
Review Comment:
yeah, thank you!
--
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
[GitHub] [calcite] YasuoStudyJava commented on pull request #3247: [CALCITE-5733] Simplify "a = ARRAY[1,2] AND a = ARRAY[2,3]" to "false"
Posted by "YasuoStudyJava (via GitHub)" <gi...@apache.org>.
YasuoStudyJava commented on PR #3247:
URL: https://github.com/apache/calcite/pull/3247#issuecomment-1584116139
> > Can you also please add a unit test here? It helps reviewers to understand better.
>
> I'll have a try to add a test.
BTW, I seem to not find pom.xml in source code of calcite?
--
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
[GitHub] [calcite] ILuffZhe commented on pull request #3247: [CALCITE-5733] Simplify "a = ARRAY[1,2] AND a = ARRAY[2,3]" to "false"
Posted by "ILuffZhe (via GitHub)" <gi...@apache.org>.
ILuffZhe commented on PR #3247:
URL: https://github.com/apache/calcite/pull/3247#issuecomment-1583804365
Can you also please add a unit test here? It helps reviewers to understand better.
--
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
[GitHub] [calcite] YasuoStudyJava commented on pull request #3247: [CALCITE-5733] Simplify "a = ARRAY[1,2] AND a = ARRAY[2,3]" to "false"
Posted by "YasuoStudyJava (via GitHub)" <gi...@apache.org>.
YasuoStudyJava commented on PR #3247:
URL: https://github.com/apache/calcite/pull/3247#issuecomment-1583910315
> Can you also please add a unit test here? It helps reviewers to understand better.
I'll have a try to add 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