fix: improve handling of NULLs #5019

big-andy-coates · 2020-04-07T15:22:21Z

Description

This PR replaces #4869. It has many of the same changes, but not SqlNull type, and more fixes on top.

fixes: #4912 and may other bugs around use of NULL, you can now:

insert NULL values using INSERT INTO, for example INSERT INTO foo VALUES (NULL, NULL);, including use of ARRAY, MAP & STRUCT constructors with NULLs.
create MAPs with null keys and values, for example MAP('k0' := NULL, CAST(NULL AS STRING) := 10). (At least one key needs to be non-null or CAST so that ksqlDB can determine the schema of the map).
CAST NULL to any type, including the complex types ARRAY, MAP and STRUCT.

Also, better error messages, on:

Pull query on NULL key. Previously, blew up for non-string keys.
Pull query with non-literal expression for the key. Previously blew up.

Testing done

usual.

Reviewing notes:

Commits are broken down as follows:

Production changes. I've added notes inline where I thought it might help.
Remove some unused functions in production code.
Test code updates (including (r)QTT).
historical plans

I advise reviewing each commit separately. It may help to view the QTT & RQTT test changes in the third commit to understand the problems being solved.

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 #")

fixes: confluentinc#4912 and may other bugs around use of NULL. fixes a few issues with NULLs in ksqlDB. You can now: - insert NULL values using `INSERT INTO`, for example `INSERT INTO foo VALUES (NULL, NULL);`, including use of `ARRAY`, `MAP` & `STRUCT` constructors with `NULL`s. - create MAPs with null keys and values, for example `MAP('k0' := NULL, CAST(NULL AS STRING) := 10)`. (At least one key needs to be non-null or CAST so that ksqlDB can determine the schema of the map). - CAST NULL to any type, including the complex types `ARRAY`, `MAP` and `STRUCT`. Also, better error messages, on: - Pull query on NULL key. Previously, blew up for non-string keys. - Pull query with non-literal expression for the key. Previously blew up.

big-andy-coates · 2020-04-07T15:28:26Z

ksqldb-engine/src/main/java/io/confluent/ksql/engine/InsertValuesExecutor.java


-          });
+    @Override
+    public Object visitNullLiteral(final NullLiteral node, final Void context) {


This avoids any compiling of code etc to handle a NULL literal.

big-andy-coates · 2020-04-07T15:29:08Z

ksqldb-execution/src/main/java/io/confluent/ksql/execution/codegen/CodeGenRunner.java

+      if (expressionType != null) {
+        // expressionType can be null if expression is NULL.
+        ee.setExpressionType(SQL_TO_JAVA_TYPE_CONVERTER.toJavaType(expressionType));
+      }


This avoids an NPE. If not called the return value is left at the default Object, which is fine for nulls.

big-andy-coates · 2020-04-07T15:29:36Z

ksqldb-execution/src/main/java/io/confluent/ksql/execution/codegen/SqlToJavaVisitor.java

+      final StringBuilder map = new StringBuilder("new MapBuilder(");
+      map.append(exp.getMap().size());
+      map.append((')'));


Switch to a new builder type that won't throw on null keys or values.

big-andy-coates · 2020-04-07T15:31:46Z

ksqldb-execution/src/main/java/io/confluent/ksql/execution/codegen/SqlToJavaVisitor.java

+      final SqlType sourceType = expr.getRight();
+      if (sourceType == null || sqlType.equals(sourceType)) {
+        // sourceType is null if source is SQL NULL
        return new Pair<>(expr.getLeft(), sqlType);
      }

-      return CASTERS.getOrDefault(
-          sqlType.baseType(),
-          (e, t, r) -> {
-            throw new KsqlException("Invalid cast operation: " + t);
-          }
-      )
-          .cast(expr, sqlType, sqlType);
+      return CASTERS.getOrDefault(sqlType.baseType(), CastVisitor::unsupportedCast)
+          .cast(expr, sqlType);
+    }


getCast no longer calls sqlType.supportsCast as it is this the getCast method that determines which casts are supported and which are not. Hence supportsCast is superfluous and likely to get out of date with what this method supports.

big-andy-coates · 2020-04-07T15:32:39Z

ksqldb-execution/src/main/java/io/confluent/ksql/execution/codegen/SqlToJavaVisitor.java

