You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "mihaibudiu (via GitHub)" <gi...@apache.org> on 2024/09/04 06:39:46 UTC
[PR] [CALCITE-4918] Add a VARIANT data type [calcite]
mihaibudiu opened a new pull request, #3947:
URL: https://github.com/apache/calcite/pull/3947
This PR introduces a `VARIANT` SQL data type, based on the design in Snowflake: https://docs.snowflake.com/en/sql-reference/data-types-semistructured. `VARIANT` is a much better base to build support for JSON that the existing functions in Calcite. All the existing Calcite JSON functions assume that JSON is stored as an unparsed string. `VARIANT` can represent any JSON document in a "binary" form (and much more than JSON!)
The PR is divided into two commits:
- the parser and validator support
- the runtime support
Turns out that the validator support is almost trivial. Most of the work is in the runtime, where a new kind of value must be introduced, Variant, which carries runtime type information. For this purpose a new class hierarchy has been introduced, rooted at `RuntimeTypeInformation`.
Currently the runtime type information carries precision and scale, but I am not sure that these are actually necessary. I will study this a bit more and may amend this in a subsequent commit.
There are no functions supporting `VARIANT` at this point, but `VARIANTS` can still be created using casts, and accessed using variant.field, variant[index], and variant['key'].
Subsequent pull requests are expected to add more functions. In particular, functions to parse and unparse JSON into variants.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2433726544
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [27 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [51.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r1744655594
##########
core/src/main/java/org/apache/calcite/util/rtti/RuntimeTypeInformation.java:
##########
@@ -0,0 +1,228 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util.rtti;
+
+import org.apache.calcite.linq4j.tree.Expression;
+import org.apache.calcite.linq4j.tree.Expressions;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeField;
+
+import java.util.AbstractMap;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * This class represents the type of a SQL expression at runtime.
Review Comment:
Remove 'This class represents'
##########
core/src/main/java/org/apache/calcite/util/Variant.java:
##########
@@ -0,0 +1,181 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
Review Comment:
The `util` package feels like the wrong place (for the reasons I described in `rtti`).
How about `org.apache.calcite.runtime`? The classes in that package are intended for use by generated code, rather than user code. But it has a similar goal to have few dependencies.
(`DateString`, `TimeString`, `TimestampString` should have been there too.)
##########
core/src/main/java/org/apache/calcite/util/Variant.java:
##########
@@ -0,0 +1,181 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
+
+import org.apache.calcite.runtime.SqlFunctions;
+import org.apache.calcite.sql.type.BasicSqlType;
+import org.apache.calcite.util.rtti.BasicSqlTypeRtti;
+import org.apache.calcite.util.rtti.RuntimeTypeInformation;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.Objects;
+
+/** This class is the runtime support for values of the VARIANT SQL type. */
+public class Variant {
+ /** Actual value. */
+ final @Nullable Object value;
Review Comment:
Making `value` nullable probably complicates a lot of code. (If the experience with `RexLiteral` is anything to go by.) Could you have a subclass for null variants?
##########
core/src/main/java/org/apache/calcite/util/rtti/RuntimeTypeInformation.java:
##########
@@ -0,0 +1,228 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util.rtti;
+
+import org.apache.calcite.linq4j.tree.Expression;
+import org.apache.calcite.linq4j.tree.Expressions;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeField;
+
+import java.util.AbstractMap;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * This class represents the type of a SQL expression at runtime.
+ * Normally SQL is a statically-typed language, and there is no need for
+ * runtime-type information. However, the VARIANT data type is actually
+ * a dynamically-typed value, and needs this kind of information.
+ * We cannot use the very similar RelDataType type since it carries extra
+ * baggage, like the type system, which is not available at runtime. */
+public abstract class RuntimeTypeInformation {
Review Comment:
I get the impression that `RuntimeTypeInformation` is connected with either the enumerable engine, or as part of the Java API (which is used to define Java UDFs). If it is either of those, I don't think it belongs in (a subpackage of) the util package.
##########
core/src/main/java/org/apache/calcite/util/rtti/RuntimeTypeInformation.java:
##########
@@ -0,0 +1,228 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util.rtti;
+
+import org.apache.calcite.linq4j.tree.Expression;
+import org.apache.calcite.linq4j.tree.Expressions;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeField;
+
+import java.util.AbstractMap;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * This class represents the type of a SQL expression at runtime.
+ * Normally SQL is a statically-typed language, and there is no need for
+ * runtime-type information. However, the VARIANT data type is actually
+ * a dynamically-typed value, and needs this kind of information.
+ * We cannot use the very similar RelDataType type since it carries extra
+ * baggage, like the type system, which is not available at runtime. */
+public abstract class RuntimeTypeInformation {
+ /** Names of SQL types as represented at runtime. */
+ public enum RuntimeSqlTypeName {
+ BOOLEAN(false),
+ TINYINT(false),
+ SMALLINT(false),
+ INTEGER(false),
+ BIGINT(false),
+ DECIMAL(false),
+ FLOAT(false),
+ REAL(false),
+ DOUBLE(false),
+ DATE(false),
+ TIME(false),
+ TIME_WITH_LOCAL_TIME_ZONE(false),
+ TIME_TZ(false),
+ TIMESTAMP(false),
+ TIMESTAMP_WITH_LOCAL_TIME_ZONE(false),
+ TIMESTAMP_TZ(false),
+ INTERVAL_LONG(false),
+ INTERVAL_SHORT(false),
+ CHAR(false),
+ VARCHAR(false),
+ BINARY(false),
+ VARBINARY(false),
+ NULL(false),
+ MULTISET(true),
+ ARRAY(true),
+ MAP(true),
+ ROW(true),
+ GEOMETRY(false),
+ // used only for VARIANT.null value
+ VARIANT(false);
+
+ private final boolean composite;
+
+ RuntimeSqlTypeName(boolean composite) {
+ this.composite = composite;
+ }
+
+ public boolean isScalar() {
+ return !this.composite;
+ }
+ }
+
+ final RuntimeSqlTypeName typeName;
+
+ protected RuntimeTypeInformation(RuntimeSqlTypeName typeName) {
+ this.typeName = typeName;
+ }
+
+ public abstract String getTypeString();
+
+ public RuntimeSqlTypeName getTypeName() {
+ return this.typeName;
+ }
+
+ public boolean isScalar() {
+ return this.typeName.isScalar();
+ }
+
+ /**
+ * Create and return an expression that creates a runtime type that
Review Comment:
s/Create/Creates/
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2333160487
@julianhyde I have hopefully implemented your suggestions. The structure is much better this way.
I have added TYPEOF and VARIANTNULL functions. TYPEOF is very useful for debugging.
The variant.iq file will grow as we add more functions.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "JinkunLiu (via GitHub)" <gi...@apache.org>.
JinkunLiu commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r2607601736
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlItemOperator.java:
##########
@@ -136,7 +137,8 @@ private static SqlSingleOperandTypeChecker getChecker(SqlCallBinding callBinding
if (name.equals("ITEM")) {
return "<ARRAY>[<INTEGER>]\n"
+ "<MAP>[<ANY>]\n"
- + "<ROW>[<CHARACTER>|<INTEGER>]";
+ + "<ROW>[<CHARACTER>|<INTEGER>]\n"
+ + "<VARIANT>[<ANY>]";
Review Comment:
Might this need to be <VARIANT>[<CHARACTER>|<INTEGER>] to align with the definition on line 128?
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2350116779
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [25 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [51.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2518435889
Looks good. Thanks for adding `variant.iq` - that's the first place I would direct someone who wants to understand this feature.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "JinkunLiu (via GitHub)" <gi...@apache.org>.
JinkunLiu commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r2607601736
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlItemOperator.java:
##########
@@ -136,7 +137,8 @@ private static SqlSingleOperandTypeChecker getChecker(SqlCallBinding callBinding
if (name.equals("ITEM")) {
return "<ARRAY>[<INTEGER>]\n"
+ "<MAP>[<ANY>]\n"
- + "<ROW>[<CHARACTER>|<INTEGER>]";
+ + "<ROW>[<CHARACTER>|<INTEGER>]\n"
+ + "<VARIANT>[<ANY>]";
Review Comment:
I think this should be `<VARIANT>[<CHARACTER>|<INTEGER>]`, as defined on line 128.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2333741616
Implicitly cast is an important part of VARIANT type, will it be supported in 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2334664152
@mihaibudiu Thanks for all your edits. Much improved.
+1 after you fix the cosmetic issues in reference.md.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2329610011
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [23 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [54.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r1870564738
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -866,6 +868,8 @@ void populate1() {
defineMethod(TRUNC_BIG_QUERY, BuiltInMethod.STRUNCATE.method, NullPolicy.STRICT);
defineMethod(TRUNCATE, BuiltInMethod.STRUNCATE.method, NullPolicy.STRICT);
defineMethod(LOG1P, BuiltInMethod.LOG1P.method, NullPolicy.STRICT);
+ defineMethod(TYPEOF, BuiltInMethod.TYPEOF.method, NullPolicy.STRICT);
Review Comment:
Ok. I noticed this function parameter is `VARIANT` now.
According to https://docs.databricks.com/en/sql/language-manual/functions/typeof.html.
The parameter type can be any expression.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r1871600970
##########
core/src/main/java/org/apache/calcite/runtime/rtti/BasicSqlTypeRtti.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.runtime.rtti;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.Objects;
+
+/** Runtime type information about a base (primitive) SQL type. */
+public class BasicSqlTypeRtti extends RuntimeTypeInformation {
+ public BasicSqlTypeRtti(RuntimeSqlTypeName typeName) {
+ super(typeName);
+ assert typeName.isScalar() : "Base SQL type must be a scalar type " + typeName;
+ }
+
+ @Override public boolean equals(@Nullable Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+
+ BasicSqlTypeRtti that = (BasicSqlTypeRtti) o;
+ return typeName == that.typeName;
+ }
+
+ @Override public int hashCode() {
+ return Objects.hashCode(typeName);
+ }
+
+ @Override public String getTypeString() {
+ switch (this.typeName) {
Review Comment:
what do you mean by "merge"?
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2524955212
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [28 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [50.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2536851862
I will squash the commits in preparation for merging, please let me know if you have objections.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r1769696994
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -1722,6 +1722,68 @@ void testCastToBoolean(CastType castType, SqlOperatorFixture f) {
f.checkNull("cast(null as row(f0 varchar, f1 varchar))");
}
+ /** Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4918">
+ * [CALCITE-4918] Add a VARIANT data type</a>. */
Review Comment:
There is a redundant space
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r2607856589
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlItemOperator.java:
##########
@@ -136,7 +137,8 @@ private static SqlSingleOperandTypeChecker getChecker(SqlCallBinding callBinding
if (name.equals("ITEM")) {
return "<ARRAY>[<INTEGER>]\n"
+ "<MAP>[<ANY>]\n"
- + "<ROW>[<CHARACTER>|<INTEGER>]";
+ + "<ROW>[<CHARACTER>|<INTEGER>]\n"
+ + "<VARIANT>[<ANY>]";
Review Comment:
This may require filing a new JIRA issue. If you can figure out what the right solution would be, even better, you can include it in the issue.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "JinkunLiu (via GitHub)" <gi...@apache.org>.
JinkunLiu commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r2607601736
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlItemOperator.java:
##########
@@ -136,7 +137,8 @@ private static SqlSingleOperandTypeChecker getChecker(SqlCallBinding callBinding
if (name.equals("ITEM")) {
return "<ARRAY>[<INTEGER>]\n"
+ "<MAP>[<ANY>]\n"
- + "<ROW>[<CHARACTER>|<INTEGER>]";
+ + "<ROW>[<CHARACTER>|<INTEGER>]\n"
+ + "<VARIANT>[<ANY>]";
Review Comment:
Might this need to be `<VARIANT>[<CHARACTER>|<INTEGER>]` to align with the definition on line 128? ? If so, I'm happy to submit a fix.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2524953060
In the last commit I have reworked the runtime implementation of VARIANT.
(We have tested this design in a different backend for a while, and it works pretty well.)
In the previous implementation the runtime type for generic types like ARRAY or MAP would hold information about the element types. In the current implementation this is no longer true: when converting an array to a variant all elements are converted to VARIANT as well. Same for a MAP. Converting a ROW to a VARIANT generates a MAP indexed by the field names.
For the user there isn't much of a difference, and, as you may notice, the tests have changed very little. The runtime cost is different, though. Neither of the schemes dominates the other, whether one is preferable depends on the workload.
I believe that this is similar to what Snowflake does, although I could not find a precise description of their exact implementation. But I believe that the TYPEOF function in Snowflake applied to an ARRAY will return just "ARRAY" and not "INT ARRAY" - so it resembles more the current implementation.
VARIANT shines at handling JSON, so in future PRs we should add more JSON support. Unlike the existing Calcite JSON support, VARIANT represents the JSON natively, and does not need to convert back and forth to strings on every operation, saving many resources.
I will give potential readers a few more days to review 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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #3947:
URL: https://github.com/apache/calcite/pull/3947
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2333393437
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [21 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [57.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2448095696
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [27 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [51.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2329764096
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [23 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [54.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2536899212
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [27 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [50.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2479949710
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [17 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [51.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2334750958
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [21 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [57.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r1744660968
##########
core/src/main/java/org/apache/calcite/util/Variant.java:
##########
@@ -0,0 +1,181 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
+
+import org.apache.calcite.runtime.SqlFunctions;
+import org.apache.calcite.sql.type.BasicSqlType;
+import org.apache.calcite.util.rtti.BasicSqlTypeRtti;
+import org.apache.calcite.util.rtti.RuntimeTypeInformation;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.Objects;
+
+/** This class is the runtime support for values of the VARIANT SQL type. */
+public class Variant {
+ /** Actual value. */
+ final @Nullable Object value;
Review Comment:
Making `value` nullable probably complicates a lot of code. (If the experience with `RexLiteral` is anything to go by.) Could you have a subclass for null variants (or some other solution)?
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2518442295
We are actually using this feature extensively in our Rust-based runtime, but our runtime implementation is slightly different. I plan to make the Java implementation a bit closer.
This is a *very* powerful feature. Once you have variant you can do many things which are very difficult otherwise, including recursive data types (like JSON). One other thing we use VARIANT for is error reporting. You can have a dynamic error type (a VARIANT MAP) which can include any other type, including any SQL record inside!
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2518458355
I agree with your remark that variants are difficult to use without TYPEOF. We could also add something like this (apologies for the crappy syntax, hopefully you get the idea):
```
CASE v
WHENINSTANCEOF number THEN v + 5
WHENINSTANCEOF varchar THEN length(v)
ELSE 0
END
```
I believe Java has something like this and maybe some SQL dialects do 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "JinkunLiu (via GitHub)" <gi...@apache.org>.
JinkunLiu commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r2607809698
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlItemOperator.java:
##########
@@ -136,7 +137,8 @@ private static SqlSingleOperandTypeChecker getChecker(SqlCallBinding callBinding
if (name.equals("ITEM")) {
return "<ARRAY>[<INTEGER>]\n"
+ "<MAP>[<ANY>]\n"
- + "<ROW>[<CHARACTER>|<INTEGER>]";
+ + "<ROW>[<CHARACTER>|<INTEGER>]\n"
+ + "<VARIANT>[<ANY>]";
Review Comment:
You're right — in Snowflake, a VARIANT value can be an OBJECT (map) or an ARRAY.
However, a Snowflake OBJECT is a special kind of map: its keys must be strings, and its values are VARIANT types (i.e., `MAP<VARCHAR, VARIANT>`).
As [Snowflake's official documentation](https://docs.snowflake.com/en/sql-reference/data-types-semistructured#object) states:
> In Snowflake's semi-structured OBJECT data, each key is of type VARCHAR, and each value is of type VARIANT.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "JinkunLiu (via GitHub)" <gi...@apache.org>.
JinkunLiu commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r2607601736
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlItemOperator.java:
##########
@@ -136,7 +137,8 @@ private static SqlSingleOperandTypeChecker getChecker(SqlCallBinding callBinding
if (name.equals("ITEM")) {
return "<ARRAY>[<INTEGER>]\n"
+ "<MAP>[<ANY>]\n"
- + "<ROW>[<CHARACTER>|<INTEGER>]";
+ + "<ROW>[<CHARACTER>|<INTEGER>]\n"
+ + "<VARIANT>[<ANY>]";
Review Comment:
Might this need to be `<VARIANT>[<CHARACTER>|<INTEGER>]` to align with the definition on line 128?
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "JinkunLiu (via GitHub)" <gi...@apache.org>.
JinkunLiu commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r2607809698
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlItemOperator.java:
##########
@@ -136,7 +137,8 @@ private static SqlSingleOperandTypeChecker getChecker(SqlCallBinding callBinding
if (name.equals("ITEM")) {
return "<ARRAY>[<INTEGER>]\n"
+ "<MAP>[<ANY>]\n"
- + "<ROW>[<CHARACTER>|<INTEGER>]";
+ + "<ROW>[<CHARACTER>|<INTEGER>]\n"
+ + "<VARIANT>[<ANY>]";
Review Comment:
You're right — in Snowflake, a VARIANT value can be an OBJECT (map) or an ARRAY. However, a Snowflake OBJECT is a special kind of map: its keys must be strings and its values are VARIANTs (i.e., map<string, VARIANT>). See Snowflake's documentation: https://docs.snowflake.com/en/sql-reference/data-types-semistructured#object
```
In Snowflake's semi-structured OBJECT data, each key is of type VARCHAR and each value is of type VARIANT.
```
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2433678776
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [27 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [51.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2334443593
Yes, let's leave implicit casts for later.
I think we can add coercions fairly easily, but other implicit casts may require much more work due to the way Calcite handles them.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r1744667636
##########
core/src/main/java/org/apache/calcite/util/rtti/RuntimeTypeInformation.java:
##########
@@ -0,0 +1,228 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util.rtti;
+
+import org.apache.calcite.linq4j.tree.Expression;
+import org.apache.calcite.linq4j.tree.Expressions;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeField;
+
+import java.util.AbstractMap;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * This class represents the type of a SQL expression at runtime.
+ * Normally SQL is a statically-typed language, and there is no need for
+ * runtime-type information. However, the VARIANT data type is actually
+ * a dynamically-typed value, and needs this kind of information.
+ * We cannot use the very similar RelDataType type since it carries extra
+ * baggage, like the type system, which is not available at runtime. */
+public abstract class RuntimeTypeInformation {
+ /** Names of SQL types as represented at runtime. */
+ public enum RuntimeSqlTypeName {
+ BOOLEAN(false),
+ TINYINT(false),
+ SMALLINT(false),
+ INTEGER(false),
+ BIGINT(false),
+ DECIMAL(false),
+ FLOAT(false),
+ REAL(false),
+ DOUBLE(false),
+ DATE(false),
+ TIME(false),
+ TIME_WITH_LOCAL_TIME_ZONE(false),
+ TIME_TZ(false),
+ TIMESTAMP(false),
+ TIMESTAMP_WITH_LOCAL_TIME_ZONE(false),
+ TIMESTAMP_TZ(false),
+ INTERVAL_LONG(false),
+ INTERVAL_SHORT(false),
+ CHAR(false),
+ VARCHAR(false),
+ BINARY(false),
+ VARBINARY(false),
+ NULL(false),
+ MULTISET(true),
+ ARRAY(true),
+ MAP(true),
+ ROW(true),
+ GEOMETRY(false),
+ // used only for VARIANT.null value
+ VARIANT(false);
+
+ private final boolean composite;
+
+ RuntimeSqlTypeName(boolean composite) {
+ this.composite = composite;
+ }
+
+ public boolean isScalar() {
+ return !this.composite;
+ }
+ }
+
+ final RuntimeSqlTypeName typeName;
+
+ protected RuntimeTypeInformation(RuntimeSqlTypeName typeName) {
+ this.typeName = typeName;
+ }
+
+ public abstract String getTypeString();
+
+ public RuntimeSqlTypeName getTypeName() {
+ return this.typeName;
+ }
+
+ public boolean isScalar() {
+ return this.typeName.isScalar();
+ }
+
+ /**
+ * Create and return an expression that creates a runtime type that
Review Comment:
... and generally, please follow the best practice of using third-person indicative for method javadoc.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2515875994
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [27 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [51.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r1870052170
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -1347,6 +1347,45 @@ void testLikeAndSimilarFails() {
.columnType("TIMESTAMP_TZ(3) NOT NULL");
}
+ @Test void testCastVariant() {
+ expr("cast(NULL as variant)")
+ .columnType("VARIANT");
+ expr("cast(1 as variant)")
+ .columnType("VARIANT NOT NULL");
+ expr("cast('abc' as variant)")
+ .columnType("VARIANT NOT NULL");
+ expr("cast(TIMESTAMP '2024-09-01 00:00:00' as variant)")
+ .columnType("VARIANT NOT NULL");
+ expr("cast(ARRAY[1,2,3] AS VARIANT)")
Review Comment:
I plan to rework a bit this PR to bring it more in line with what Snowflake does.
In this case converting an ARRAY to a variant produces a VARIANT whose value is an array of variant. So the elements are all recursively converted to VARIANT. Same thing is true for MAP.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2547057618
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [27 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [50.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2329500646
Can you move (or at least copy) that description to the Jira case? Jira cases are our feature specifications. Minimal features thatI would like to see specified:
* How VARIANT interacts with JDBC
* Whether there is an 'instanceof' operator
* Behavior in CAST (and implicit conversion)
* How variants are converted to and from strings
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2329656162
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [23 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [54.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r1769697163
##########
core/src/main/java/org/apache/calcite/runtime/variant/VariantNull.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.runtime.variant;
+
+import org.apache.calcite.runtime.rtti.RuntimeTypeInformation;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+/** The VARIANT type has its own notion of null, which is
Review Comment:
Maybe adding a line break would be better.
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -1722,6 +1722,68 @@ void testCastToBoolean(CastType castType, SqlOperatorFixture f) {
f.checkNull("cast(null as row(f0 varchar, f1 varchar))");
}
+ /** Test cases for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4918">
+ * [CALCITE-4918] Add a VARIANT data type</a>. */
Review Comment:
One more space
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2525032271
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [28 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [50.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r1871593297
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -866,6 +868,8 @@ void populate1() {
defineMethod(TRUNC_BIG_QUERY, BuiltInMethod.STRUNCATE.method, NullPolicy.STRICT);
defineMethod(TRUNCATE, BuiltInMethod.STRUNCATE.method, NullPolicy.STRICT);
defineMethod(LOG1P, BuiltInMethod.LOG1P.method, NullPolicy.STRICT);
+ defineMethod(TYPEOF, BuiltInMethod.TYPEOF.method, NullPolicy.STRICT);
Review Comment:
This function can be generalized, but it wasn't in scope for 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2334498485
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [21 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [57.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "JinkunLiu (via GitHub)" <gi...@apache.org>.
JinkunLiu commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r2607601736
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlItemOperator.java:
##########
@@ -136,7 +137,8 @@ private static SqlSingleOperandTypeChecker getChecker(SqlCallBinding callBinding
if (name.equals("ITEM")) {
return "<ARRAY>[<INTEGER>]\n"
+ "<MAP>[<ANY>]\n"
- + "<ROW>[<CHARACTER>|<INTEGER>]";
+ + "<ROW>[<CHARACTER>|<INTEGER>]\n"
+ + "<VARIANT>[<ANY>]";
Review Comment:
Might this need to be `<VARIANT>[<CHARACTER>|<INTEGER>]` to align with the definition on line 128?
If so, I'm happy to submit a fix.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r2607669538
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlItemOperator.java:
##########
@@ -136,7 +137,8 @@ private static SqlSingleOperandTypeChecker getChecker(SqlCallBinding callBinding
if (name.equals("ITEM")) {
return "<ARRAY>[<INTEGER>]\n"
+ "<MAP>[<ANY>]\n"
- + "<ROW>[<CHARACTER>|<INTEGER>]";
+ + "<ROW>[<CHARACTER>|<INTEGER>]\n"
+ + "<VARIANT>[<ANY>]";
Review Comment:
Well, the variant value can be a MAP, and the map key can have any type, so it should be possible to have keys with other types. But perhaps that case requires first casting the variant to a MAP?
Clearly, one of the two is wrong, though.
Can you investigate what happens in systems like Snowflake?
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r1868876707
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -866,6 +868,8 @@ void populate1() {
defineMethod(TRUNC_BIG_QUERY, BuiltInMethod.STRUNCATE.method, NullPolicy.STRICT);
defineMethod(TRUNCATE, BuiltInMethod.STRUNCATE.method, NullPolicy.STRICT);
defineMethod(LOG1P, BuiltInMethod.LOG1P.method, NullPolicy.STRICT);
+ defineMethod(TYPEOF, BuiltInMethod.TYPEOF.method, NullPolicy.STRICT);
Review Comment:
`TYPEOF` is a new function in CALCITE. Can we use a split Jira issue to support it?
##########
site/_docs/reference.md:
##########
@@ -1239,14 +1240,66 @@ Note:
| MAP | Collection of keys mapped to values | Example: map < int, varchar >
| MULTISET | Unordered collection that may contain duplicates | Example: int multiset
| ARRAY | Ordered, contiguous collection that may contain duplicates | Example: varchar(10) array
-| CURSOR | Cursor over the result of executing a query |
Review Comment:
Do we need to remove this `|`? ANY and UNKNOWN also end with `|`;
##########
core/src/main/java/org/apache/calcite/runtime/rtti/RuntimeTypeInformation.java:
##########
@@ -0,0 +1,243 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.runtime.rtti;
+
+import org.apache.calcite.linq4j.tree.Expression;
+import org.apache.calcite.linq4j.tree.Expressions;
+import org.apache.calcite.linq4j.tree.Primitive;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeField;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.AbstractMap;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * The type of a SQL expression at runtime.
+ * Normally SQL is a statically-typed language, and there is no need for
+ * runtime-type information. However, the VARIANT data type is actually
Review Comment:
There is an extra space before `However`.
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -1347,6 +1347,45 @@ void testLikeAndSimilarFails() {
.columnType("TIMESTAMP_TZ(3) NOT NULL");
}
+ @Test void testCastVariant() {
+ expr("cast(NULL as variant)")
+ .columnType("VARIANT");
+ expr("cast(1 as variant)")
+ .columnType("VARIANT NOT NULL");
+ expr("cast('abc' as variant)")
+ .columnType("VARIANT NOT NULL");
+ expr("cast(TIMESTAMP '2024-09-01 00:00:00' as variant)")
+ .columnType("VARIANT NOT NULL");
+ expr("cast(ARRAY[1,2,3] AS VARIANT)")
Review Comment:
If we convert `cast(ARRAY[1,2,3, null] AS VARIANT`, What's the columnType?
##########
core/src/main/java/org/apache/calcite/runtime/rtti/BasicSqlTypeRtti.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.runtime.rtti;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.Objects;
+
+/** Runtime type information about a base (primitive) SQL type. */
+public class BasicSqlTypeRtti extends RuntimeTypeInformation {
+ public BasicSqlTypeRtti(RuntimeSqlTypeName typeName) {
+ super(typeName);
+ assert typeName.isScalar() : "Base SQL type must be a scalar type " + typeName;
+ }
+
+ @Override public boolean equals(@Nullable Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+
+ BasicSqlTypeRtti that = (BasicSqlTypeRtti) o;
+ return typeName == that.typeName;
+ }
+
+ @Override public int hashCode() {
+ return Objects.hashCode(typeName);
+ }
+
+ @Override public String getTypeString() {
+ switch (this.typeName) {
Review Comment:
Can we merge some types? Most of the names of the types here have not changed.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r1870048705
##########
core/src/main/java/org/apache/calcite/runtime/rtti/RuntimeTypeInformation.java:
##########
@@ -0,0 +1,243 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.runtime.rtti;
+
+import org.apache.calcite.linq4j.tree.Expression;
+import org.apache.calcite.linq4j.tree.Expressions;
+import org.apache.calcite.linq4j.tree.Primitive;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeField;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.AbstractMap;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * The type of a SQL expression at runtime.
+ * Normally SQL is a statically-typed language, and there is no need for
+ * runtime-type information. However, the VARIANT data type is actually
Review Comment:
Yes, that's how I learned to type, emacs encourages this forfmatting for end of sentence.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r1870046805
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -866,6 +868,8 @@ void populate1() {
defineMethod(TRUNC_BIG_QUERY, BuiltInMethod.STRUNCATE.method, NullPolicy.STRICT);
defineMethod(TRUNCATE, BuiltInMethod.STRUNCATE.method, NullPolicy.STRICT);
defineMethod(LOG1P, BuiltInMethod.LOG1P.method, NullPolicy.STRICT);
+ defineMethod(TYPEOF, BuiltInMethod.TYPEOF.method, NullPolicy.STRICT);
Review Comment:
VARIANT without TYPEOF is of not much use.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2525275201
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [28 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [50.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r1747601705
##########
site/_docs/reference.md:
##########
@@ -1232,17 +1232,17 @@ Note:
### Non-scalar types
-| Type | Description | Example type
-|:-------- |:---------------------------|:---------------
-| ANY | The union of all types |
+| Type | Description | Example type |
Review Comment:
Our markdown style is to not have a `|` after the last column. Can you remove it.
(Intellij does not respect that style, so its edits need to be reverted.)
Also revert the changes in spacing, which become spurious diffs.
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2334726435
I will leave this PR open for a few more days in case people want to comment. Otherwise I have tagged it with "merge 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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3947:
URL: https://github.com/apache/calcite/pull/3947#issuecomment-2423017784
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947) **Quality Gate passed**
Issues
 [27 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3947&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3947&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [51.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3947&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3947)
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "JinkunLiu (via GitHub)" <gi...@apache.org>.
JinkunLiu commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r2607809698
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlItemOperator.java:
##########
@@ -136,7 +137,8 @@ private static SqlSingleOperandTypeChecker getChecker(SqlCallBinding callBinding
if (name.equals("ITEM")) {
return "<ARRAY>[<INTEGER>]\n"
+ "<MAP>[<ANY>]\n"
- + "<ROW>[<CHARACTER>|<INTEGER>]";
+ + "<ROW>[<CHARACTER>|<INTEGER>]\n"
+ + "<VARIANT>[<ANY>]";
Review Comment:
You're right — in Snowflake, a VARIANT value can be an OBJECT (map) or an ARRAY.
However, a Snowflake OBJECT is a special kind of map: its keys must be strings, and its values are VARIANT types (i.e., `map<string, VARIANT>`).
As [Snowflake's official documentation](https://docs.snowflake.com/en/sql-reference/data-types-semistructured#object) states:
```
In Snowflake's semi-structured OBJECT data, each key is of type VARCHAR, and each value is of type VARIANT.
```
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "JinkunLiu (via GitHub)" <gi...@apache.org>.
JinkunLiu commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r2607809698
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlItemOperator.java:
##########
@@ -136,7 +137,8 @@ private static SqlSingleOperandTypeChecker getChecker(SqlCallBinding callBinding
if (name.equals("ITEM")) {
return "<ARRAY>[<INTEGER>]\n"
+ "<MAP>[<ANY>]\n"
- + "<ROW>[<CHARACTER>|<INTEGER>]";
+ + "<ROW>[<CHARACTER>|<INTEGER>]\n"
+ + "<VARIANT>[<ANY>]";
Review Comment:
You're right — in Snowflake, a VARIANT value can be an OBJECT (map) or an ARRAY.
However, a Snowflake OBJECT is a special kind of map: its keys must be strings, and its values are VARIANT types (i.e., `map<string, VARIANT>`).
As [Snowflake's official documentation](https://docs.snowflake.com/en/sql-reference/data-types-semistructured#object) said:
```
In Snowflake's semi-structured OBJECT data, each key is of type VARCHAR, and each value is of type VARIANT.
```
--
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
Re: [PR] [CALCITE-4918] Add a VARIANT data type [calcite]
Posted by "JinkunLiu (via GitHub)" <gi...@apache.org>.
JinkunLiu commented on code in PR #3947:
URL: https://github.com/apache/calcite/pull/3947#discussion_r2607809698
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlItemOperator.java:
##########
@@ -136,7 +137,8 @@ private static SqlSingleOperandTypeChecker getChecker(SqlCallBinding callBinding
if (name.equals("ITEM")) {
return "<ARRAY>[<INTEGER>]\n"
+ "<MAP>[<ANY>]\n"
- + "<ROW>[<CHARACTER>|<INTEGER>]";
+ + "<ROW>[<CHARACTER>|<INTEGER>]\n"
+ + "<VARIANT>[<ANY>]";
Review Comment:
You're right — in Snowflake, a VARIANT value can be an OBJECT (map) or an ARRAY.
However, a Snowflake OBJECT is a special kind of map: its keys must be strings, and its values are VARIANT types (i.e., `MAP<VARCHAR, VARIANT>`).
As [Snowflake's official documentation](https://docs.snowflake.com/en/sql-reference/data-types-semistructured#object) states:
```
In Snowflake's semi-structured OBJECT data, each key is of type VARCHAR, and each value is of type VARIANT.
```
--
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