feat: native map/array constructors #4232

agavra · 2020-01-07T00:19:13Z

BREAKING CHANGE: the old syntax for creating arrays using the
builtin AS_ARRAY function will no longer work

Description

Follow-up on #4120 - this PR creates maps and arrays natively instead of going through the UDF path. This has the benefit of custom syntax and stricter type checking.

Testing done

QTT
Unit Tests

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

big-andy-coates

Nice one @agavra

There are some comments inline, plus:

Syntax wise, all our other constructors use standard brackets. I think array should too. I know Postgres supports both ARRAY[] and ARRAY(). But IMHO I think it's cleaner to leave [] for array access, not construction.
This PR would benefit from RQTT test cases for INSERT VALUES. I'm confident this will flag up missing functionality in the coercer code.
At the moment it looks like its not possible to have an empty arrays or maps, or arrays and maps with only null elements / values. I think this is fine for this initial PR, but this is a hole we'll want to fix quickly. We should raise, (and link), suitable issues to track this and, ideally, get this done soon. One possible solution is to support casting elements, e.g. Postgres supports ARRAY(null)::int[], ARRAY()::int[], which casts all the elements in the array to INT and the array type to ARRAY<INT>.
You've dropped as_array. Is there the option to keep it until just before we go version 1.0? We could mark it as deprecated in the UDFs description and remove it from the docs. We could even log a warning when a query starts that uses it. Thereby giving people time to move over.

ksql-execution/src/main/java/io/confluent/ksql/execution/util/ExpressionTypeManager.java

big-andy-coates · 2020-01-07T10:40:14Z

ksql-execution/src/main/java/io/confluent/ksql/execution/util/ExpressionTypeManager.java

+          .collect(Collectors.toList());
+
+      if (sqlTypes.size() == 0) {
+        throw new KsqlException("Cannot extract type from array of nulls!");


Again with the exclamation marks ;)

If this is customer facing, which as a KsqlException I'm assuming it is, then maybe something like:

Can not construct array with all NULL elements.

You could even link to the github issue that will add support for empty/all-null arrays so that people can upvote it.

Can the user work around this by casting one of the NULLs to a type? Maybe include this in the error message and add a QTT test case.

What about for INSERT VALUES where the required array type is known.... can we support all nulls then? How about empty arrays?

Can the user work around this by casting one of the NULLs to a type? Maybe include this in the error message and add a QTT test case.

Yes and done.

What about for INSERT VALUES where the required array type is known.... can we support all nulls then? How about empty arrays?

This is not a trivial change. I've thought about this in the context of various things (i.e. struct type inference) and it's out of scope for this PR, though technically possible.

Can we link in a github issue to track this second part please?

ksql-execution/src/main/java/io/confluent/ksql/execution/util/ExpressionTypeManager.java

big-andy-coates · 2020-01-07T10:44:06Z

ksql-execution/src/main/java/io/confluent/ksql/execution/util/ExpressionTypeManager.java

