Part 2 of changes for SQL Compatible Null Handling by nishantmonu51 · Pull Request #5958 · apache/druid

nishantmonu51 · 2018-07-06T19:06:42Z

Part 2 of changes for SQL Compatible Null Handling -
This PR contains changes to improve handling of null values in druid by treating nulls as missing values which don’t actually exist. This will make it more compatible to SQL standard and help with integration with other existing BI systems which support ODBC/JDBC. (Detailed description in proposal - #4349)

NOTE - NULL and empty strings are treated differently in entire druid query processing system.

Nullability for Aggregators/Metrics

As per the current implementation, most aggregators are coded to replace null values with default e.g. sum treats them as zeroes, min treats them as positive infinity, etc.
To match the new semantics and make aggregators nullable, where if an aggregator encounters only the null values the result will be null value following changes are made -

Added a NullableAggregator/NullableBufferAggregator which is be used as a decorator for existing aggregator implementations to make them nullable. This will return null values if all the values aggregated are null, If any of the aggregated value is non-null, it will have same behavior as the delegate aggregator.
If new null behavior is enabled, aggregator factories will decorate the aggregators with NullableAggregator/NullableBufferAggregator.
Added a new abstract class NullableAggregatorFactory which helps in reducing duplicate code and decide to switch between nullable and non-nullable aggregators.
Cardinality and HyperUnique aggregators ignore null values and only count non-null values, If all the encountered values are null, the result will be 0. This is different from current behavior where null values are also counted.
Count aggregator - since count aggregator is not associated with any column and just a count of the number of rows, it has the same behavior as before.

Math Expressions Null Handling

For general calculations like sum, full expression is be considered as null if any of the components is null.
StringLiteral now supports null and and expressions containing non-quoted null is parsed as StringLiteral with null
Specifying a default value for null is supported by the use of NVL or IF clause to assign default values at query time.
Comparisons between null and non-null values will follow three-valued logic and the result of a comparison involving NULL is not a boolean value – it is a null value.

Filtering on Null values

SelectorDimFilter currently specifies filtering on null values but the implementation assumes null and empty strings as equivalent. The implementation is changed to consider null and empty string differently. Generation of cache key for selectorDimFilter is also modified to support null.
InFilter/ExpressionFilter/LikeFilter are also modified to not treat nulls as empty strings.

Changes to Druid build-in SQL layer

NULL and empty strings are treated differently.
IS NULL and IS NOT NULL filters against null values instead of empty strings.
NVL, IFNULL and COALESCE work as per SQL standard.
count now skips null values and works as per SQL standard.

Backwards Compatibility

"druid.generic.useDefaultValueForNull" is added as a new config. The default value is set to true for backwards compatibility.
code places in query/ingestion where we converted emptyToNull or nullToEmpty now use NullHandlingHelper which only does the conversion if backwards compatibility is enabled.
Storage layer changes are backwards compatible for GenericIndexed/String columns.
For Numeric columns new V2 serde implementations are introduced which also supports serde of nullRowsBitmaps. Previous implementations are kept unchanged to read any existing segments.

Testing

Most of the unit tests that tested nulls are modified to have two branches of expected results to test new/old behavior both.
For CI have added new runs to test matrix to test new behavior for nulls.
To run the tests with new behavior run "-Ddruid.generic.useDefaultValueForNull=false"

Pending work -
https://github.com/apache/incubator-druid/labels/Area%20-%20Null%20Handling

nishantmonu51 · 2018-07-06T19:08:01Z

@leventov : Created a new PR, please review.

leventov · 2018-07-07T11:59:27Z

@nishantmonu51 as far as I can see you didn't address all comments that I left in the last batch in the previous PR, please answer to them there (if you are able to open that PR)

nishantmonu51 · 2018-07-09T16:45:42Z

@leventov: last batch of comments was re lines more than 120 cols, add line breaks and reformatted code. Also replied to your comments on previous PR, If you have any more comments can you please add them here ?

jihoonson · 2018-07-09T21:29:43Z

Since this PR is tagged as Design Review, I'm going to review this PR too.

jihoonson · 2018-07-09T22:57:09Z

@nishantmonu51 is this the last PR for #4349? Would you let me know which are not done yet in #5278 and this PR?

jihoonson · 2018-07-10T00:43:28Z

Some comments and questions for the PR description.

Aggrgator/BufferAggregator - Added a new method boolean isNull() which returns false by default. aggregators that support nullability can choose to override this and return true if the aggregated result is null.

This should be moved to #5278.

Changes to Druid build-in SQL layer

NULL and empty strings are treated differently.

I haven't checked through the whole patch, so am wondering this is true only for the SQL layer or the entire Druid query processing system.

jihoonson

This is a great work. Thanks @nishantmonu51!

Reviewed up to LoadingLookup and still reviewing.

jihoonson · 2018-07-09T22:13:18Z

    return value;
  }

