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

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>

sonarcloud · 2024-09-13T20:35:25Z

Quality Gate passed

Issues
25 New issues
0 Accepted issues

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

See analysis details on SonarCloud

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.

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 from a7642e4 to 78bff8d Compare September 11, 2024 01:46

mihaibudiu added 7 commits September 13, 2024 13:16

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

863e112

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>

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

037b1d4

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>

CheckerFramework and ErrorProne fixes

a3915a0

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>

Implement VARIANT functions TYPEOF, VARIANTNULL; add variant.iq

63a3393

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>

Add missing @OverRide to make ErrorProne happy

3096d2f

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>

Use Calcite conventions for markdown

746966a

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>

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

a35f642

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>

mihaibudiu force-pushed the variant branch 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

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

sonarcloud bot commented Sep 13, 2024

caicancai left a comment •

edited

Loading

caicancai Sep 22, 2024 •

edited

Loading

caicancai Sep 22, 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

julianhyde Sep 5, 2024

Choose a reason for hiding this comment

julianhyde Sep 5, 2024

Choose a reason for hiding this comment

julianhyde Sep 5, 2024

Choose a reason for hiding this comment

julianhyde Sep 5, 2024

Choose a reason for hiding this comment

julianhyde Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

julianhyde Sep 5, 2024

Choose a reason for hiding this comment

mihaibudiu commented Sep 6, 2024

suibianwanwank commented Sep 6, 2024 • edited Loading

mihaibudiu commented Sep 6, 2024

julianhyde Sep 6, 2024

Choose a reason for hiding this comment

julianhyde commented Sep 6, 2024

mihaibudiu commented Sep 6, 2024

sonarcloud bot commented Sep 13, 2024

Quality Gate passed

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

caicancai Sep 22, 2024

Choose a reason for hiding this comment

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