+
+      if (sqlTypes.size() != 1) {
+        throw new KsqlException(
+            "Invalid map expression! All values must be of the same type. " + exp);


As with ARRAYs, it might help the user if the error included the types of the values, so that they could more easily see where the issue is., e.g. a UDF that returns a type different to what they thought.

Maybe add details of how the user could use CAST to fix the issue?

Added a better error message, but I don't think we need to tell the user to CAST as that may not be the right solution for them.

What's the issue with suggesting CAST as a possible solution? We're not forcing them to use it...

Fair enough

big-andy-coates · 2020-01-07T10:44:23Z

ksql-execution/src/main/java/io/confluent/ksql/execution/util/ExpressionTypeManager.java

+      }
+
+      if (sqlTypes.get(0) == null) {
+        throw new KsqlException("Maps do not accept null values!");


Maybe something like:

Can not construct MAP with all NULL values.

You could even link to the github issue that will add support for empty/all-null arrays so that people can upvote it.

Can the user work around this by casting one of the NULLs to a type? Maybe include this in the error message and add a QTT test case.

What about for INSERT VALUES where the required map type is known.... can we support all nulls then? How about empty maps?

Second part you've covered above. Lets just make sure its captured in an issue, maybe linking to it from the error.

Still think the error message could be improve to:

| Can not construct MAP with all NULL values.

And add the trick of using CAST.

ksql-functional-tests/src/test/resources/query-validation-tests/asarray.json

ksql-functional-tests/src/test/resources/query-validation-tests/asmap.json

agavra · 2020-01-07T17:23:53Z

Thanks for the review @big-andy-coates! I agree with your comments about testing and casting and I'll add those in.

I don't think we should keep AS_ARRAY because anyone that is upgrading to 0.7 will already have lots of breaking changes to make and this one isn't hard to change.

For the array constructor syntax, I think I'd prefer to keep it as is (which is the sql99 standard: http://web.cecs.pdx.edu/~len/sql1999.pdf):

<array value list constructor> ::= ARRAY <left bracket or trigraph> <array element list> <right bracket or trigraph>

A trigraph is ??(, which very few sql implementations seem to support. All examples I've found online of sql systems use the ARRAY[] syntax.

blueedgenick · 2020-01-07T18:28:18Z

I'm fully supportive of the square-bracket syntax for array creation - it is, after all, a language construct and not a function (at least it is now! ) and this matches the systems i've used in the past. Introducing new syntax for CASTing, a la postgres, seems iffy though without further discussion.

…

On Tue, Jan 7, 2020 at 9:24 AM Almog Gavra ***@***.***> wrote: Thanks for the review @big-andy-coates <https://github.com/big-andy-coates>! I agree with your comments about testing and casting and I'll add those in. I don't think we should keep AS_ARRAY because anyone that is upgrading to 0.7 will already have lots of breaking changes to make and this one isn't hard to change. For the array constructor syntax, I think I'd prefer to keep it as is (which is the sql99 standard: http://web.cecs.pdx.edu/~len/sql1999.pdf): <array value list constructor> ::= ARRAY <left bracket or trigraph> <array element list> <right bracket or trigraph> A trigraph is ??(, which very few sql implementations seem to support. All examples I've found online of sql systems use the ARRAY[] syntax. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#4232?email_source=notifications&email_token=ABCXJIBWZWBX6NAFNHDPRSTQ4S3DJA5CNFSM4KDNZDI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIJT4LA#issuecomment-571686444>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABCXJIC6JAHK56CXQVTJOIDQ4S3DJANCNFSM4KDNZDIQ> .

ksql-execution/src/main/java/io/confluent/ksql/execution/util/ExpressionTypeManager.java

big-andy-coates · 2020-01-16T12:30:30Z

ksql-execution/src/main/java/io/confluent/ksql/execution/util/ExpressionTypeManager.java

+          .collect(Collectors.toList());
+
+      if (sqlTypes.size() == 0) {
+        throw new KsqlException("Cannot extract type from array of nulls!");


Can we link in a github issue to track this second part please?

big-andy-coates · 2020-01-16T12:35:53Z

ksql-execution/src/main/java/io/confluent/ksql/execution/util/ExpressionTypeManager.java

+        throw new KsqlException("Only STRING keys are supported in maps but got: " + gkeyTypes);
+      }
+
+      final List<SqlType> sqlTypes = exp.getMap()


super nit: can we call this valueTypes and the other keyTypes to make it clear?

big-andy-coates · 2020-01-16T12:37:07Z

ksql-execution/src/main/java/io/confluent/ksql/execution/util/ExpressionTypeManager.java

+      }
+
+      if (sqlTypes.get(0) == null) {
+        throw new KsqlException("Maps do not accept null values!");


Second part you've covered above. Lets just make sure its captured in an issue, maybe linking to it from the error.

Still think the error message could be improve to:

| Can not construct MAP with all NULL values.

And add the trick of using CAST.

big-andy-coates

Thanks @agavra for addressing my suggestions. Agree with everything else. Couple of points / nits below.

Andy

big-andy-coates · 2020-01-16T12:38:45Z

ksql-functional-tests/src/test/resources/query-validation-tests/asarray.json

@@ -7,7 +7,7 @@
      "name": "construct a list from two elements",
      "statements": [
        "CREATE STREAM TEST (a INT, b INT) WITH (kafka_topic='test_topic', value_format='JSON');",
-        "CREATE STREAM OUTPUT AS SELECT as_array(a, b, 3) as l FROM TEST;"
+        "CREATE STREAM OUTPUT AS SELECT ARRAY[a, b, 3] as l FROM TEST;"


might want to rename this test file... it's currently asarray.json and AS_ARRAY no longer exists ;) . How about arrays.json :p

big-andy-coates · 2020-01-16T12:39:53Z

ksql-functional-tests/src/test/resources/query-validation-tests/asmap.json

@@ -3,6 +3,19 @@
    "Tests covering map creation"
  ],
  "tests": [
+    {
+      "name": "create map from named tuples",


likewise, consider renaming this test file.

big-andy-coates · 2020-01-16T12:45:45Z

ksql-functional-tests/src/test/resources/rest-query-validation-tests/insert-values.json

+    {
+      "name": "should handle arbitrary nested expressions",
+      "statements": [
+        "CREATE STREAM TEST (I INT, A ARRAY<ARRAY<INT>>) WITH (kafka_topic='test_topic', value_format='JSON');",
+        "INSERT INTO TEST (I, A) VALUES (-1, ARRAY[ARRAY[1]]);"
+      ],
+      "inputs": [
+      ],
+      "outputs": [
+        {"topic": "test_topic", "key": null, "value": {"I": -1, "A": [[1]]}}
+      ]
+    },


I don't think we're fully testing the coercer with this test as the 1 from ARRAY[ARRAY[1]] will default to a IntegerLiteral and the type is ARRAY<ARRAY<INT>>.

What happens if you make it ARRAY<ARRAY<BIGINT>>?

Same goes for the map test below.

BREAKING CHANGE: the old syntax for creating arrays using the builtin AS_ARRAY function will no longer work

agavra requested a review from a team as a code owner January 7, 2020 00:19

big-andy-coates reviewed Jan 7, 2020

View reviewed changes

agavra mentioned this pull request Jan 7, 2020

Explicit Typing for Array/Map Creation #4239

Open

agavra requested review from big-andy-coates and a team January 7, 2020 21:14

agavra force-pushed the maps_arrays branch 2 times, most recently from a22eff5 to 841a14d Compare January 13, 2020 18:59

big-andy-coates reviewed Jan 16, 2020

View reviewed changes

big-andy-coates approved these changes Jan 16, 2020

View reviewed changes

agavra mentioned this pull request Jan 16, 2020

Type Inference in INSERT VALUES #4334

Open

agavra added 3 commits January 17, 2020 09:36

feat: native map/array constructors

74d5d94

BREAKING CHANGE: the old syntax for creating arrays using the builtin AS_ARRAY function will no longer work

feat: improve error messages

911761a

fix: andys comments

79dff2f

agavra force-pushed the maps_arrays branch from e1896e0 to 79dff2f Compare January 17, 2020 18:09

agavra merged commit 3ecfaad into confluentinc:master Jan 17, 2020

agavra deleted the maps_arrays branch January 17, 2020 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: native map/array constructors #4232

feat: native map/array constructors #4232

agavra commented Jan 7, 2020

big-andy-coates left a comment

big-andy-coates Jan 7, 2020 •

edited

Loading

agavra Jan 7, 2020

big-andy-coates Jan 16, 2020

agavra Jan 16, 2020

big-andy-coates Jan 7, 2020 •

edited

Loading

agavra Jan 7, 2020

big-andy-coates Jan 16, 2020

agavra Jan 16, 2020

big-andy-coates Jan 7, 2020 •

edited

Loading

agavra Jan 7, 2020

big-andy-coates Jan 16, 2020 •

edited

Loading

agavra commented Jan 7, 2020

blueedgenick commented Jan 7, 2020 via email

big-andy-coates Jan 16, 2020

big-andy-coates Jan 16, 2020

big-andy-coates Jan 16, 2020 •

edited

Loading

big-andy-coates left a comment

big-andy-coates Jan 16, 2020

big-andy-coates Jan 16, 2020

big-andy-coates Jan 16, 2020

agavra Jan 16, 2020

feat: native map/array constructors #4232

feat: native map/array constructors #4232

Conversation

agavra commented Jan 7, 2020

Description

Testing done

Reviewer checklist

big-andy-coates left a comment

Choose a reason for hiding this comment

big-andy-coates Jan 7, 2020 • edited Loading

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

big-andy-coates Jan 7, 2020 • edited Loading

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

big-andy-coates Jan 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

big-andy-coates Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

agavra commented Jan 7, 2020

blueedgenick commented Jan 7, 2020 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

big-andy-coates Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

big-andy-coates 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

big-andy-coates Jan 7, 2020 •

edited

Loading

big-andy-coates Jan 7, 2020 •

edited

Loading

big-andy-coates Jan 7, 2020 •

edited

Loading

big-andy-coates Jan 16, 2020 •

edited

Loading

big-andy-coates Jan 16, 2020 •

edited

Loading