-  public boolean isNull()


nit: I think it's better to call this method rather than doing value() == null in call sites.

this method is replaced with isNumericNull as that is what we need to check for Expressions.
isNumericNull will check whether the primitive long/float/double equivalent is null or not.
It is possible for an expression to have a non-null String value but it can return null when parsed as a primitive long/float/double.

jihoonson · 2018-07-09T22:16:33Z

    if (leftVal.type() == ExprType.STRING && rightVal.type() == ExprType.STRING) {
      return evalString(leftVal.asString(), rightVal.asString());
    } else if (leftVal.type() == ExprType.LONG && rightVal.type() == ExprType.LONG) {
+      if (NullHandling.sqlCompatible() && (leftVal.isNumericNull() || rightVal.isNumericNull())) {


Wonder why we check this here instead of changing evalLong() or evalDouble to return a nullable Long or Double, respectively. Is this to avoid boxing/unboxing? If so, I wonder how much it affects to the performance.

Not sure about how much performance effect boxing/unboxing will have, in this form atleast the change is on safer side and does not affect performance for non-sql case.
We can run more benchmarks in a follow up PR and change this api to return boxed values if the performance diff is not much.

jihoonson · 2018-07-10T00:53:26Z

-      return theLong == null ? 0L : theLong;
+      Number number = asNumber();
+      if (number == null) {
+        assert NullHandling.replaceWithDefault();


How about checking and throwing an Exception rather than a assert statement? The assert statement wouldn't be super helpful for users if it happens. Same for all other assert statements in this class.

IIRC, I was actually doing that before, but changed this to assert based on discussions on previous PRs.

So, would you point me out where the discussion is? I guess it's in #5278?

unable to find the discussion among lots of review comments.
I believe the intent here was to catch any coding errors/places where we may have missed handling nulls properly.
@leventov: can probably add more context here.

#5278 (comment) #5278 (comment) #5278 (comment) #5278 (comment)

jihoonson · 2018-07-10T01:08:34Z

  public void aggregate()
  {
-    histogram.offer(selector.getFloat());
+    if (NullHandling.replaceWithDefault() || !selector.isNull()) {


What does NullHandling.replaceWithDefault() mean here? Does that mean nulls have replaced with some default value at ingestion time? If so, I guess select.isNull() always return false and thus this check can be simplified into if (!selector.isNull()).

this means whether nulls should be replaced with default value.
In case of ExpressionColumnValueSelector isNull will compute the expression and then give the result, the check is there to not have any performance impact of calling isNull for default case.

Got it. Thanks. Would you add a comment here?

jihoonson · 2018-07-10T01:24:09Z

+  public String apply(@Nullable final String key)
  {
-    if (key == null) {
+    String keyEquivalent = NullHandling.nullToEmptyIfNeeded(key);


I thought this kind of null value conversion is needed only when retrieving values from the storage layer (like in ColumnValueSelector), so that we expect key is already converted to some default value if needed. But, apparently it seems not. Would you explain why this is needed after retrieving values?

this is to preserve backwards compatibility,
previously, nulls and empty strings were considered same.
and if a looking has mapping "" -> "someval" then all nulls/empty strings will be resolved to "someval".

nishantmonu51 · 2018-07-10T14:49:57Z

Thanks @jihoonson for helping with the review and taking this PR forward.

leventov · 2018-07-14T18:36:28Z

 import java.util.Objects;

-public class FloatFirstAggregatorFactory extends AggregatorFactory
+public class FloatFirstAggregatorFactory extends NullableAggregatorFactory<ColumnValueSelector>


This comment: #5452 (comment) is not addressed.

not a blocker for this PR, would like to do it a follow up PR - #6039

leventov · 2018-07-14T18:37:59Z

      Chronology chronology = timeZone == null ? ISOChronology.getInstanceUTC() : ISOChronology.getInstance(timeZone);
      final Object value = originArg.eval(bindings).value();
-      origin = value != null ? new DateTime(value, chronology) : null;
+      if (value instanceof String && StringUtils.isEmpty((String) value)) {


This comment: #5452 (comment) is not addressed

#6040
Will fix this line in this PR and prohibit the api in future PR.

leventov · 2018-07-14T18:39:02Z

  public DimFilter optimize()
  {
-    return new InDimFilter(dimension, ImmutableList.of(value), extractionFn).optimize();
+    return new InDimFilter(dimension, Arrays.asList(value), extractionFn).optimize();


Collections.singletonList()

leventov · 2018-07-14T18:39:56Z

  // converted to strings. We should also add functions
  // for these and modify ColumnComparisonFilter to handle
  // comparing Long and Float columns to eachother.
+  // Returns null when the underlying Long/Float value is null.


Not addressed: #5452 (comment)

leventov · 2018-07-14T18:44:14Z

  private Ordering<Row> metricOrdering(final String column, final Comparator comparator)
  {
-    return Ordering.from(Comparator.comparing((Row row) -> row.getRaw(column), Comparator.nullsLast(comparator)));
+    if (NullHandling.sqlCompatible()) {


Need a comment explaining this.

added comment.

leventov · 2018-07-14T19:00:50Z

@@ -93,7 +93,7 @@ public String getAnnouncementPath(String listenerName)
  {
    return ZKPaths.makePath(
        getListenersPath(), Preconditions.checkNotNull(


Each argument should be on a separate line

leventov · 2018-07-14T19:02:37Z

  public static String escapeStringLiteral(final String s)
  {
+    Preconditions.checkNotNull(s);
    if (s == null) {


This check immediately after Preconditions.checkNotNull?

removed the == null check, it was not needed.

leventov · 2018-07-14T19:03:43Z

              final Object value = row[i];
-              final String stringValue = value != null ? String.valueOf(value) : "";
+              if (value == null) {
+                // NULLS are not supposed to match NULLs in a join. So ignore them.


leventov · 2018-07-14T19:03:57Z

          literal = rexBuilder.makeLiteral(exprResult.asBoolean(), constExp.getType(), true);
        } else if (sqlTypeName == SqlTypeName.DATE) {
-          if (!constExp.getType().isNullable() && exprResult.isNull()) {
+          if (!constExp.getType().isNullable() && exprResult.isNumericNull()) {


It is possible for an expression to have a non-null String value but it can return null when parsed as a primitive long/float/double.
ExprEval.isNumericNull checks whether the parsed primitive value is null or not.
Added comment to code.

leventov · 2018-07-14T19:04:10Z

-                        getCluster().getRexBuilder().makeLiteral(value)
-                    )
-                );
+                // NULLS are not supposed to match NULLs in a join. So ignore them.


nishantmonu51 · 2018-07-24T16:09:26Z

@leventov @jihoonson : Thanks for the review comments, addressed them.
Do you have any more comments ?
Also, can you please add some info on how much of the review is pending and when can we expect to merge this one in.
Its blocking feature development/patches for Apache Hive Integration for handling nulls coming from druid properly.

jihoonson

@nishantmonu51 I finished my first review. You did a really nice job. I would like to thank you.

Two more comments:

There are some checkstyle errors including wrong license. Please fix them.
Would you please check my previous comments?
- #5958 (comment)
- #5958 (comment)

jihoonson · 2018-07-24T21:23:33Z

+    if (NullHandling.sqlCompatible()) {
+      // empty string should also have same behavior
+      Assert.assertEquals(null, loadingLookup.apply(""));
+    }


Can be Assert.assertNull().

Also, do we need to test for non-sql-compatible mode? Like:

Assert.assertNull(loadingLookup.apply(null)); if (NullHandling.sqlCompatible()) { // empty string should also have same behavior Assert.assertNull(loadingLookup.apply("")); } else { Assert.assertEquals("", loadingLookup.apply("")); }

jihoonson · 2018-07-24T21:23:49Z

+    if (NullHandling.sqlCompatible()) {
+      Assert.assertEquals(Collections.emptyList(), loadingLookup.unapply(null));
+    } else {
+      Assert.assertEquals(null, loadingLookup.unapply(null));


Assert.assertNull()

jihoonson · 2018-07-24T21:24:23Z

-    Assert.assertEquals(ImmutableMap.of("value", Lists.newArrayList("key")), loadingLookup.unapplyAll(ImmutableSet.<String>of("value")));
+    Assert.assertEquals(
+        ImmutableMap.of("value", Lists.newArrayList("key")),
+        loadingLookup.unapplyAll(ImmutableSet.<String>of("value"))


Would you please remove the unnecessary type argument?

jihoonson · 2018-07-24T21:35:32Z

  }

-  public long sumMetric(final Task task, final DimFilter filter, final String metric)
+  public Long sumMetric(final Task task, final DimFilter filter, final String metric)


Please annotate @Nullable.

jihoonson · 2018-07-24T21:35:53Z

  }

-  public long sumMetric(final Task task, final DimFilter filter, final String metric)
+  public Long sumMetric(final Task task, final DimFilter filter, final String metric)


Please annotate @Nullable.

jihoonson · 2018-07-24T23:29:42Z

    byte[] extractionFnBytes = extractionFn == null ? new byte[0] : extractionFn.getCacheKey();

-    ByteBuffer filterCacheKey = ByteBuffer.allocate(3
+    ByteBuffer filterCacheKey = ByteBuffer.allocate(5


nit: can we use CacheKeyBuilder here?

jihoonson · 2018-07-24T23:35:15Z

    byte[] extractionFnBytes = extractionFn == null ? new byte[0] : extractionFn.getCacheKey();

-    return ByteBuffer.allocate(3 + dimensionBytes.length + valueBytes.length + extractionFnBytes.length)
+    return ByteBuffer.allocate(5 + dimensionBytes.length + valueBytes.length + extractionFnBytes.length)


nit: can we use CacheKeyBuilder here?

jihoonson · 2018-07-24T23:39:02Z

    }
+
+    // This class is only used when SQL compatible null handling is enabled.
+    private class NullableRowBasedKeySerdeHelper implements RowBasedKeySerdeHelper


Please add a comment about the memory layout.

jihoonson · 2018-07-24T23:48:05Z

    if (isSimpleEquals()) {
      // Verify that dimension equals prefix.
-      return ImmutableList.of(selector.getBitmapIndex(dimension, likeMatcher.getPrefix()));
+      return ImmutableList.of(selector.getBitmapIndex(


format

return ImmutableList.of( selector.getBitmapIndex( dimension, NullHandling.emptyToNullIfNeeded(likeMatcher.getPrefix()) ) );

jihoonson · 2018-07-24T23:51:37Z

      return DruidExpression.fromFunctionCall(
          "timestamp_ceil",
-          ImmutableList.of(
+          Arrays.asList(


Why this change?

Maybe Stream.of() is better.

Changed because ImmutableList doesn't support nulls, I suppose. Anyway, a parallel PR #5980 handles this in the whole codebase.

leventov

LGTM besides @jihoonson's comments, and if you also assign #6039 to yourself and do that after this PR.

nishantmonu51 · 2018-07-27T11:12:16Z

Aggrgator/BufferAggregator - Added a new method boolean isNull() which returns false by default. aggregators that support nullability can choose to override this and return true if the aggregated result is null.
This should be moved to #5278.

Removed it from this PR.

Changes to Druid build-in SQL layer
NULL and empty strings are treated differently.
I haven't checked through the whole patch, so am wondering this is true only for the SQL layer or the entire Druid query processing system.

it applies to whole query processing. updated PR description to add a note.

nishantmonu51 · 2018-07-27T11:19:42Z

@jihoonson : For the pending work, I have labelled the issues with Area : Null Handling
https://github.com/apache/incubator-druid/labels/Area%20-%20Null%20Handling

Have updated the PR based on your comments, please check if i missed any comments.

jihoonson · 2018-07-27T21:07:10Z

Thanks @nishantmonu51. Would you please check this comment: #5958 (comment)?

Also, I'm not sure what have done in #5278 + this PR and not. I think at least Comparisons between null and non-null values will follow three-valued logic and the result of a comparison involving NULL is not a boolean value – it is a null value. is not done yet? Would you please clarify what are remaining in #4349?

nishantmonu51 · 2018-07-31T13:38:51Z

Comparisons between null and non-null values will follow three-valued logic and the result of a comparison involving NULL is not a boolean value – it is a null value.

this actually works now for Expressions, See BinaryEvalOpExprBase.eval, if any of the values being compared is null it returns null.
Do you see any places where this is might not work ?

nishantmonu51 · 2018-07-31T14:25:20Z

@jihoonson In terms of functionality, After this PR, all things mentioned in the proposal except following should be working -

last of @gianm comments (Add config to replace nulls with default value at ingestion time instead of relying on system wide config #6053).
Ability to specify default values at ingestion

Users will be able to specify default values to replace NULL values at ingestion time.

Handling of follow up comments perf related
Updating/Adding docs for the new behavior

nishantmonu51 · 2018-08-01T03:59:31Z

@jihoonson any more comments here ?

jihoonson · 2018-08-01T05:24:35Z

@nishantmonu51 awesome! Thanks for clarifying. Everything looks good except the travis failure. Would you check it please?

asdf2014 · 2018-08-01T06:39:52Z

            .andReturn(Collections.singletonList("key"))
            .once();
    EasyMock.replay(reverseLookupCache);
+<<<<<<< HEAD


Please resolve the conflict..

nishantmonu51 · 2018-08-01T18:12:22Z

@jihoonson: travis is passing now.

jihoonson

LGTM

…ling-merge

…ailures

nishantmonu51 · 2018-08-02T14:01:03Z

@leventov @jihoonson @b-slim Thanks for the review.
This PR is ready for merge and has got required +1s.
can one of you please merge this ?

nishantmonu51 added Feature Area - Batch Ingestion Area - Querying Design Review Development Blocker Area - SQL Area - Null Handling labels Jul 6, 2018

nishantmonu51 mentioned this pull request Jul 6, 2018

SQL compatible Null Handling Part 2 - Processing Layer and Druid-SQL changes #5452

Closed

nishantmonu51 added 2 commits July 9, 2018 22:11

Part 2 of changes for SQL Compatible Null Handling

abc7cbb

Review comments - break lines longer than 120 characters

6a6d35a

nishantmonu51 force-pushed the null-handling-pr branch from 93f41e8 to 6a6d35a Compare July 9, 2018 16:42

nishantmonu51 mentioned this pull request Jul 9, 2018

Renamed 'Generic Column' -> 'Numeric Column'; Fixed a few resource leaks in processing; misc refinements #5957

Merged

jihoonson reviewed Jul 10, 2018

View reviewed changes

nishantmonu51 added this to the 0.13.0 milestone Jul 10, 2018

leventov requested changes Jul 14, 2018

View reviewed changes

nishantmonu51 added 2 commits July 24, 2018 20:34

Merge remote-tracking branch 'upstream/master' into null-handling-pr

711fabe

review comments

cd2703e

jihoonson reviewed Jul 25, 2018

View reviewed changes

leventov reviewed Jul 25, 2018

View reviewed changes

This was referenced Jul 26, 2018

Make non-numeric aggregator factories nullable #6048

Closed

Avoid boxing/unboxing in PostAggregators #6049

Open

review comments

d4a0584

nishantmonu51 added 2 commits July 27, 2018 09:35

fix CalciteQueryTest failure

c839d63

Null Handling - Review comments

aaa9d43

Merge remote-tracking branch 'upstream/master' into null-handling-merge

7963038

nishantmonu51 added 2 commits July 31, 2018 19:21

review comments

7cbebdc

Merge review comments after testing in another branch

e1d37bd

nishantmonu51 added 2 commits July 31, 2018 20:05

review comments

6022648

Merge remote-tracking branch 'upstream/master' into null-handling-pr

7335a2f

asdf2014 reviewed Aug 1, 2018

View reviewed changes

nishantmonu51 added 6 commits August 1, 2018 16:39

fix checkstyle

36c0dc8

fix checkstyle

6c2102f

remove unrelated change

4458b41

fix test failure

a458ade

fix failing test

71904cd

fix travis failures

9fd19e9

jihoonson approved these changes Aug 1, 2018

View reviewed changes

asdf2014 approved these changes Aug 2, 2018

View reviewed changes

nishantmonu51 added 3 commits August 2, 2018 16:30

Merge remote-tracking branch 'remotes/upstream/master' into null-hand…

1437081

…ling-merge

fix merge conflicts

312c5db

Make StringLast and StringFirst aggregators nullable and fix travis f…

2ae1007

…ailures

b-slim merged commit 75c8a87 into apache:master Aug 2, 2018

dclim mentioned this pull request Oct 10, 2018

Druid 0.13.0-incubating release notes #6442

Closed

Conversation

nishantmonu51 commented Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Nullability for Aggregators/Metrics

Math Expressions Null Handling

Filtering on Null values

Changes to Druid build-in SQL layer

Backwards Compatibility

Testing

Uh oh!

nishantmonu51 commented Jul 6, 2018

Uh oh!

leventov commented Jul 7, 2018

Uh oh!

nishantmonu51 commented Jul 9, 2018

Uh oh!

jihoonson commented Jul 9, 2018

Uh oh!

jihoonson commented Jul 9, 2018

Uh oh!

jihoonson commented Jul 10, 2018

Uh oh!

jihoonson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nishantmonu51 commented Jul 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

nishantmonu51 commented Jul 6, 2018 •

edited

Loading