+        final Pair<String, SqlType> expr, final SqlType returnType
    ) {
-      if (!(sqltype instanceof SqlDecimal)) {
-        throw new KsqlException("Expected decimal type: " + sqltype);
+      if (!(returnType instanceof SqlDecimal)) {
+        throw new KsqlException("Expected decimal type: " + returnType);


note: these methods were always called with the same param for returnType and sqlType: removed the duplication.

big-andy-coates · 2020-04-07T15:33:05Z

ksqldb-execution/src/main/java/io/confluent/ksql/execution/codegen/helpers/MapBuilder.java

+ * Used to construct maps using the builder pattern. Note that we cannot use {@link
+ * com.google.common.collect.ImmutableMap} because it does not accept null values.
+ */
+public class MapBuilder {


Builder of maps with nulls

big-andy-coates · 2020-04-07T15:34:54Z

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

-      }
-
-      expressionTypeContext.setSqlType(sqlType);
+      expressionTypeContext.setSqlType(node.getType().getSqlType());


Removed this as its not needed. It's getCast in SqlToJavaVisitor that determines what casts are supported. Removing this check just means it fails, with the same error, slightly later in flow.

big-andy-coates · 2020-04-07T15:36:52Z

ksqldb-parser/src/main/java/io/confluent/ksql/schema/ksql/DefaultSqlValueCoercer.java

            }
          }).build();

  @Override
-  public Optional<?> coerce(final Object value, final SqlType targetType) {
+  public Result coerce(final Object value, final SqlType targetType) {


refactored to allow coerce to return a successful coercion with a no value result, i.e. coercion of nulls. Previous return value would not differentiate between a failure and 'no value'

big-andy-coates · 2020-04-07T15:57:58Z

ksqldb-parser/src/main/java/io/confluent/ksql/schema/ksql/DefaultSqlValueCoercer.java

+    if (value == null) {
+      // NULL can be cast to any type:
+      return Result.nullResult();
+    }


look mar, null handling!

big-andy-coates · 2020-04-07T16:00:26Z

ksqldb-parser/src/main/java/io/confluent/ksql/schema/ksql/SqlValueCoercer.java

   */
-  Optional<?> coerce(Object value, SqlType targetSchema);
+  Result coerce(Object value, SqlType targetSchema);


As above, this change in return value is to allow callers to differentiate between:

the coercion failed. Previously this returned Optional.empty and now returns Result.failed.

the coercion succeeded and the result was null. Previously, this threw an NPE and now returns Result.nullResult.

Normal results where previously Optional.of(whateva) and are now Result.of(whateva).

big-andy-coates · 2020-04-07T16:06:36Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/execution/PullQueryExecutor.java

+    if (!(other instanceof Literal)) {
+      throw new KsqlException("Ony comparison to literals is currently supported: " + comparison);
+    }

+    if (other instanceof NullLiteral) {
+      throw new KsqlException("Primary key columns can not be NULL: " + comparison);
+    }


Catches errors on pull queries.

first one catches comparison of ROWKEY to non-literal, e.g. a udf call, or some other expression. Previously this would blow up with class cast exception.

second one catches comparison with NULL. For STRING keys this previously returned some weird streams error and for non-STRING keys some confusing error about not being able to coerce a STRING to the required key type. (NullLiteral returns "null" from getValue for some weird reason).

agavra

Seems surprisingly straightforward 😄 always my favorite types of PRs

big-andy-coates requested a review from a team as a code owner April 7, 2020 15:22

big-andy-coates added 3 commits April 7, 2020 16:22

chore: remove unused code

841c804

test: tests

5d22ed7

test: historical plans

c0d8792

big-andy-coates mentioned this pull request Apr 7, 2020

fix: improved NULL handling #4869

Closed

2 tasks

big-andy-coates commented Apr 7, 2020

View reviewed changes

big-andy-coates added 2 commits April 7, 2020 17:26

Merge branch 'master' into nulls

2028a47

chore: fix spot bugs

746e65d

agavra approved these changes Apr 9, 2020

View reviewed changes

Merge branch 'master' into nulls

f6a4144

big-andy-coates merged commit c53dd68 into confluentinc:master Apr 14, 2020

big-andy-coates deleted the nulls branch April 14, 2020 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: improve handling of NULLs #5019

fix: improve handling of NULLs #5019

big-andy-coates commented Apr 7, 2020 •

edited

big-andy-coates Apr 7, 2020

big-andy-coates Apr 7, 2020

big-andy-coates Apr 7, 2020

big-andy-coates Apr 7, 2020

big-andy-coates Apr 7, 2020

big-andy-coates Apr 7, 2020

big-andy-coates Apr 7, 2020

big-andy-coates Apr 7, 2020

big-andy-coates Apr 7, 2020

big-andy-coates Apr 7, 2020

big-andy-coates Apr 7, 2020

agavra left a comment

fix: improve handling of NULLs #5019

fix: improve handling of NULLs #5019

Conversation

big-andy-coates commented Apr 7, 2020 • edited

Description

Testing done

Reviewing notes:

Reviewer checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agavra left a comment

Choose a reason for hiding this comment

big-andy-coates commented Apr 7, 2020 •

edited