Re: [PR] [CALCITE-5387] Type-mismatch on nullability in JoinPushTransitivePredicatesRule RelRule [calcite]
ian-bertolacci commented on code in PR #3452: URL: https://github.com/apache/calcite/pull/3452#discussion_r1350768855 ## core/src/main/java/org/apache/calcite/rel/metadata/RelMdPredicates.java: ## @@ -666,6 +666,29 @@ static class JoinConditionBasedPredicateInference { equivalence = BitSets.closure(equivalence); } +/** + * As RexPermuteInputsShuttle, with one exception. When visiting an inputRef, + * it will replace the type of the InputRef with the type found in the input fields, + * instead of keeping the original type. This is used within + * when generating the Left/RightInferredPredicates, to avoid nullability mismatches + * between the types of the join and the types of the inputs. + */ +private class TypeChangingRexPermuteInputsShuttle +extends org.apache.calcite.rex.RexPermuteInputsShuttle { Review Comment: Maybe there are situations where fields is not empty, but also not the same size as the number of input columns? I could see something like this working: ```java RelDataType dataType = target < fields.size() ? local.getType() : fields.get(target).getType(); ``` -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-5387] Type-mismatch on nullability in JoinPushTransitivePredicatesRule RelRule [calcite]
LakeShen commented on PR #3452: URL: https://github.com/apache/calcite/pull/3452#issuecomment-1748326807 More details could see developing Calcite guide:https://calcite.apache.org/develop/ -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-5387] Type-mismatch on nullability in JoinPushTransitivePredicatesRule RelRule [calcite]
sonarcloud[bot] commented on PR #3452: URL: https://github.com/apache/calcite/pull/3452#issuecomment-1747643124 Kudos, SonarCloud Quality Gate passed! [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=apache_calcite=3452) [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_calcite=3452=false=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite=3452=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3452=false=BUG) [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_calcite=3452=false=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite=3452=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3452=false=VULNERABILITY) [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3452=false=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3452=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3452=false=SECURITY_HOTSPOT) [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_calcite=3452=false=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite=3452=false=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_calcite=3452=false=CODE_SMELL) [![92.9%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90-16px.png '92.9%')](https://sonarcloud.io/component_measures?id=apache_calcite=3452=new_coverage=list) [92.9% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3452=new_coverage=list) [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_calcite=3452=new_duplicated_lines_density=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3452=new_duplicated_lines_density=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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-5387] Type-mismatch on nullability in JoinPushTransitivePredicatesRule RelRule [calcite]
keatn commented on code in PR #3452: URL: https://github.com/apache/calcite/pull/3452#discussion_r1346428537 ## core/src/main/java/org/apache/calcite/rel/metadata/RelMdPredicates.java: ## @@ -666,6 +666,29 @@ static class JoinConditionBasedPredicateInference { equivalence = BitSets.closure(equivalence); } +/** + * As RexPermuteInputsShuttle, with one exception. When visiting an inputRef, + * it will replace the type of the InputRef with the type found in the input fields, + * instead of keeping the original type. This is used within + * when generating the Left/RightInferredPredicates, to avoid nullability mismatches + * between the types of the join and the types of the inputs. + */ +private class TypeChangingRexPermuteInputsShuttle +extends org.apache.calcite.rex.RexPermuteInputsShuttle { + + TypeChangingRexPermuteInputsShuttle(Mappings.TargetMapping mapping, RelNode... inputs) { +super(mapping, inputs); + } + + @Override public RexNode visitInputRef(RexInputRef local) { +final int index = local.getIndex(); +int target = mapping.getTarget(index); +return new RexInputRef( +target, +fields.get(target).getType()); Review Comment: I think in order to check that the two types are equivalent except for nullability, I would need to have access to a typefactory, so I could do something like `typefatory.createTypeWithNullability(t1, t2.isNullable()).equals(t1)`. For now, I added a sanity check to confirm that the two types are of the same type family. I'm not certain if this is a good check for compatibility, and am open to changing 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-5387] Type-mismatch on nullability in JoinPushTransitivePredicatesRule RelRule [calcite]
keatn commented on code in PR #3452: URL: https://github.com/apache/calcite/pull/3452#discussion_r1346428283 ## core/src/main/java/org/apache/calcite/rel/metadata/RelMdPredicates.java: ## @@ -666,6 +666,29 @@ static class JoinConditionBasedPredicateInference { equivalence = BitSets.closure(equivalence); } +/** + * As RexPermuteInputsShuttle, with one exception. When visiting an inputRef, + * it will replace the type of the InputRef with the type found in the input fields, + * instead of keeping the original type. This is used within + * when generating the Left/RightInferredPredicates, to avoid nullability mismatches + * between the types of the join and the types of the inputs. + */ +private class TypeChangingRexPermuteInputsShuttle +extends org.apache.calcite.rex.RexPermuteInputsShuttle { Review Comment: I tried doing something like this in `RexPermuteInputsShuttle` ``` if (fields.isEmpty()) { return new RexInputRef( target, local.getType()); } else { return new RexInputRef( target, fields.get(target).getType()); } ``` (Note that the if statement is needed because it's valid to create a `RexPermuteInputsShuttle` without any backing fields.) This caused several tests to fail with index out of bound errors. The reason wasn't obvious to me. I'll do a bit of poking around and see if I can figure out why. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org