[CALCITE-4918] Add a VARIANT data type #3947

mihaibudiu · 2024-09-04T06:39:43Z

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.

julianhyde · 2024-09-04T16:23:22Z

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

julianhyde

Looks good.

One useful addition would be a variant.iq quidem test. With extensive comments, it could read like a tutorial, showing all the things you can do with variants. As a new language feature, specific to Calcite (albeit based on Snowflake) I think it needs better documentation than we usually provide.

julianhyde · 2024-09-05T00:38:22Z

core/src/main/java/org/apache/calcite/util/rtti/RuntimeTypeInformation.java

+import static java.util.Objects.requireNonNull;
+
+/**
+ * This class represents the type of a SQL expression at runtime.


Remove 'This class represents'

julianhyde · 2024-09-05T00:40:13Z

core/src/main/java/org/apache/calcite/util/rtti/RuntimeTypeInformation.java

+ * 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 {


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.

julianhyde · 2024-09-05T00:40:29Z

core/src/main/java/org/apache/calcite/util/rtti/RuntimeTypeInformation.java

+  }
+
+  /**
+   * Create and return an expression that creates a runtime type that


s/Create/Creates/

... and generally, please follow the best practice of using third-person indicative for method javadoc.

julianhyde · 2024-09-05T00:44:33Z

core/src/main/java/org/apache/calcite/util/Variant.java

+/** This class is the runtime support for values of the VARIANT SQL type. */
+public class Variant {
+  /** Actual value. */
+  final @Nullable Object value;


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)?

julianhyde · 2024-09-05T00:51:14Z

core/src/main/java/org/apache/calcite/util/Variant.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;


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.)

mihaibudiu · 2024-09-06T03:20:17Z

@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.

suibianwanwank · 2024-09-06T10:18:13Z

Implicitly cast is an important part of VARIANT type, will it be supported in this PR?

edit: I just noticed you mention this in Jira.

mihaibudiu · 2024-09-06T16:44:45Z

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.

julianhyde · 2024-09-06T19:09:43Z

site/_docs/reference.md

-| Type     | Description                | Example type
-|:-------- |:---------------------------|:---------------
-| ANY      | The union of all types |
+| Type     | Description                | Example type |


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.

julianhyde · 2024-09-06T19:10:53Z

@mihaibudiu Thanks for all your edits. Much improved.

+1 after you fix the cosmetic issues in reference.md.

mihaibudiu · 2024-09-06T19:59:14Z

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".

caicancai

@mihaibudiu I reviewed the comments and left some minor comment. I think this PR can be merged.

caicancai · 2024-09-22T02:22:38Z

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>. */


There is a redundant space

caicancai · 2024-09-22T02:24:43Z

core/src/main/java/org/apache/calcite/runtime/variant/VariantNull.java

