[GitHub] [calcite] XuQianJin-Stars commented on issue #1865: [CALCITE-3648] MySQL UNCOMPRESS function support
XuQianJin-Stars commented on issue #1865: [CALCITE-3648] MySQL UNCOMPRESS function support URL: https://github.com/apache/calcite/pull/1865#issuecomment-604232418 hi, @ritesh-kapoor Can this PR add a description? 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] XuQianJin-Stars commented on a change in pull request #1865: [CALCITE-3648] MySQL UNCOMPRESS function support
XuQianJin-Stars commented on a change in pull request #1865: [CALCITE-3648]
MySQL UNCOMPRESS function support
URL: https://github.com/apache/calcite/pull/1865#discussion_r398304749
##
File path:
core/src/main/java/org/apache/calcite/runtime/CompressionFunctions.java
##
@@ -62,4 +65,21 @@ public static ByteString compress(String data) {
}
}
+ /**
+ * MySql Decompression is based on zlib.
+ * https://docs.oracle.com/javase/8/docs/api/java/util/zip/Inflater.html;>Inflater
+ * is used to implement decompression.
+ */
+ public static String uncompress(ByteString data) {
+try {
+ if (data == null || data.length() == 0) {
Review comment:
> For `data.length==0`, is it possible to be an empty string?
```
mysql> select uncompress(null),uncompress('');
+--++
| uncompress(null) | uncompress('') |
+--++
| NULL ||
+--++
1 row in set (0.00 sec)
```
It should be an empty string
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 #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
danny0405 commented on a change in pull request #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r398304160
##
File path: core/build.gradle.kts
##
@@ -41,7 +41,6 @@ dependencies {
api("com.fasterxml.jackson.core:jackson-annotations")
api("org.apache.calcite.avatica:avatica-core")
-api("org.apiguardian:apiguardian-api")
Review comment:
There are so many un-expected change.
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 #1861: [CALCITE-3468] JDBC adapter may generate casts on Oracle for varchar without the precision and for char with the precision exc
danny0405 commented on a change in pull request #1861: [CALCITE-3468] JDBC
adapter may generate casts on Oracle for varchar without the precision and for
char with the precision exceeding max length of Oracle
URL: https://github.com/apache/calcite/pull/1861#discussion_r398303679
##
File path: core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
##
@@ -1015,17 +1015,23 @@ private static boolean flattenFields(
* @param type type descriptor
* @param charSetName charSet name
* @param maxPrecision The max allowed precision.
+ * @param allowsPrecisionUnspecified whether allows precision not specified
+ * @param defaultPrecision The default precision.
* @return corresponding parse representation
*/
public static SqlDataTypeSpec convertTypeToSpec(RelDataType type,
- String charSetName, int maxPrecision) {
+ String charSetName, int maxPrecision,
+ boolean allowsPrecisionUnspecified, int defaultPrecision) {
SqlTypeName typeName = type.getSqlTypeName();
Review comment:
I would suggest to replace `allowsPrecisionUnspecified` and
`defaultPrecision` with a single `explicitPrecision`, and if a
`explicitPrecision` is specified, use it directly.
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 #1861: [CALCITE-3468] JDBC adapter may generate casts on Oracle for varchar without the precision and for char with the precision exc
danny0405 commented on a change in pull request #1861: [CALCITE-3468] JDBC
adapter may generate casts on Oracle for varchar without the precision and for
char with the precision exceeding max length of Oracle
URL: https://github.com/apache/calcite/pull/1861#discussion_r398303679
##
File path: core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java
##
@@ -1015,17 +1015,23 @@ private static boolean flattenFields(
* @param type type descriptor
* @param charSetName charSet name
* @param maxPrecision The max allowed precision.
+ * @param allowsPrecisionUnspecified whether allows precision not specified
+ * @param defaultPrecision The default precision.
* @return corresponding parse representation
*/
public static SqlDataTypeSpec convertTypeToSpec(RelDataType type,
- String charSetName, int maxPrecision) {
+ String charSetName, int maxPrecision,
+ boolean allowsPrecisionUnspecified, int defaultPrecision) {
SqlTypeName typeName = type.getSqlTypeName();
Review comment:
I would suggest to replace `allowsPrecisionUnspecified` and
`defaultPrecision` with a single `explicitPrecision`, and if a
`explicitPrecision` is specified, use it directly. Leave out the complex
`allowsPrecisionUnspecified` decision to the invoker.
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 #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
hsyuan commented on a change in pull request #1875: [CALCITE-3873] Use global
caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r398303036
##
File path: core/src/main/java/org/apache/calcite/util/ReflectUtil.java
##
@@ -403,71 +403,40 @@ private static Method lookupVisitMethod(
}
/**
- * Creates a dispatcher for calls to {@link #lookupVisitMethod}. The
- * dispatcher caches methods between invocations.
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}. By default
the
+ * dispatcher caches methods globally. If caching methods between invocations
+ * is preferred, use the overridden method {@link #createDispatcher(Class,
Class, boolean)}.
*
- * @param visitorBaseClazz Visitor base class
- * @param visiteeBaseClazz Visitee base class
+ * @param visitorBaseClazzVisitor base class
+ * @param visiteeBaseClazzVisitee base class
* @return cache of methods
*/
public static ReflectiveVisitDispatcher createDispatcher(
final Class visitorBaseClazz,
final Class visiteeBaseClazz) {
+return createDispatcher(visitorBaseClazz, visiteeBaseClazz, true);
+ }
+
+ /**
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}.
+ *
+ * @param visitorBaseClazzVisitor base class
+ * @param visiteeBaseClazzVisitee base class
+ * @param useGlobalMethodCacheIf set to true, the created dispatchers
will
+ *share a globally cache to store methods to
+ *mitigating reflection invocation overhead
+ *when looking up method. If set to false,
every
+ *dispatcher instance will only cache methods
+ *between invocations individually.
+ * @return cache of methods
+ */
+ public static ReflectiveVisitDispatcher createDispatcher(
+ final Class visitorBaseClazz,
+ final Class visiteeBaseClazz,
+ final boolean useGlobalMethodCache) {
Review comment:
There won't be seeable behavior change with the flag. We can always use
global cache and remove the flag.
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] wenhuitang commented on a change in pull request #1861: [CALCITE-3468] JDBC adapter may generate casts on Oracle for varchar without the precision and for char with the precision ex
wenhuitang commented on a change in pull request #1861: [CALCITE-3468] JDBC
adapter may generate casts on Oracle for varchar without the precision and for
char with the precision exceeding max length of Oracle
URL: https://github.com/apache/calcite/pull/1861#discussion_r398299807
##
File path:
core/src/main/java/org/apache/calcite/sql/dialect/OracleSqlDialect.java
##
@@ -54,10 +54,20 @@
case VARCHAR:
// Maximum size of 4000 bytes for varchar2.
return 4000;
+ case CHAR:
+return 2000;
default:
return super.getMaxPrecision(typeName);
}
}
+@Override public int getDefaultPrecision(SqlTypeName typeName) {
+ switch (typeName) {
+ case VARCHAR:
Review comment:
Thanks a lot, I add the comment for this kind of situation.
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] wenhuitang commented on a change in pull request #1861: [CALCITE-3468] JDBC adapter may generate casts on Oracle for varchar without the precision and for char with the precision ex
wenhuitang commented on a change in pull request #1861: [CALCITE-3468] JDBC
adapter may generate casts on Oracle for varchar without the precision and for
char with the precision exceeding max length of Oracle
URL: https://github.com/apache/calcite/pull/1861#discussion_r398299547
##
File path: core/src/main/java/org/apache/calcite/sql/SqlDialect.java
##
@@ -772,16 +773,26 @@ public boolean supportsDataType(RelDataType type) {
return true;
}
- /** Returns SqlNode for type in "cast(column as type)", which might be
+ public boolean allowTypeWithUnspecifiedPrecision(RelDataType type) {
+return true;
+ }
Review comment:
The document of this method has been added.
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 #1865: [CALCITE-3648] MySQL UNCOMPRESS function support
DonnyZone commented on a change in pull request #1865: [CALCITE-3648] MySQL
UNCOMPRESS function support
URL: https://github.com/apache/calcite/pull/1865#discussion_r398296250
##
File path:
core/src/main/java/org/apache/calcite/runtime/CompressionFunctions.java
##
@@ -62,4 +65,21 @@ public static ByteString compress(String data) {
}
}
+ /**
+ * MySql Decompression is based on zlib.
+ * https://docs.oracle.com/javase/8/docs/api/java/util/zip/Inflater.html;>Inflater
+ * is used to implement decompression.
+ */
+ public static String uncompress(ByteString data) {
+try {
+ if (data == null || data.length() == 0) {
Review comment:
For `data.length==0`, is it possible to be an empty string?
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] neoremind commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
neoremind commented on a change in pull request #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r398291034
##
File path: core/src/main/java/org/apache/calcite/util/ReflectUtil.java
##
@@ -403,71 +403,40 @@ private static Method lookupVisitMethod(
}
/**
- * Creates a dispatcher for calls to {@link #lookupVisitMethod}. The
- * dispatcher caches methods between invocations.
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}. By default
the
+ * dispatcher caches methods globally. If caching methods between invocations
+ * is preferred, use the overridden method {@link #createDispatcher(Class,
Class, boolean)}.
*
- * @param visitorBaseClazz Visitor base class
- * @param visiteeBaseClazz Visitee base class
+ * @param visitorBaseClazzVisitor base class
+ * @param visiteeBaseClazzVisitee base class
* @return cache of methods
*/
public static ReflectiveVisitDispatcher createDispatcher(
final Class visitorBaseClazz,
final Class visiteeBaseClazz) {
+return createDispatcher(visitorBaseClazz, visiteeBaseClazz, true);
+ }
+
+ /**
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}.
+ *
+ * @param visitorBaseClazzVisitor base class
+ * @param visiteeBaseClazzVisitee base class
+ * @param useGlobalMethodCacheIf set to true, the created dispatchers
will
+ *share a globally cache to store methods to
+ *mitigating reflection invocation overhead
+ *when looking up method. If set to false,
every
+ *dispatcher instance will only cache methods
+ *between invocations individually.
+ * @return cache of methods
+ */
+ public static ReflectiveVisitDispatcher createDispatcher(
+ final Class visitorBaseClazz,
+ final Class visiteeBaseClazz,
+ final boolean useGlobalMethodCache) {
Review comment:
To maintain backward compatibility, I add one param boolean
`useGlobalMethodCache` to method `ReflectiveVisitDispatcher
createDispatcher(..)` and provide an overridden method to make
`useGlobalMethodCache=true` by default, so anywhere else won't be changed, but
the underlying behavior will enable global caching. In case there might be
situations where `ReflectiveVisitDispatcher` is likely to cache per instance
and does not want to bother the global cache, so to make that flexibility
possible I made such change.
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] neoremind commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
neoremind commented on a change in pull request #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r398291034
##
File path: core/src/main/java/org/apache/calcite/util/ReflectUtil.java
##
@@ -403,71 +403,40 @@ private static Method lookupVisitMethod(
}
/**
- * Creates a dispatcher for calls to {@link #lookupVisitMethod}. The
- * dispatcher caches methods between invocations.
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}. By default
the
+ * dispatcher caches methods globally. If caching methods between invocations
+ * is preferred, use the overridden method {@link #createDispatcher(Class,
Class, boolean)}.
*
- * @param visitorBaseClazz Visitor base class
- * @param visiteeBaseClazz Visitee base class
+ * @param visitorBaseClazzVisitor base class
+ * @param visiteeBaseClazzVisitee base class
* @return cache of methods
*/
public static ReflectiveVisitDispatcher createDispatcher(
final Class visitorBaseClazz,
final Class visiteeBaseClazz) {
+return createDispatcher(visitorBaseClazz, visiteeBaseClazz, true);
+ }
+
+ /**
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}.
+ *
+ * @param visitorBaseClazzVisitor base class
+ * @param visiteeBaseClazzVisitee base class
+ * @param useGlobalMethodCacheIf set to true, the created dispatchers
will
+ *share a globally cache to store methods to
+ *mitigating reflection invocation overhead
+ *when looking up method. If set to false,
every
+ *dispatcher instance will only cache methods
+ *between invocations individually.
+ * @return cache of methods
+ */
+ public static ReflectiveVisitDispatcher createDispatcher(
+ final Class visitorBaseClazz,
+ final Class visiteeBaseClazz,
+ final boolean useGlobalMethodCache) {
Review comment:
To maintain backward compatibility, I add one param boolean
`useGlobalMethodCache` to method `ReflectiveVisitDispatcher
createDispatcher(..)` and provide an overridden method to make
`useGlobalMethodCache=true` by default, so anywhere else won't be changed, but
the underlying behavior will enable caching. In case there might be situations
where `ReflectiveVisitDispatcher` is likely to cache per instance and does not
want to bother the global cache, so to make that flexibility possible I made
such change.
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 merged pull request #1872: [CALCITE-3871] Remove dependency of org.apiguardian:apiguardian-api
danny0405 merged pull request #1872: [CALCITE-3871] Remove dependency of org.apiguardian:apiguardian-api URL: https://github.com/apache/calcite/pull/1872 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
[calcite] branch master updated: [CALCITE-3871] Remove dependency of org.apiguardian:apiguardian-api
This is an automated email from the ASF dual-hosted git repository.
danny0405 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/calcite.git
The following commit(s) were added to refs/heads/master by this push:
new ebbba56 [CALCITE-3871] Remove dependency of
org.apiguardian:apiguardian-api
ebbba56 is described below
commit ebbba566fbdbdd0923adda596467090d708cc14b
Author: yuzhao.cyz
AuthorDate: Wed Mar 25 14:49:30 2020 +0800
[CALCITE-3871] Remove dependency of org.apiguardian:apiguardian-api
Also fix the pigunit version to 0.16.0.
---
bom/build.gradle.kts | 1 -
core/build.gradle.kts | 1 -
.../java/org/apache/calcite/rel/RelWriter.java | 3 +-
.../main/java/org/apache/calcite/rex/RexNode.java | 3 +-
.../main/java/org/apache/calcite/rex/RexUtil.java | 3 +-
.../main/java/org/apache/calcite/runtime/Hook.java | 3 +-
.../main/java/org/apache/calcite/sql/SqlKind.java | 2 +-
gradle.properties | 3 +-
linq4j/build.gradle.kts| 2 -
.../main/java/org/apache/calcite/linq4j/API.java | 120 +
.../apache/calcite/linq4j/EnumerableDefaults.java | 2 -
11 files changed, 126 insertions(+), 17 deletions(-)
diff --git a/bom/build.gradle.kts b/bom/build.gradle.kts
index ed4e16d..4dc2867 100644
--- a/bom/build.gradle.kts
+++ b/bom/build.gradle.kts
@@ -104,7 +104,6 @@ dependencies {
apiv("org.apache.pig:pig")
apiv("org.apache.pig:pigunit", "pig")
apiv("org.apache.spark:spark-core_2.10", "spark")
-apiv("org.apiguardian:apiguardian-api")
apiv("org.bouncycastle:bcpkix-jdk15on", "bouncycastle")
apiv("org.bouncycastle:bcprov-jdk15on", "bouncycastle")
apiv("org.cassandraunit:cassandra-unit")
diff --git a/core/build.gradle.kts b/core/build.gradle.kts
index f80da4c..caa7df2 100644
--- a/core/build.gradle.kts
+++ b/core/build.gradle.kts
@@ -41,7 +41,6 @@ dependencies {
api("com.fasterxml.jackson.core:jackson-annotations")
api("org.apache.calcite.avatica:avatica-core")
-api("org.apiguardian:apiguardian-api")
implementation("com.esri.geometry:esri-geometry-api")
implementation("com.fasterxml.jackson.core:jackson-core")
diff --git a/core/src/main/java/org/apache/calcite/rel/RelWriter.java
b/core/src/main/java/org/apache/calcite/rel/RelWriter.java
index 4f6d114..883d8bb 100644
--- a/core/src/main/java/org/apache/calcite/rel/RelWriter.java
+++ b/core/src/main/java/org/apache/calcite/rel/RelWriter.java
@@ -16,12 +16,11 @@
*/
package org.apache.calcite.rel;
+import org.apache.calcite.linq4j.API;
import org.apache.calcite.rex.RexNode;
import org.apache.calcite.sql.SqlExplainLevel;
import org.apache.calcite.util.Pair;
-import org.apiguardian.api.API;
-
import java.util.List;
/**
diff --git a/core/src/main/java/org/apache/calcite/rex/RexNode.java
b/core/src/main/java/org/apache/calcite/rex/RexNode.java
index 7a8f058..37b3987 100644
--- a/core/src/main/java/org/apache/calcite/rex/RexNode.java
+++ b/core/src/main/java/org/apache/calcite/rex/RexNode.java
@@ -17,11 +17,10 @@
package org.apache.calcite.rex;
import org.apache.calcite.config.CalciteSystemProperty;
+import org.apache.calcite.linq4j.API;
import org.apache.calcite.rel.type.RelDataType;
import org.apache.calcite.sql.SqlKind;
-import org.apiguardian.api.API;
-
import java.util.Collection;
import java.util.concurrent.atomic.AtomicInteger;
diff --git a/core/src/main/java/org/apache/calcite/rex/RexUtil.java
b/core/src/main/java/org/apache/calcite/rex/RexUtil.java
index 7dd6a9d..c53a7ea 100644
--- a/core/src/main/java/org/apache/calcite/rex/RexUtil.java
+++ b/core/src/main/java/org/apache/calcite/rex/RexUtil.java
@@ -16,6 +16,7 @@
*/
package org.apache.calcite.rex;
+import org.apache.calcite.linq4j.API;
import org.apache.calcite.linq4j.function.Predicate1;
import org.apache.calcite.plan.RelOptPredicateList;
import org.apache.calcite.plan.RelOptUtil;
@@ -50,8 +51,6 @@ import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
-import org.apiguardian.api.API;
-
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
diff --git a/core/src/main/java/org/apache/calcite/runtime/Hook.java
b/core/src/main/java/org/apache/calcite/runtime/Hook.java
index 22ac163..c97d5f7 100644
--- a/core/src/main/java/org/apache/calcite/runtime/Hook.java
+++ b/core/src/main/java/org/apache/calcite/runtime/Hook.java
@@ -16,11 +16,10 @@
*/
package org.apache.calcite.runtime;
+import org.apache.calcite.linq4j.API;
import org.apache.calcite.rel.RelRoot;
import org.apache.calcite.util.Holder;
-import org.apiguardian.api.API;
-
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.CopyOnWriteArrayList;
diff --git
[GitHub] [calcite] hsyuan commented on a change in pull request #1869: [CALCITE-3868] Remove redundant ruleSet and ruleNames in VolcanoPlanner
hsyuan commented on a change in pull request #1869: [CALCITE-3868] Remove
redundant ruleSet and ruleNames in VolcanoPlanner
URL: https://github.com/apache/calcite/pull/1869#discussion_r398241191
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##
@@ -392,35 +382,24 @@ public RelSet getSet(RelNode rel) {
this.mapRel2Subset.clear();
this.relImportances.clear();
this.ruleQueue.clear();
-this.ruleNames.clear();
this.materializations.clear();
this.latticeByName.clear();
this.provenanceMap.clear();
}
public List getRules() {
-return ImmutableList.copyOf(ruleSet);
+return ImmutableList.copyOf(mapDescToRule.values());
}
public boolean addRule(RelOptRule rule) {
Review comment:
@zabetak I have addressed you comments. Thanks.
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 #1869: [CALCITE-3868] Remove redundant ruleSet and ruleNames in VolcanoPlanner
hsyuan commented on a change in pull request #1869: [CALCITE-3868] Remove
redundant ruleSet and ruleNames in VolcanoPlanner
URL: https://github.com/apache/calcite/pull/1869#discussion_r398241191
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
##
@@ -392,35 +382,24 @@ public RelSet getSet(RelNode rel) {
this.mapRel2Subset.clear();
this.relImportances.clear();
this.ruleQueue.clear();
-this.ruleNames.clear();
this.materializations.clear();
this.latticeByName.clear();
this.provenanceMap.clear();
}
public List getRules() {
-return ImmutableList.copyOf(ruleSet);
+return ImmutableList.copyOf(mapDescToRule.values());
}
public boolean addRule(RelOptRule rule) {
Review comment:
@zabetak I have addressed your comments. Thanks.
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 #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
hsyuan commented on a change in pull request #1875: [CALCITE-3873] Use global
caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r398234467
##
File path: core/src/main/java/org/apache/calcite/util/ReflectUtil.java
##
@@ -403,71 +403,40 @@ private static Method lookupVisitMethod(
}
/**
- * Creates a dispatcher for calls to {@link #lookupVisitMethod}. The
- * dispatcher caches methods between invocations.
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}. By default
the
+ * dispatcher caches methods globally. If caching methods between invocations
+ * is preferred, use the overridden method {@link #createDispatcher(Class,
Class, boolean)}.
*
- * @param visitorBaseClazz Visitor base class
- * @param visiteeBaseClazz Visitee base class
+ * @param visitorBaseClazzVisitor base class
+ * @param visiteeBaseClazzVisitee base class
* @return cache of methods
*/
public static ReflectiveVisitDispatcher createDispatcher(
final Class visitorBaseClazz,
final Class visiteeBaseClazz) {
+return createDispatcher(visitorBaseClazz, visiteeBaseClazz, true);
+ }
+
+ /**
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}.
+ *
+ * @param visitorBaseClazzVisitor base class
+ * @param visiteeBaseClazzVisitee base class
+ * @param useGlobalMethodCacheIf set to true, the created dispatchers
will
+ *share a globally cache to store methods to
+ *mitigating reflection invocation overhead
+ *when looking up method. If set to false,
every
+ *dispatcher instance will only cache methods
+ *between invocations individually.
+ * @return cache of methods
+ */
+ public static ReflectiveVisitDispatcher createDispatcher(
+ final Class visitorBaseClazz,
+ final Class visiteeBaseClazz,
+ final boolean useGlobalMethodCache) {
Review comment:
Perhaps we don't need this configuration, always use cache. Any reason that
will not use cache?
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 #1869: [CALCITE-3868] Remove redundant ruleSet and ruleNames in VolcanoPlanner
hsyuan commented on a change in pull request #1869: [CALCITE-3868] Remove
redundant ruleSet and ruleNames in VolcanoPlanner
URL: https://github.com/apache/calcite/pull/1869#discussion_r398182889
##
File path:
core/src/main/java/org/apache/calcite/plan/AbstractRelOptPlanner.java
##
@@ -44,18 +44,13 @@
* Abstract base for implementations of the {@link RelOptPlanner} interface.
*/
public abstract class AbstractRelOptPlanner implements RelOptPlanner {
- //~ Static fields/initializers -
-
- /** Regular expression for integer. */
- private static final Pattern INTEGER_PATTERN = Pattern.compile("[0-9]+");
-
//~ Instance fields
/**
* Maps rule description to rule, just to ensure that rules' descriptions
* are unique.
*/
- private final Map mapDescToRule = new HashMap<>();
+ protected final Map mapDescToRule = new HashMap<>();
Review comment:
It turns out we still need it to be protected to avoid copying.
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r398088521
##
File path: core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java
##
@@ -337,10 +339,20 @@ void mergeWith(
// merge subsets
for (RelSubset otherSubset : otherSet.subsets) {
- RelSubset subset =
- getOrCreateSubset(
- otherSubset.getCluster(),
- otherSubset.getTraitSet());
+ RelSubset subset = null;
+ RelTraitSet otherTraits = otherSubset.getTraitSet();
+
+ // if it is logical or derived physical traitSet
+ if (otherSubset.state == 0 || otherSubset.isDerived()) {
+subset = getOrCreateSubset(cluster, otherTraits, false);
+ }
+
+ // it may be required only, or both derived and required,
+ // in which case, register again.
+ if (otherSubset.isRequired()) {
Review comment:
fixed, thanks.
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 #1860: [CALCITE-2970] Add abstractConverter only between derived and required traitset
hsyuan commented on a change in pull request #1860: [CALCITE-2970] Add
abstractConverter only between derived and required traitset
URL: https://github.com/apache/calcite/pull/1860#discussion_r398088611
##
File path: core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java
##
@@ -160,107 +162,106 @@ void obliterateRelNode(RelNode rel) {
public RelSubset add(RelNode rel) {
assert equivalentSet == null : "adding to a dead set";
final RelTraitSet traitSet = rel.getTraitSet().simplify();
-final RelSubset subset = getOrCreateSubset(rel.getCluster(), traitSet);
+final RelSubset subset = getOrCreateSubset(
+rel.getCluster(), traitSet, rel.isEnforcer());
subset.add(rel);
return subset;
}
+ /**
+ * If the subset is required, convert derived subsets to this subset.
+ * Otherwise, convert this subset to required subsets in this RelSet.
+ * The subset can be both required and derived.
+ */
private void addAbstractConverters(
- VolcanoPlanner planner, RelOptCluster cluster, RelSubset subset, boolean
subsetToOthers) {
-// Converters from newly introduced subset to all the remaining one (vice
versa), only if
-// we can convert. No point adding converters if it is not possible.
-for (RelSubset other : subsets) {
+ RelOptCluster cluster, RelSubset subset, boolean required) {
+List others = subsets.stream().filter(
+n -> required ? n.isDerived() : n.isRequired())
+.collect(Collectors.toList());
+for (RelSubset other : others) {
assert other.getTraitSet().size() == subset.getTraitSet().size();
+ RelSubset from = subset;
+ RelSubset to = other;
+
+ if (required) {
+from = other;
+to = subset;
+ }
- if ((other == subset)
- || (subsetToOthers
- && !subset.getConvention().useAbstractConvertersForConversion(
- subset.getTraitSet(), other.getTraitSet()))
- || (!subsetToOthers
- && !other.getConvention().useAbstractConvertersForConversion(
- other.getTraitSet(), subset.getTraitSet( {
+ if (from == to || !from.getConvention()
+ .useAbstractConvertersForConversion(
+ from.getTraitSet(), to.getTraitSet())) {
continue;
}
final ImmutableList difference =
- subset.getTraitSet().difference(other.getTraitSet());
+ to.getTraitSet().difference(from.getTraitSet());
- boolean addAbstractConverter = true;
- int numTraitNeedConvert = 0;
-
- for (RelTrait curOtherTrait : difference) {
-RelTraitDef traitDef = curOtherTrait.getTraitDef();
-RelTrait curRelTrait = subset.getTraitSet().getTrait(traitDef);
-
-if (curRelTrait == null) {
- addAbstractConverter = false;
- break;
-}
+ boolean needsConverter = false;
-assert curRelTrait.getTraitDef() == traitDef;
-
-boolean canConvert = false;
-boolean needConvert = false;
-if (subsetToOthers) {
- // We can convert from subset to other. So, add converter with
subset as child and
- // traitset as the other's traitset.
- canConvert = traitDef.canConvert(
- cluster.getPlanner(), curRelTrait, curOtherTrait, subset);
- needConvert = !curRelTrait.satisfies(curOtherTrait);
-} else {
- // We can convert from others to subset.
- canConvert = traitDef.canConvert(
- cluster.getPlanner(), curOtherTrait, curRelTrait, other);
- needConvert = !curOtherTrait.satisfies(curRelTrait);
-}
+ for (RelTrait fromTrait : difference) {
+RelTraitDef traitDef = fromTrait.getTraitDef();
+RelTrait toTrait = to.getTraitSet().getTrait(traitDef);
-if (!canConvert) {
- addAbstractConverter = false;
+if (toTrait == null || !traitDef.canConvert(
+cluster.getPlanner(), fromTrait, toTrait, from)) {
+ needsConverter = false;
break;
}
-if (needConvert) {
- numTraitNeedConvert++;
+if (!fromTrait.satisfies(toTrait)) {
+ needsConverter = true;
}
}
- if (addAbstractConverter && numTraitNeedConvert > 0) {
-if (subsetToOthers) {
- final AbstractConverter converter =
- new AbstractConverter(cluster, subset, null,
other.getTraitSet());
- planner.register(converter, other);
-} else {
- final AbstractConverter converter =
- new AbstractConverter(cluster, other, null,
subset.getTraitSet());
- planner.register(converter, subset);
-}
+ if (needsConverter) {
+final AbstractConverter converter =
+new AbstractConverter(cluster, from, null, to.getTraitSet());
+cluster.getPlanner().register(converter, to);
}
}
}
+ RelSubset
[GitHub] [calcite] neoremind opened a new pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
neoremind opened a new pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation URL: https://github.com/apache/calcite/pull/1875 By examining a simple query through flame graph (see [issue](https://issues.apache.org/jira/browse/CALCITE-3873)), one interesting point is that I find there are too many calls using reflection, which is not performant, although the total overhead is less than 1%, I still spend some time trying to improve. Most invocations are rooted down to `ReflectiveVisitDispatcher`, the current implementation creates new instance whenever needed, and looking up methods by reflection per instance, I think by caching methods globally, as the methods count is countable to 68 possible places, different `ReflectiveVisitDispatcher` in different thread is able to reuse. The fundamental change will benefit other likewise invocations as well. 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 issue #1862: [CALCITE-3864] Implement Concat function
DonnyZone commented on issue #1862: [CALCITE-3864] Implement Concat function URL: https://github.com/apache/calcite/pull/1862#issuecomment-603832001 +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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1837: [CALCITE-3835] Overloaded table functions fail with an assertion error if param types differ
danny0405 commented on a change in pull request #1837: [CALCITE-3835]
Overloaded table functions fail with an assertion error if param types differ
URL: https://github.com/apache/calcite/pull/1837#discussion_r397832408
##
File path: core/src/main/java/org/apache/calcite/sql/SqlUtil.java
##
@@ -676,14 +680,16 @@ public static boolean matchRoutinesByParameterCount(
}
private static RelDataType bestMatch(List sqlFunctions, int i,
- RelDataTypePrecedenceList precList) {
+ List argNames, RelDataTypePrecedenceList precList) {
RelDataType bestMatch = null;
for (SqlFunction function : sqlFunctions) {
List paramTypes = function.getParamTypes();
if (paramTypes == null) {
continue;
}
- final RelDataType paramType = paramTypes.get(i);
+ final RelDataType paramType = argNames != null
+ ? paramTypes.get(function.getParamNames().indexOf(argNames.get(i)))
Review comment:
Thanks for the explanation. The code is a mess. Maybe we should have a
method named filterRoutinesByParameterName
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-avatica] vlsi opened a new pull request #120: Update Gradle: 6.1.1 -> 6.3
vlsi opened a new pull request #120: Update Gradle: 6.1.1 -> 6.3 URL: https://github.com/apache/calcite-avatica/pull/120 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
[calcite] branch master updated (c60f675 -> 989fc12)
This is an automated email from the ASF dual-hosted git repository. vladimirsitnikov pushed a change to branch master in repository https://gitbox.apache.org/repos/asf/calcite.git. from c60f675 Update Gradle: 6.1.1 -> 6.3 add 989fc12 [CALCITE-3660] Disable PigRelBuilderStyleTest#testScanAndFilter since it fails too often for no reason No new revisions were added by this update. Summary of changes: pig/src/test/java/org/apache/calcite/test/PigRelBuilderStyleTest.java | 1 + 1 file changed, 1 insertion(+)
[calcite] branch master updated (ffcfb92 -> c60f675)
This is an automated email from the ASF dual-hosted git repository. vladimirsitnikov pushed a change to branch master in repository https://gitbox.apache.org/repos/asf/calcite.git. from ffcfb92 [CALCITE-3829] MergeJoinEnumerator should not use inputs enumerators until it is really required add c60f675 Update Gradle: 6.1.1 -> 6.3 No new revisions were added by this update. Summary of changes: .github/workflows/main.yml | 6 ++--- .travis.yml| 1 + .../apache/calcite/buildtools/javacc/JavaCCTask.kt | 12 - gradle/wrapper/gradle-wrapper.jar | Bin 55616 -> 58694 bytes gradle/wrapper/gradle-wrapper.properties | 4 +-- gradlew| 29 + gradlew.bat| 3 +++ 7 files changed, 32 insertions(+), 23 deletions(-)
[GitHub] [calcite] vlsi merged pull request #1874: Update Gradle: 6.1.1 -> 6.3
vlsi merged pull request #1874: Update Gradle: 6.1.1 -> 6.3 URL: https://github.com/apache/calcite/pull/1874 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] wenhuitang commented on issue #1862: [CALCITE-3864] Implement Concat function
wenhuitang commented on issue #1862: [CALCITE-3864] Implement Concat function URL: https://github.com/apache/calcite/pull/1862#issuecomment-603802313 Comments has been addressed, and reference.md has been updated too. 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] wenhuitang commented on a change in pull request #1862: [CALCITE-3864] Implement Concat function
wenhuitang commented on a change in pull request #1862: [CALCITE-3864] Implement Concat function URL: https://github.com/apache/calcite/pull/1862#discussion_r397801950 ## File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java ## @@ -332,6 +334,8 @@ NullPolicy.STRICT); defineMethod(CONCAT, BuiltInMethod.STRING_CONCAT.method, NullPolicy.STRICT); +defineMethod(CONCAT2, BuiltInMethod.STRING_CONCAT.method, NullPolicy.STRICT); Review comment: Still keep defineMethod for CONCAT2 for looking up method clearly. 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] vvysotskyi commented on a change in pull request #1837: [CALCITE-3835] Overloaded table functions fail with an assertion error if param types differ
vvysotskyi commented on a change in pull request #1837: [CALCITE-3835]
Overloaded table functions fail with an assertion error if param types differ
URL: https://github.com/apache/calcite/pull/1837#discussion_r397800959
##
File path: core/src/main/java/org/apache/calcite/sql/SqlUtil.java
##
@@ -676,14 +680,16 @@ public static boolean matchRoutinesByParameterCount(
}
private static RelDataType bestMatch(List sqlFunctions, int i,
- RelDataTypePrecedenceList precList) {
+ List argNames, RelDataTypePrecedenceList precList) {
RelDataType bestMatch = null;
for (SqlFunction function : sqlFunctions) {
List paramTypes = function.getParamTypes();
if (paramTypes == null) {
continue;
}
- final RelDataType paramType = paramTypes.get(i);
+ final RelDataType paramType = argNames != null
+ ? paramTypes.get(function.getParamNames().indexOf(argNames.get(i)))
Review comment:
@danny0405, good question!
This method is called after calling `filterRoutinesByParameterType()`, which
will return `false` for the case when the incorrect name was specified in
`argNames` list:
https://github.com/apache/calcite/blob/1baee8524a8daa9c67f08f83080e269fc5938bc5/core/src/main/java/org/apache/calcite/sql/SqlUtil.java#L611
So at this point, we will have a list of functions, that have all parameter
names from `argNames` list, if it is specified.
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 #1837: [CALCITE-3835] Overloaded table functions fail with an assertion error if param types differ
danny0405 commented on a change in pull request #1837: [CALCITE-3835]
Overloaded table functions fail with an assertion error if param types differ
URL: https://github.com/apache/calcite/pull/1837#discussion_r397766734
##
File path: core/src/main/java/org/apache/calcite/sql/SqlUtil.java
##
@@ -676,14 +680,16 @@ public static boolean matchRoutinesByParameterCount(
}
private static RelDataType bestMatch(List sqlFunctions, int i,
- RelDataTypePrecedenceList precList) {
+ List argNames, RelDataTypePrecedenceList precList) {
RelDataType bestMatch = null;
for (SqlFunction function : sqlFunctions) {
List paramTypes = function.getParamTypes();
if (paramTypes == null) {
continue;
}
- final RelDataType paramType = paramTypes.get(i);
+ final RelDataType paramType = argNames != null
+ ? paramTypes.get(function.getParamNames().indexOf(argNames.get(i)))
Review comment:
Does all kinds of functions return a non-empty list if the method call have
parameter names ? I don't think so, and how about
`function.getParamNames().indexOf(argNames.get(i))` returns `-1` there ?
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 #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
DonnyZone commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data … URL: https://github.com/apache/calcite/pull/1859#discussion_r397756517 ## File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java ## @@ -405,4 +405,92 @@ default RelDataType deriveDecimalModType(RelDataTypeFactory typeFactory, return null; } + /** + * Infers the return type of a decimal truncate operation. Decimal truncate + * involves at least one decimal operand. + * + * The default implementation is SQL:2003 compliant: the declared type of + * the result is the declared type of the first operand (decimal to be truncated). + * + * @see Glossary#SQL2003 SQL:2003 Part 2 Section 6.27 Review comment: My point is the document added in this PR. If it is not a standard manner, we'd better not add this confusing paragraph. > The default implementation is SQL:2003 compliant: the declared type of > the result is the declared type of the first operand (decimal to be truncated). > @see Glossary#SQL2003 SQL:2003 Part 2 Section 6.27 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 #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inferenc
DonnyZone commented on a change in pull request #1862: [CALCITE-3864] Add
Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and
correct the return type inference of SqlLibraryOperators.CONCAT_FUNCTION
URL: https://github.com/apache/calcite/pull/1862#discussion_r397748715
##
File path: core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java
##
@@ -655,6 +655,54 @@ public int size() {
return ret;
};
+
+ /**
+ * Type-inference strategy for String concatenation.
Review comment:
Improve the comment, you can refer to `DYADIC_STRING_SUM_PRECISION` for "||"
operator.
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 issue #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inference of SqlLibraryOper
DonnyZone commented on issue #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inference of SqlLibraryOperators.CONCAT_FUNCTION URL: https://github.com/apache/calcite/pull/1862#issuecomment-603760540 Moreover, (1) add several tests in `functions.iq`; (2) please revise the commit message & JIRA title & PR title as "Implement Concat function". 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 #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inferenc
DonnyZone commented on a change in pull request #1862: [CALCITE-3864] Add
Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and
correct the return type inference of SqlLibraryOperators.CONCAT_FUNCTION
URL: https://github.com/apache/calcite/pull/1862#discussion_r397741431
##
File path: core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java
##
@@ -655,6 +655,54 @@ public int size() {
return ret;
};
+
+ /**
+ * Type-inference strategy for String concatenation.
+ * For example,
+ *
+ * concat(cast('a' as varchar(2)), cast('b' as varchar(3)),cast('c' as
varchar(2)))
+ * returns varchar(7),
+ *
+ * concat(cast('a' as varchar), cast('b' as varchar(2), cast('c' as
varchar(2
+ * returns varchar,
+ *
+ * concat(cast('a' as varchar(65535)), cast('b' as varchar(2)), cast('c' as
varchar(2)))
+ * returns varchar
+ */
+ public static final SqlReturnTypeInference MULTIVALENT_STRING_SUM_PRECISION =
+ opBinding -> {
+boolean hasPrecisionNotSpecifiedOperand = false;
+int typePrecision;
+long amount = 0;
+List operandTypes = opBinding.collectOperandTypes();
+for (RelDataType operandType: operandTypes) {
Review comment:
You can directly break the iteration when: 1) encountering
`PRECISION_NOT_SPECIFIED`, 2) `amount` large than max precision.
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 edited a comment on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
danny0405 edited a comment on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data … URL: https://github.com/apache/calcite/pull/1859#issuecomment-603748315 > @danny0405 This is similar to #1311 which made some of the other decimal related rules configurable using a custom type system. > > Our understanding back then was this was the recommended way. Could you please elaborate on your approach if you think this is not the right way to achieve the same. In my opinion, #1311 is very different because `+` and `-` are builtin operators and we have no way for downstream projects to replace these operators, but `Round` and `Truncate` are different, they are sql functions, and if your project has maintained the sql operators by yourself, there is a chance you just override the type inference of these two instead of the whole type system, which seems too heavy and special to customize. 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 issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
danny0405 commented on issue #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data … URL: https://github.com/apache/calcite/pull/1859#issuecomment-603748315 > @danny0405 This is similar to #1311 which made some of the other decimal related rules configurable using a custom type system. > > Our understanding back then was this was the recommended way. Could you please elaborate on your approach if you think this is not the right way to achieve the same. In my opinion, #1311 is very different because `+` and `-` are builtin operators and we have no way for downstream projects to replace these operators, but `Round` and `Truncate` is different, they are sql functions, and if your project have maintained the sql operators by yourself, there is a chance you just override the type inference of these two instead of the whole type system, which seems too heavy and special to customize. 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] vlsi opened a new pull request #1874: Update Gradle: 6.1.1 -> 6.3
vlsi opened a new pull request #1874: Update Gradle: 6.1.1 -> 6.3 URL: https://github.com/apache/calcite/pull/1874 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 #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inferenc
DonnyZone commented on a change in pull request #1862: [CALCITE-3864] Add
Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and
correct the return type inference of SqlLibraryOperators.CONCAT_FUNCTION
URL: https://github.com/apache/calcite/pull/1862#discussion_r397714763
##
File path: core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
##
@@ -460,6 +460,14 @@ public static ByteString concat(ByteString s0, ByteString
s1) {
return s0.concat(s1);
}
+ /** SQL {@code concat(string0, string2, string3, ...)} function. */
Review comment:
To align with `args`, "arg0, arg1..." is better than "string0, .." in doc.
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 #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inferenc
DonnyZone commented on a change in pull request #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inference of SqlLibraryOperators.CONCAT_FUNCTION URL: https://github.com/apache/calcite/pull/1862#discussion_r397713213 ## File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java ## @@ -332,6 +334,8 @@ NullPolicy.STRICT); defineMethod(CONCAT, BuiltInMethod.STRING_CONCAT.method, NullPolicy.STRICT); +defineMethod(CONCAT2, BuiltInMethod.STRING_CONCAT.method, NullPolicy.STRICT); Review comment: Maybe there is no need to register `CONCAT2` into `RexImpTable`. In runtime, Calcite can automatically find the concat function. You can have a try. 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 #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inferenc
DonnyZone commented on a change in pull request #1862: [CALCITE-3864] Add
Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and
correct the return type inference of SqlLibraryOperators.CONCAT_FUNCTION
URL: https://github.com/apache/calcite/pull/1862#discussion_r397711754
##
File path: core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java
##
@@ -460,6 +460,14 @@ public static ByteString concat(ByteString s0, ByteString
s1) {
return s0.concat(s1);
}
+ /** SQL {@code concat(string0, string2, string3, ...)} function. */
+ public static String concat(String... args) {
+List argList = Arrays.asList(args);
Review comment:
You can use `String.join("", args)`.
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] praveenbingo commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
praveenbingo commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data … URL: https://github.com/apache/calcite/pull/1859#discussion_r397710842 ## File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java ## @@ -405,4 +405,92 @@ default RelDataType deriveDecimalModType(RelDataTypeFactory typeFactory, return null; } + /** + * Infers the return type of a decimal truncate operation. Decimal truncate + * involves at least one decimal operand. + * + * The default implementation is SQL:2003 compliant: the declared type of + * the result is the declared type of the first operand (decimal to be truncated). + * + * @see Glossary#SQL2003 SQL:2003 Part 2 Section 6.27 Review comment: There is not a consistent one from what we found SQL Server seems to go with what calcite does where as Oracle does not. Also i think that is outside the scope of this PR.. 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] wenhuitang commented on issue #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inference of SqlLibraryOpe
wenhuitang commented on issue #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inference of SqlLibraryOperators.CONCAT_FUNCTION URL: https://github.com/apache/calcite/pull/1862#issuecomment-603733532 > There is no confusion. From user's perspective, when the library is specified, the function can be registered correctly. These two functions are placed in two distinct slots. According to you comments, this pull request has been updated, thanks a lot. 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 #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data …
DonnyZone commented on a change in pull request #1859: [CALCITE-3863] Make Truncate/Round return type inference overridable through rel data … URL: https://github.com/apache/calcite/pull/1859#discussion_r397681534 ## File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeSystem.java ## @@ -405,4 +405,92 @@ default RelDataType deriveDecimalModType(RelDataTypeFactory typeFactory, return null; } + /** + * Infers the return type of a decimal truncate operation. Decimal truncate + * involves at least one decimal operand. + * + * The default implementation is SQL:2003 compliant: the declared type of + * the result is the declared type of the first operand (decimal to be truncated). + * + * @see Glossary#SQL2003 SQL:2003 Part 2 Section 6.27 Review comment: I know SQL standard document the general operators (`+`, `-`, `*`, `/`, `%`) for decimal types. But can we find the guideline in SQL standard for `truncate` and `round`? 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] liyafan82 opened a new pull request #1873: [CALCITE-3872] Simplify expressions with unary minus
liyafan82 opened a new pull request #1873: [CALCITE-3872] Simplify expressions with unary minus URL: https://github.com/apache/calcite/pull/1873 Support simplifying expression - ( - ( x ) ) as x. 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 issue #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inference of SqlLibraryOper
DonnyZone commented on issue #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inference of SqlLibraryOperators.CONCAT_FUNCTION URL: https://github.com/apache/calcite/pull/1862#issuecomment-603674884 There is no confusion. From user's perspective, when the library is specified, the function can be registered correctly. These two functions are placed in two distinct slots. 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] wenhuitang removed a comment on issue #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inference of SqlLi
wenhuitang removed a comment on issue #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inference of SqlLibraryOperators.CONCAT_FUNCTION URL: https://github.com/apache/calcite/pull/1862#issuecomment-603669239 > (1)In Oracle, `concat` function accepts only two operands. In `SqlLibraryOperators`, We'd better seperate the `CONCAT_FUNCTION` into `CONCAT_FUNCTION` and `ORACLE_CONCAT_FUNCTION`. For `ORACLE_CONCAT_FUNCTION`, we can use `SqlOperandCountRanges.of(2)` to enable the constraint. How about seperate the `CONCAT_FUNCTION` into `CONCAT_FUNCTION` which uses SqlOperandCountRanges.from(3) and `CONCAT2` which uses `SqlOperandCountRanges.of(2)`? And the libraries for `CONCAT_FUNCTION` are PG and MySql. The libraries for `CONCAT2` are Oracle, PG and MySQL .Since currently `CONCAT_FUNCTION` which use SqlOperandCountRanges.from(2) contains the case of concat2 that may lead to confusion. 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 opened a new pull request #1872: [CALCITE-3871] Remove dependency of org.apiguardian:apiguardian-api
danny0405 opened a new pull request #1872: [CALCITE-3871] Remove dependency of org.apiguardian:apiguardian-api URL: https://github.com/apache/calcite/pull/1872 Also fix the pigunit version to 0.16.0. 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] wenhuitang edited a comment on issue #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inference of SqlLib
wenhuitang edited a comment on issue #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inference of SqlLibraryOperators.CONCAT_FUNCTION URL: https://github.com/apache/calcite/pull/1862#issuecomment-603669239 > (1)In Oracle, `concat` function accepts only two operands. In `SqlLibraryOperators`, We'd better seperate the `CONCAT_FUNCTION` into `CONCAT_FUNCTION` and `ORACLE_CONCAT_FUNCTION`. For `ORACLE_CONCAT_FUNCTION`, we can use `SqlOperandCountRanges.of(2)` to enable the constraint. How about seperate the `CONCAT_FUNCTION` into `CONCAT_FUNCTION` which uses SqlOperandCountRanges.from(3) and `CONCAT2` which uses `SqlOperandCountRanges.of(2)`? And the libraries for `CONCAT_FUNCTION` are PG and MySql. The libraries for `CONCAT2` are Oracle, PG and MySQL .Since currently `CONCAT_FUNCTION` which use SqlOperandCountRanges.from(2) contains the case of concat2 that may lead to confusion. 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] wenhuitang commented on issue #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inference of SqlLibraryOpe
wenhuitang commented on issue #1862: [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inference of SqlLibraryOperators.CONCAT_FUNCTION URL: https://github.com/apache/calcite/pull/1862#issuecomment-603669239 > (1)In Oracle, `concat` function accepts only two operands. In `SqlLibraryOperators`, We'd better seperate the `CONCAT_FUNCTION` into `CONCAT_FUNCTION` and `ORACLE_CONCAT_FUNCTION`. For `ORACLE_CONCAT_FUNCTION`, we can use `SqlOperandCountRanges.of(2)` to enable the constraint. How about seperate the `CONCAT_FUNCTION` into `CONCAT_FUNCTION` which uses SqlOperandCountRanges.from(3) and `CONCAT2` which uses `SqlOperandCountRanges.of(2)`? And the libraries for `CONCAT_FUNCTION` are PG and MySql. The libraries for `CONCAT2` are Oracle, PG and MySQL .Since currently `CONCAT_FUNCTION` which use SqlOperandCountRanges.from(2) contains the case of concat2 that may lead to confusion. 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
