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 2022/07/08 14:48:21 UTC
[GitHub] [calcite] abhishek-das-gupta opened a new pull request, #2853: [CALCITE-4632] SARG datatype should be less restictive than any of the input types
abhishek-das-gupta opened a new pull request, #2853:
URL: https://github.com/apache/calcite/pull/2853
Instead of first row expression's datatype, SARG datatype should consider datatype
of every row expression present in the `IN` clause and should return the most general of a
set of datatypes.
--
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] abhishek-das-gupta commented on a diff in pull request #2853: [CALCITE-4632] SARG datatype should be less restictive than any of the input types
Posted by GitBox <gi...@apache.org>.
abhishek-das-gupta commented on code in PR #2853:
URL: https://github.com/apache/calcite/pull/2853#discussion_r989827399
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -1252,6 +1253,26 @@ private static String toSql(RelNode root, SqlDialect dialect,
relFn(relFn).optimize(rules, null).ok(expected);
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4632">[CALCITE-4632]
+ * Find the least restrictive datatype for SARG</a>. */
+ @Test void testLeastRestrictiveTypeForSarg() {
Review Comment:
ACK
--
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] libenchao closed pull request #2853: [CALCITE-4632] SARG datatype should be less restictive than any of the input types
Posted by GitBox <gi...@apache.org>.
libenchao closed pull request #2853: [CALCITE-4632] SARG datatype should be less restictive than any of the input types
URL: https://github.com/apache/calcite/pull/2853
--
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] abhishek-das-gupta commented on pull request #2853: [CALCITE-4632] SARG datatype should be less restictive than any of the input types
Posted by GitBox <gi...@apache.org>.
abhishek-das-gupta commented on PR #2853:
URL: https://github.com/apache/calcite/pull/2853#issuecomment-1277047972
@libenchao I agree with your comment. Please let when can resolve this issue, If nothing more needs to be done.
--
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] libenchao commented on a diff in pull request #2853: [CALCITE-4632] SARG datatype should be less restictive than any of the input types
Posted by GitBox <gi...@apache.org>.
libenchao commented on code in PR #2853:
URL: https://github.com/apache/calcite/pull/2853#discussion_r997163233
##########
core/src/test/java/org/apache/calcite/rex/RexBuilderTest.java:
##########
@@ -624,6 +624,29 @@ private void checkDate(RexLiteral literal) {
assertThat(inCall.getKind(), is(SqlKind.SEARCH));
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4632">[CALCITE-4632]
+ * Find the least restrictive datatype for SARG</a>. */
+ @Test void testLeastRestrictiveTypeForSarg() {
+ final RelDataTypeFactory typeFactory = new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+ final RexBuilder rexBuilder = new RexBuilder(typeFactory);
+ final RelDataType decimalType = typeFactory.createSqlType(SqlTypeName.DECIMAL);
+ RexNode left = rexBuilder.makeInputRef(decimalType, 0);
+ final RexNode literal1 = rexBuilder.makeExactLiteral(new BigDecimal("1.0"));
+ final RexNode literal2 = rexBuilder.makeExactLiteral(new BigDecimal("20000.0"));
+
+ RexNode inCall = rexBuilder.makeIn(left, ImmutableList.of(literal1, literal2));
+ assertThat(inCall.getKind(), is(SqlKind.SEARCH));
+
+ final RexNode sarg = ((RexCall) inCall).operands.get(1);
+ final RelDataType leastRestrictiveType =
+ typeFactory.leastRestrictive(ImmutableList.of(literal1.getType(), literal2.getType()));
Review Comment:
I'm not sure your new commit addressed Julian's comment, but per my understanding, I prefer the below way (and this is how I understand Julian's comment), what do you think?
```java
RelDataType expected = typeFactory.createSqlType(SqlTypeName.DECIMAL, 6, 1);
assertEquals(sarg.getType(), expected);
```
--
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] libenchao commented on a diff in pull request #2853: [CALCITE-4632] SARG datatype should be less restictive than any of the input types
Posted by GitBox <gi...@apache.org>.
libenchao commented on code in PR #2853:
URL: https://github.com/apache/calcite/pull/2853#discussion_r990619993
##########
core/src/main/java/org/apache/calcite/rex/RexBuilder.java:
##########
@@ -1355,10 +1356,14 @@ public RexNode makeIn(RexNode arg, List<? extends RexNode> ranges) {
if (areAssignable(arg, ranges)) {
final Sarg sarg = toSarg(Comparable.class, ranges, RexUnknownAs.UNKNOWN);
if (sarg != null) {
- final RexNode range0 = ranges.get(0);
+ final List<RelDataType> types = ranges.stream()
+ .map(RexNode::getType)
+ .collect(Collectors.toList());
+ RelDataType searchType = typeFactory.leastRestrictive(types);
+ searchType = searchType == null ? ranges.get(0).getType() : searchType;
Review Comment:
The question is: "is it correct to use `ranges.get(0).getType()`?". If it's not correct, then we'd better throw exception. If we decide to keep it, then we should know in what cases will `searchType == null` is true, and we still want to use `ranges.get(0).getType()` as the final type.
--
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] libenchao commented on pull request #2853: [CALCITE-4632] SARG datatype should be less restictive than any of the input types
Posted by GitBox <gi...@apache.org>.
libenchao commented on PR #2853:
URL: https://github.com/apache/calcite/pull/2853#issuecomment-1272723723
I found that there is a similar work before: [CALCITE-4888](https://issues.apache.org/jira/browse/CALCITE-4888).
@beyond1920 What do you think about this? I think we can add you as a co-author of this issue, and resolve both CALCITE-4888 and CALCITE-4632 together.
--
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 #2853: [CALCITE-4632] SARG datatype should be less restictive than any of the input types
Posted by GitBox <gi...@apache.org>.
julianhyde commented on code in PR #2853:
URL: https://github.com/apache/calcite/pull/2853#discussion_r996032708
##########
core/src/test/java/org/apache/calcite/rex/RexBuilderTest.java:
##########
@@ -624,6 +624,29 @@ private void checkDate(RexLiteral literal) {
assertThat(inCall.getKind(), is(SqlKind.SEARCH));
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4632">[CALCITE-4632]
+ * Find the least restrictive datatype for SARG</a>. */
+ @Test void testLeastRestrictiveTypeForSarg() {
+ final RelDataTypeFactory typeFactory = new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+ final RexBuilder rexBuilder = new RexBuilder(typeFactory);
+ final RelDataType decimalType = typeFactory.createSqlType(SqlTypeName.DECIMAL);
+ RexNode left = rexBuilder.makeInputRef(decimalType, 0);
+ final RexNode literal1 = rexBuilder.makeExactLiteral(new BigDecimal("1.0"));
+ final RexNode literal2 = rexBuilder.makeExactLiteral(new BigDecimal("20000.0"));
+
+ RexNode inCall = rexBuilder.makeIn(left, ImmutableList.of(literal1, literal2));
+ assertThat(inCall.getKind(), is(SqlKind.SEARCH));
+
+ final RexNode sarg = ((RexCall) inCall).operands.get(1);
+ final RelDataType leastRestrictiveType =
+ typeFactory.leastRestrictive(ImmutableList.of(literal1.getType(), literal2.getType()));
Review Comment:
Can you assert what the least restrictive type is? It will make the test more concrete.
--
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] abhishek-das-gupta commented on a diff in pull request #2853: [CALCITE-4632] SARG datatype should be less restictive than any of the input types
Posted by GitBox <gi...@apache.org>.
abhishek-das-gupta commented on code in PR #2853:
URL: https://github.com/apache/calcite/pull/2853#discussion_r996345727
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -1258,6 +1259,26 @@ private static String toSql(RelNode root, SqlDialect dialect,
relFn(relFn).optimize(rules, null).ok(expected);
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4632">[CALCITE-4632]
+ * Find the least restrictive datatype for SARG</a>. */
+ @Test void testLeastRestrictiveTypeForSarg() {
+ final Function<RelBuilder, RelNode> relFn = b -> b
+ .scan("EMP")
+ .filter(
+ b.or(b.isNull(b.field("COMM")),
+ b.in(
+ b.field("COMM"),
+ b.literal(new BigDecimal("1.0")), b.literal(new BigDecimal("20000.0")))
+ )
+ )
Review Comment:
Sorry about this. Addressed this.
##########
core/src/test/java/org/apache/calcite/rex/RexBuilderTest.java:
##########
@@ -624,6 +624,29 @@ private void checkDate(RexLiteral literal) {
assertThat(inCall.getKind(), is(SqlKind.SEARCH));
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4632">[CALCITE-4632]
+ * Find the least restrictive datatype for SARG</a>. */
+ @Test void testLeastRestrictiveTypeForSarg() {
+ final RelDataTypeFactory typeFactory = new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+ final RexBuilder rexBuilder = new RexBuilder(typeFactory);
+ final RelDataType decimalType = typeFactory.createSqlType(SqlTypeName.DECIMAL);
+ RexNode left = rexBuilder.makeInputRef(decimalType, 0);
+ final RexNode literal1 = rexBuilder.makeExactLiteral(new BigDecimal("1.0"));
+ final RexNode literal2 = rexBuilder.makeExactLiteral(new BigDecimal("20000.0"));
+
+ RexNode inCall = rexBuilder.makeIn(left, ImmutableList.of(literal1, literal2));
+ assertThat(inCall.getKind(), is(SqlKind.SEARCH));
+
+ final RexNode sarg = ((RexCall) inCall).operands.get(1);
+ final RelDataType leastRestrictiveType =
+ typeFactory.leastRestrictive(ImmutableList.of(literal1.getType(), literal2.getType()));
Review Comment:
ACK
--
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] abhishek-das-gupta commented on a diff in pull request #2853: [CALCITE-4632] SARG datatype should be less restictive than any of the input types
Posted by GitBox <gi...@apache.org>.
abhishek-das-gupta commented on code in PR #2853:
URL: https://github.com/apache/calcite/pull/2853#discussion_r997862622
##########
core/src/test/java/org/apache/calcite/rex/RexBuilderTest.java:
##########
@@ -624,6 +624,29 @@ private void checkDate(RexLiteral literal) {
assertThat(inCall.getKind(), is(SqlKind.SEARCH));
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4632">[CALCITE-4632]
+ * Find the least restrictive datatype for SARG</a>. */
+ @Test void testLeastRestrictiveTypeForSarg() {
+ final RelDataTypeFactory typeFactory = new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT);
+ final RexBuilder rexBuilder = new RexBuilder(typeFactory);
+ final RelDataType decimalType = typeFactory.createSqlType(SqlTypeName.DECIMAL);
+ RexNode left = rexBuilder.makeInputRef(decimalType, 0);
+ final RexNode literal1 = rexBuilder.makeExactLiteral(new BigDecimal("1.0"));
+ final RexNode literal2 = rexBuilder.makeExactLiteral(new BigDecimal("20000.0"));
+
+ RexNode inCall = rexBuilder.makeIn(left, ImmutableList.of(literal1, literal2));
+ assertThat(inCall.getKind(), is(SqlKind.SEARCH));
+
+ final RexNode sarg = ((RexCall) inCall).operands.get(1);
+ final RelDataType leastRestrictiveType =
+ typeFactory.leastRestrictive(ImmutableList.of(literal1.getType(), literal2.getType()));
Review Comment:
Yes, i think I mistinterpreted it. Apologies.
--
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] abhishek-das-gupta commented on a diff in pull request #2853: [CALCITE-4632] SARG datatype should be less restictive than any of the input types
Posted by GitBox <gi...@apache.org>.
abhishek-das-gupta commented on code in PR #2853:
URL: https://github.com/apache/calcite/pull/2853#discussion_r990811601
##########
core/src/main/java/org/apache/calcite/rex/RexBuilder.java:
##########
@@ -1355,10 +1356,14 @@ public RexNode makeIn(RexNode arg, List<? extends RexNode> ranges) {
if (areAssignable(arg, ranges)) {
final Sarg sarg = toSarg(Comparable.class, ranges, RexUnknownAs.UNKNOWN);
if (sarg != null) {
- final RexNode range0 = ranges.get(0);
+ final List<RelDataType> types = ranges.stream()
+ .map(RexNode::getType)
+ .collect(Collectors.toList());
+ RelDataType searchType = typeFactory.leastRestrictive(types);
+ searchType = searchType == null ? ranges.get(0).getType() : searchType;
Review Comment:
Thanks @libenchao . I think if we don't find a least restrictive `searchType` among the `types`, then selecting `ranges.get(0).getType()` is wrong. Since it might eventually fail at Rel to Sql conversion. Hence throwing an exception
--
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] libenchao commented on pull request #2853: [CALCITE-4632] SARG datatype should be less restictive than any of the input types
Posted by GitBox <gi...@apache.org>.
libenchao commented on PR #2853:
URL: https://github.com/apache/calcite/pull/2853#issuecomment-1283915022
@abhishek-das-gupta I found that `RexBuilder#makeBetween` also may have the same problem, could you help to confirm this, and fix it if it exists? (In this issue, or a follow-up issue, it's up to you, I'm ok with both way)
--
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 #2853: [CALCITE-4632] SARG datatype should be less restictive than any of the input types
Posted by GitBox <gi...@apache.org>.
julianhyde commented on code in PR #2853:
URL: https://github.com/apache/calcite/pull/2853#discussion_r996031793
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -1258,6 +1259,26 @@ private static String toSql(RelNode root, SqlDialect dialect,
relFn(relFn).optimize(rules, null).ok(expected);
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4632">[CALCITE-4632]
+ * Find the least restrictive datatype for SARG</a>. */
+ @Test void testLeastRestrictiveTypeForSarg() {
+ final Function<RelBuilder, RelNode> relFn = b -> b
+ .scan("EMP")
+ .filter(
+ b.or(b.isNull(b.field("COMM")),
+ b.in(
+ b.field("COMM"),
+ b.literal(new BigDecimal("1.0")), b.literal(new BigDecimal("20000.0")))
+ )
+ )
Review Comment:
Please remove newlines before close-parentheses. This is not Calcite's coding style.
--
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] libenchao commented on a diff in pull request #2853: [CALCITE-4632] SARG datatype should be less restictive than any of the input types
Posted by GitBox <gi...@apache.org>.
libenchao commented on code in PR #2853:
URL: https://github.com/apache/calcite/pull/2853#discussion_r989705887
##########
core/src/main/java/org/apache/calcite/rex/RexBuilder.java:
##########
@@ -1355,10 +1356,14 @@ public RexNode makeIn(RexNode arg, List<? extends RexNode> ranges) {
if (areAssignable(arg, ranges)) {
final Sarg sarg = toSarg(Comparable.class, ranges, RexUnknownAs.UNKNOWN);
if (sarg != null) {
- final RexNode range0 = ranges.get(0);
+ final List<RelDataType> types = ranges.stream()
+ .map(RexNode::getType)
+ .collect(Collectors.toList());
+ RelDataType searchType = typeFactory.leastRestrictive(types);
+ searchType = searchType == null ? ranges.get(0).getType() : searchType;
Review Comment:
If`searchType == null` is `true`, do we really need to proceed with `ranges.get(0).getType()`?
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -1252,6 +1253,26 @@ private static String toSql(RelNode root, SqlDialect dialect,
relFn(relFn).optimize(rules, null).ok(expected);
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4632">[CALCITE-4632]
+ * Find the least restrictive datatype for SARG</a>. */
+ @Test void testLeastRestrictiveTypeForSarg() {
Review Comment:
As discussed in the JIRA, the root cause is not from `RelToSqlConverter`, so I'm not sure this is the right place to add tests. How about we add more tests in `RexBuilderTest` and assert the `RexNode`'s type returned from `RexBuilder#makeIn`?
--
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] abhishek-das-gupta commented on a diff in pull request #2853: [CALCITE-4632] SARG datatype should be less restictive than any of the input types
Posted by GitBox <gi...@apache.org>.
abhishek-das-gupta commented on code in PR #2853:
URL: https://github.com/apache/calcite/pull/2853#discussion_r990604610
##########
core/src/main/java/org/apache/calcite/rex/RexBuilder.java:
##########
@@ -1355,10 +1356,14 @@ public RexNode makeIn(RexNode arg, List<? extends RexNode> ranges) {
if (areAssignable(arg, ranges)) {
final Sarg sarg = toSarg(Comparable.class, ranges, RexUnknownAs.UNKNOWN);
if (sarg != null) {
- final RexNode range0 = ranges.get(0);
+ final List<RelDataType> types = ranges.stream()
+ .map(RexNode::getType)
+ .collect(Collectors.toList());
+ RelDataType searchType = typeFactory.leastRestrictive(types);
+ searchType = searchType == null ? ranges.get(0).getType() : searchType;
Review Comment:
All the usages of `RelDataTypeFactory.leastRestrictive` handles the edge case where the return type is null. So I thought about defaulting to how it get handles now in current master. Is it really not necessary to handle this case? Can you provide your thoughts on this?
--
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