+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+/** The VARIANT type has its own notion of null, which is


Maybe adding a line break would be better.

Signed-off-by: Mihai Budiu <[email protected]>

sonarcloud · 2024-12-04T00:36:32Z

Quality Gate passed

Issues
27 New issues
0 Accepted issues

Measures
0 Security Hotspots
51.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

NobiGo

It's really a complicated PR. We may be able to fill in some optimizations about VARIANT in some future PRs.
Some example:
CAST(e as VARIANT ) is not null to e is not null.
CAST(cast(e as variant) as int) to cast(e as int)

NobiGo · 2024-12-04T07:50:57Z

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);


TYPEOF is a new function in CALCITE. Can we use a split Jira issue to support it?

VARIANT without TYPEOF is of not much use.

NobiGo · 2024-12-04T07:57:05Z

core/src/main/java/org/apache/calcite/runtime/rtti/BasicSqlTypeRtti.java

+  }
+
+  @Override public String getTypeString()  {
+    switch (this.typeName) {


Can we merge some types? Most of the names of the types here have not changed.

NobiGo · 2024-12-04T08:04:50Z

core/src/main/java/org/apache/calcite/runtime/rtti/RuntimeTypeInformation.java

+/**
+ * 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


There is an extra space before However.

Yes, that's how I learned to type, emacs encourages this forfmatting for end of sentence.

NobiGo · 2024-12-04T08:39:49Z

core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java

+        .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)")


If we convert cast(ARRAY[1,2,3, null] AS VARIANT, What's the columnType?

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.

NobiGo · 2024-12-04T08:57:25Z

site/_docs/reference.md

@@ -1239,14 +1240,66 @@ Note:
 | MAP      | Collection of keys mapped to values | Example: map &lt; int, varchar &gt;
 | 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 |


Do we need to remove this |? ANY and UNKNOWN also end with |;

julianhyde · 2024-12-04T19:55:29Z

Looks good. Thanks for adding variant.iq - that's the first place I would direct someone who wants to understand this feature.

mihaibudiu · 2024-12-04T19:59:14Z

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!

julianhyde · 2024-12-04T20:07:42Z

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.

mihaibudiu mentioned this pull request Sep 4, 2024

[SQL] JSON support feldera/feldera#2401

Open

7 tasks

mihaibudiu force-pushed the variant branch 2 times, most recently from 5759a66 to 44e3d35 Compare September 4, 2024 18:34

julianhyde reviewed Sep 5, 2024

View reviewed changes

mihaibudiu force-pushed the variant branch from 44e3d35 to 1162156 Compare September 6, 2024 03:19

mihaibudiu force-pushed the variant branch from 1162156 to 3ed4604 Compare September 6, 2024 06:43

julianhyde reviewed Sep 6, 2024

View reviewed changes

mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Sep 6, 2024

mihaibudiu force-pushed the variant branch 2 times, most recently from 78bff8d to a35f642 Compare September 13, 2024 20:20

caicancai approved these changes Sep 14, 2024

View reviewed changes

caicancai reviewed Sep 22, 2024

View reviewed changes

mihaibudiu force-pushed the variant branch from a35f642 to 9eb0fe7 Compare October 2, 2024 23:50

mihaibudiu force-pushed the variant branch 2 times, most recently from af0ff63 to 5827923 Compare October 23, 2024 22:40

mihaibudiu force-pushed the main branch from dc8c4ff to 855ad83 Compare October 23, 2024 22:46

mihaibudiu force-pushed the variant branch 2 times, most recently from 2ee9768 to 9b88b5b Compare October 30, 2024 18:35

mihaibudiu force-pushed the variant branch from 9b88b5b to 5fc9186 Compare November 15, 2024 21:08

mihaibudiu added 5 commits December 3, 2024 16:17

[CALCITE-4918] Add a VARIANT data type - parser and validator

90b6547

Signed-off-by: Mihai Budiu <[email protected]>

[CALCITE-4918] Add a VARIANT data type - runtime support

808fa86

Signed-off-by: Mihai Budiu <[email protected]>

CheckerFramework and ErrorProne fixes

9cd31eb

Signed-off-by: Mihai Budiu <[email protected]>

Implement VARIANT functions TYPEOF, VARIANTNULL; add variant.iq

de23a0d

Signed-off-by: Mihai Budiu <[email protected]>

Add missing @OverRide to make ErrorProne happy

02a463f

Signed-off-by: Mihai Budiu <[email protected]>

mihaibudiu added 3 commits December 3, 2024 16:17

Use Calcite conventions for markdown

bbda35c

Signed-off-by: Mihai Budiu <[email protected]>

Allow a VARIANT storing a numeric value be cast to another numeric value

9680d23

Signed-off-by: Mihai Budiu <[email protected]>

Do not clone needlessly RelNode

6317e1c

Signed-off-by: Mihai Budiu <[email protected]>

mihaibudiu force-pushed the variant branch from 5fc9186 to 6317e1c Compare December 4, 2024 00:24

NobiGo approved these changes Dec 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-4918] Add a VARIANT data type #3947

[CALCITE-4918] Add a VARIANT data type #3947

mihaibudiu commented Sep 4, 2024

julianhyde commented Sep 4, 2024

julianhyde left a comment

julianhyde Sep 5, 2024

julianhyde Sep 5, 2024

julianhyde Sep 5, 2024

julianhyde Sep 5, 2024

julianhyde Sep 5, 2024 •

edited

Loading

julianhyde Sep 5, 2024

mihaibudiu commented Sep 6, 2024

suibianwanwank commented Sep 6, 2024 •

edited

Loading

mihaibudiu commented Sep 6, 2024

julianhyde Sep 6, 2024

julianhyde commented Sep 6, 2024

mihaibudiu commented Sep 6, 2024

caicancai left a comment •

edited

Loading

caicancai Sep 22, 2024 •

edited

Loading

caicancai Sep 22, 2024

sonarcloud bot commented Dec 4, 2024

NobiGo left a comment

NobiGo Dec 4, 2024

mihaibudiu Dec 4, 2024

NobiGo Dec 4, 2024

NobiGo Dec 4, 2024

mihaibudiu Dec 4, 2024

NobiGo Dec 4, 2024

mihaibudiu Dec 4, 2024

NobiGo Dec 4, 2024

julianhyde commented Dec 4, 2024

mihaibudiu commented Dec 4, 2024

julianhyde commented Dec 4, 2024


		import org.checkerframework.checker.nullness.qual.Nullable;

		/** The VARIANT type has its own notion of null, which is

[CALCITE-4918] Add a VARIANT data type #3947

Are you sure you want to change the base?

[CALCITE-4918] Add a VARIANT data type #3947

Conversation

mihaibudiu commented Sep 4, 2024

julianhyde commented Sep 4, 2024

julianhyde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianhyde Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaibudiu commented Sep 6, 2024

suibianwanwank commented Sep 6, 2024 • edited Loading

mihaibudiu commented Sep 6, 2024

Choose a reason for hiding this comment

julianhyde commented Sep 6, 2024

mihaibudiu commented Sep 6, 2024

caicancai left a comment • edited Loading

Choose a reason for hiding this comment

caicancai Sep 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Dec 4, 2024

Quality Gate passed

NobiGo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianhyde commented Dec 4, 2024

mihaibudiu commented Dec 4, 2024

julianhyde commented Dec 4, 2024

julianhyde Sep 5, 2024 •

edited

Loading

suibianwanwank commented Sep 6, 2024 •

edited

Loading

caicancai left a comment •

edited

Loading

caicancai Sep 22, 2024 •

edited

Loading