[GitHub] [calcite] jinxing64 opened a new pull request #1413: [CALCITE-3292] SqlToRelConverter#substituteSubQuery fails with NullPo…
jinxing64 opened a new pull request #1413: [CALCITE-3292]
SqlToRelConverter#substituteSubQuery fails with NullPo…
URL: https://github.com/apache/calcite/pull/1413
interException when converting SqlUpdate
Current code fails below test
```
@Test public void testUpdateSubQueryWithIn1() {
final String sql = "update emp\n"
+ "set empno = 1 where emp.empno in (\n"
+ " select emp.empno from emp where emp.empno=2)";
sql(sql).ok();
}
java.lang.NullPointerExceptionjava.lang.NullPointerException at
org.apache.calcite.rel.logical.LogicalJoin.create(LogicalJoin.java:146) at
org.apache.calcite.rel.logical.LogicalJoin.create(LogicalJoin.java:163) at
org.apache.calcite.sql2rel.SqlToRelConverter.substituteSubQuery(SqlToRelConverter.java:1130)
at
org.apache.calcite.sql2rel.SqlToRelConverter.replaceSubQueries(SqlToRelConverter.java:1014)
at
org.apache.calcite.sql2rel.SqlToRelConverter.convertUpdate(SqlToRelConverter.java:3574)
at
org.apache.calcite.sql2rel.SqlToRelConverter.convertQueryRecursive(SqlToRelConverter.java:3176)
at
org.apache.calcite.sql2rel.SqlToRelConverter.convertQuery(SqlToRelConverter.java:563)
at
org.apache.calcite.test.SqlToRelTestBase$TesterImpl.convertSqlToRel(SqlToRelTestBase.java:616)
at
org.apache.calcite.test.SqlToRelTestBase$TesterImpl.assertConvertsTo(SqlToRelTestBase.java:731)
at
org.apache.calcite.test.SqlToRelConverterTest$Sql.convertsTo(SqlToRelConverterTest.java:3601)
at
org.apache.calcite.test.SqlToRelConverterTest$Sql.ok(SqlToRelConverterTest.java:3593)
```
In above case, `Subquery` is used as `SqlUpdate#condition`, when converting
and trying to replace the subquery in `SqlUpdate#condition`, `BalckBoard#root`
is null and it makes no sense to do the subquery substitution.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] LiShuMing commented on a change in pull request #1401: [CALCITE-3280] Add regexp_replace function (Shuming Li)
LiShuMing commented on a change in pull request #1401: [CALCITE-3280] Add
regexp_replace function (Shuming Li)
URL: https://github.com/apache/calcite/pull/1401#discussion_r317450773
##
File path:
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java
##
@@ -313,6 +313,16 @@ private SqlLibraryOperators() {
OperandTypes.INTEGER,
SqlFunctionCategory.STRING);
+ @LibraryOperator(libraries = {MYSQL, POSTGRESQL, ORACLE})
+ public static final SqlFunction REGEXP_REPLACE =
+ new SqlFunction("REGEXP_REPLACE",
+ SqlKind.OTHER_FUNCTION,
+ ReturnTypes.cascade(ReturnTypes.explicit(SqlTypeName.VARCHAR),
+ SqlTypeTransforms.TO_NULLABLE),
Review comment:
I updated the code as you said by following the `Mysql`'s 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] LiShuMing commented on a change in pull request #1401: [CALCITE-3280] Add regexp_replace function (Shuming Li)
LiShuMing commented on a change in pull request #1401: [CALCITE-3280] Add
regexp_replace function (Shuming Li)
URL: https://github.com/apache/calcite/pull/1401#discussion_r317450773
##
File path:
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java
##
@@ -313,6 +313,16 @@ private SqlLibraryOperators() {
OperandTypes.INTEGER,
SqlFunctionCategory.STRING);
+ @LibraryOperator(libraries = {MYSQL, POSTGRESQL, ORACLE})
+ public static final SqlFunction REGEXP_REPLACE =
+ new SqlFunction("REGEXP_REPLACE",
+ SqlKind.OTHER_FUNCTION,
+ ReturnTypes.cascade(ReturnTypes.explicit(SqlTypeName.VARCHAR),
+ SqlTypeTransforms.TO_NULLABLE),
Review comment:
I updated the codes as you said by following the `Mysql`'s 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1412: [CALCITE-3291] Fix testcase SqlFunctionsTest failed
hsyuan commented on a change in pull request #1412: [CALCITE-3291] Fix testcase
SqlFunctionsTest failed
URL: https://github.com/apache/calcite/pull/1412#discussion_r317447487
##
File path: core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java
##
@@ -881,7 +881,7 @@ private void thereAndBack(byte[] bytes) {
try {
String o = md5((String) null);
fail("Expected NPE, got " + o);
Review comment:
Should we update NPE?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] shenh062326 opened a new pull request #1412: [CALCITE-3291] Fix testcase SqlFunctionsTest failed
shenh062326 opened a new pull request #1412: [CALCITE-3291] Fix testcase SqlFunctionsTest failed URL: https://github.com/apache/calcite/pull/1412 Issue was illustrated in https://issues.apache.org/jira/browse/CALCITE-3291 , change SqlFunctionsTest.testMd5 and SqlFunctionsTest.testSha1 from NullPointerException to IllegalArgumentException. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #706: [CALCITE-2302] Implicit type cast support
danny0405 commented on a change in pull request #706: [CALCITE-2302] Implicit
type cast support
URL: https://github.com/apache/calcite/pull/706#discussion_r317439450
##
File path:
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java
##
@@ -132,11 +132,19 @@ RelDataType deriveCovarType(RelDataTypeFactory
typeFactory,
* @return the result type for a decimal addition.
*/
default RelDataType deriveDecimalPlusType(RelDataTypeFactory typeFactory,
-RelDataType type1, RelDataType
type2) {
+ RelDataType type1, RelDataType type2) {
if (SqlTypeUtil.isExactNumeric(type1)
&& SqlTypeUtil.isExactNumeric(type2)) {
if (SqlTypeUtil.isDecimal(type1)
|| SqlTypeUtil.isDecimal(type2)) {
+// Java numeric will always have invalid precision/scale,
+// use its default decimal precision/scale instead.
+type1 = RelDataTypeFactoryImpl.isJavaType(type1)
Review comment:
Sure, let me add them in, i believe this patch would be merged soon.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #706: [CALCITE-2302] Implicit type cast support
DonnyZone commented on a change in pull request #706: [CALCITE-2302] Implicit
type cast support
URL: https://github.com/apache/calcite/pull/706#discussion_r317437128
##
File path:
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java
##
@@ -132,11 +132,19 @@ RelDataType deriveCovarType(RelDataTypeFactory
typeFactory,
* @return the result type for a decimal addition.
*/
default RelDataType deriveDecimalPlusType(RelDataTypeFactory typeFactory,
-RelDataType type1, RelDataType
type2) {
+ RelDataType type1, RelDataType type2) {
if (SqlTypeUtil.isExactNumeric(type1)
&& SqlTypeUtil.isExactNumeric(type2)) {
if (SqlTypeUtil.isDecimal(type1)
|| SqlTypeUtil.isDecimal(type2)) {
+// Java numeric will always have invalid precision/scale,
+// use its default decimal precision/scale instead.
+type1 = RelDataTypeFactoryImpl.isJavaType(type1)
Review comment:
Hi, @danny0405. Maybe we also need to handle `deriveDecimalMultiplyType` and
`deriveDecimalDivideType` in this class. I wait for your fix here to complete
CALCITE-3245.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1408: [CALCITE-3288] linq4j: support Set literals
danny0405 commented on a change in pull request #1408: [CALCITE-3288] linq4j:
support Set literals
URL: https://github.com/apache/calcite/pull/1408#discussion_r317426301
##
File path:
linq4j/src/main/java/org/apache/calcite/linq4j/tree/ConstantExpression.java
##
@@ -245,6 +249,31 @@ private static ExpressionWriter map(ExpressionWriter
writer, Map map,
return writer.append(end);
}
+ private static ExpressionWriter writeSet(ExpressionWriter writer, Set set) {
+writer.append("com.google.common.collect.ImmutableSet.");
+if (set.isEmpty()) {
+ return writer.append("of()");
+}
+if (set.size() < 5) {
+ return set(writer, set, "of(", ",", ")");
+}
+return set(writer, set, "builder().add(", ")\n.add(", ").build()");
+ }
+
+ private static ExpressionWriter set(ExpressionWriter writer, Set set,
+ String begin, String entrySep, String
end) {
Review comment:
Fix the indent.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1408: [CALCITE-3288] linq4j: support Set literals
danny0405 commented on a change in pull request #1408: [CALCITE-3288] linq4j:
support Set literals
URL: https://github.com/apache/calcite/pull/1408#discussion_r317426245
##
File path:
linq4j/src/main/java/org/apache/calcite/linq4j/tree/ConstantExpression.java
##
@@ -245,6 +249,31 @@ private static ExpressionWriter map(ExpressionWriter
writer, Map map,
return writer.append(end);
}
+ private static ExpressionWriter writeSet(ExpressionWriter writer, Set set) {
+writer.append("com.google.common.collect.ImmutableSet.");
+if (set.isEmpty()) {
Review comment:
Instead of hard code the package name, can we use
`XXX.class.getCanonicalName()` ?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] shenh062326 commented on a change in pull request #1410: [CALCITE-3287] Let getRowCount for union in RelMdRowCount.java take into account 'union all'
shenh062326 commented on a change in pull request #1410: [CALCITE-3287] Let
getRowCount for union in RelMdRowCount.java take into account 'union all'
URL: https://github.com/apache/calcite/pull/1410#discussion_r317424866
##
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMdRowCount.java
##
@@ -83,15 +83,7 @@ public Double getRowCount(RelSubset subset,
RelMetadataQuery mq) {
}
public Double getRowCount(Union rel, RelMetadataQuery mq) {
-double rowCount = 0.0;
-for (RelNode input : rel.getInputs()) {
- Double partialRowCount = mq.getRowCount(input);
- if (partialRowCount == null) {
-return null;
- }
- rowCount += partialRowCount;
-}
-return rowCount;
+return rel.estimateRowCount(mq);
Review comment:
Thanks @hsyuan , since AbstractRelNode.estimateRowCount return double, but
not Double, I can't let AbstractRelNode.estimateRowCount return null, so I
can't reuse rel.estimateRowCount. I will directly edit getRowCount in
RelMdRowCount.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] xy2953396112 opened a new pull request #1411: evaluate in AbstractNode
xy2953396112 opened a new pull request #1411: evaluate in AbstractNode URL: https://github.com/apache/calcite/pull/1411 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] shenh062326 commented on a change in pull request #1410: [CALCITE-3287] Let getRowCount for union in RelMdRowCount.java take into account 'union all'
shenh062326 commented on a change in pull request #1410: [CALCITE-3287] Let
getRowCount for union in RelMdRowCount.java take into account 'union all'
URL: https://github.com/apache/calcite/pull/1410#discussion_r317424866
##
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMdRowCount.java
##
@@ -83,15 +83,7 @@ public Double getRowCount(RelSubset subset,
RelMetadataQuery mq) {
}
public Double getRowCount(Union rel, RelMetadataQuery mq) {
-double rowCount = 0.0;
-for (RelNode input : rel.getInputs()) {
- Double partialRowCount = mq.getRowCount(input);
- if (partialRowCount == null) {
-return null;
- }
- rowCount += partialRowCount;
-}
-return rowCount;
+return rel.estimateRowCount(mq);
Review comment:
Thanks @hsyuan , I will 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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] xy2953396112 commented on issue #1408: [CALCITE-3288] linq4j: support Set literals
xy2953396112 commented on issue #1408: [CALCITE-3288] linq4j: support Set literals URL: https://github.com/apache/calcite/pull/1408#issuecomment-524678295 > Could you add your name to the commit message like other contributors and squash the commits? I will merge when it is done. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] hsyuan commented on issue #1408: [CALCITE-3288] linq4j: support Set literals
hsyuan commented on issue #1408: [CALCITE-3288] linq4j: support Set literals URL: https://github.com/apache/calcite/pull/1408#issuecomment-524674902 Could you add your name to the commit message like other contributors and squash the commits? I will merge when it is 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] hsyuan commented on issue #1410: [CALCITE-3287] Fix minor bug, getRowCount for union in RelMdRowCount.java not consider 'union all'
hsyuan commented on issue #1410: [CALCITE-3287] Fix minor bug, getRowCount for union in RelMdRowCount.java not consider 'union all' URL: https://github.com/apache/calcite/pull/1410#issuecomment-524671868 For the title, we usually don't use `Fix bug`, just describe the issue or bug concisely in the title. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1410: [CALCITE-3287] Fix minor bug, getRowCount for union in RelMdRowCount.java not consider 'union all'
hsyuan commented on a change in pull request #1410: [CALCITE-3287] Fix minor
bug, getRowCount for union in RelMdRowCount.java not consider 'union all'
URL: https://github.com/apache/calcite/pull/1410#discussion_r317417476
##
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMdRowCount.java
##
@@ -83,15 +83,7 @@ public Double getRowCount(RelSubset subset,
RelMetadataQuery mq) {
}
public Double getRowCount(Union rel, RelMetadataQuery mq) {
-double rowCount = 0.0;
-for (RelNode input : rel.getInputs()) {
- Double partialRowCount = mq.getRowCount(input);
- if (partialRowCount == null) {
-return null;
- }
- rowCount += partialRowCount;
-}
-return rowCount;
+return rel.estimateRowCount(mq);
Review comment:
`Union.estimateRowCount` doesn't take into account the case that one of the
inputs might return 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] shenh062326 opened a new pull request #1410: [CALCITE-3287] Fix minor bug, getRowCount for union in RelMdRowCount.java not consider 'union all'
shenh062326 opened a new pull request #1410: [CALCITE-3287] Fix minor bug, getRowCount for union in RelMdRowCount.java not consider 'union all' URL: https://github.com/apache/calcite/pull/1410 Issue was illustrated in https://issues.apache.org/jira/browse/CALCITE-3287, Change getRowCount for union in RelMdRowCount.java to call Union.estimateRowCount. 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: us...@infra.apache.org With regards, Apache Git Services
