[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r201364128 ## File path: common/src/main/java/io/druid/math/expr/Expr.java ## @@ -365,15 +371,21 @@ public ExprEval eval(ObjectBinding bindings) // Result of any Binary expressions is null if any of the argument is null. // e.g "select null * 2 as c;" or "select null + 1 as c;" will return null as per Standard SQL spec. -if (NullHandling.sqlCompatible() && (leftVal.isNull() || rightVal.isNull())) { +if (NullHandling.sqlCompatible() && (leftVal.value() == null || rightVal.value() == null)) { return ExprEval.of(null); } 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())) { Review comment: 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. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r201366966 ## File path: common/src/main/java/io/druid/math/expr/Expr.java ## @@ -365,15 +371,21 @@ public ExprEval eval(ObjectBinding bindings) // Result of any Binary expressions is null if any of the argument is null. // e.g "select null * 2 as c;" or "select null + 1 as c;" will return null as per Standard SQL spec. -if (NullHandling.sqlCompatible() && (leftVal.isNull() || rightVal.isNull())) { +if (NullHandling.sqlCompatible() && (leftVal.value() == null || rightVal.value() == null)) { return ExprEval.of(null); } 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())) { Review comment: https://github.com/apache/incubator-druid/issues/5991 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r201367313 ## File path: common/src/main/java/io/druid/math/expr/ExprEval.java ## @@ -245,36 +252,63 @@ public final ExprType type() @Override public final int asInt() { - if (value == null) { + Number number = asNumber(); + if (number == null) { assert NullHandling.replaceWithDefault(); return 0; } - - final Integer theInt = Ints.tryParse(value); - assert NullHandling.replaceWithDefault() || theInt != null; - return theInt == null ? 0 : theInt; + return number.intValue(); } @Override public final long asLong() { - // GuavaUtils.tryParseLong handles nulls, no need for special null handling here. - final Long theLong = GuavaUtils.tryParseLong(value); - assert NullHandling.replaceWithDefault() || theLong != null; - return theLong == null ? 0L : theLong; + Number number = asNumber(); + if (number == null) { +assert NullHandling.replaceWithDefault(); Review comment: IIRC, I was actually doing that before, but changed this to assert based on discussions on previous PRs. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r201371259 ## File path: extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/ApproximateHistogramAggregator.java ## @@ -59,7 +60,9 @@ public ApproximateHistogramAggregator( @Override public void aggregate() { -histogram.offer(selector.getFloat()); +if (NullHandling.replaceWithDefault() || !selector.isNull()) { Review comment: 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. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r201373058 ## File path: extensions-core/lookups-cached-single/src/main/java/io/druid/server/lookup/LoadingLookup.java ## @@ -62,15 +63,19 @@ public LoadingLookup( @Override - public String apply(final String key) + public String apply(@Nullable final String key) { -if (key == null) { +String keyEquivalent = NullHandling.nullToEmptyIfNeeded(key); Review comment: 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". This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204795900 ## File path: processing/src/main/java/io/druid/query/aggregation/first/FloatFirstAggregatorFactory.java ## @@ -32,19 +32,21 @@ import io.druid.query.aggregation.AggregatorFactory; import io.druid.query.aggregation.AggregatorUtil; import io.druid.query.aggregation.BufferAggregator; +import io.druid.query.aggregation.NullableAggregatorFactory; import io.druid.query.monomorphicprocessing.RuntimeShapeInspector; -import io.druid.segment.BaseObjectColumnValueSelector; import io.druid.segment.ColumnSelectorFactory; +import io.druid.segment.ColumnValueSelector; import io.druid.segment.column.Column; +import javax.annotation.Nullable; import java.nio.ByteBuffer; import java.util.Arrays; import java.util.Comparator; import java.util.List; import java.util.Map; import java.util.Objects; -public class FloatFirstAggregatorFactory extends AggregatorFactory +public class FloatFirstAggregatorFactory extends NullableAggregatorFactory Review comment: not a blocker for this PR, would like to do it a follow up PR - https://github.com/apache/incubator-druid/issues/6039 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204797189 ## File path: processing/src/main/java/io/druid/query/expression/ExprUtils.java ## @@ -73,7 +74,14 @@ public static PeriodGranularity toPeriodGranularity( } else { 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)) { Review comment: https://github.com/apache/incubator-druid/issues/6040 Will fix this line in this PR and prohibit the api in future PR. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204801468 ## File path: processing/src/main/java/io/druid/query/groupby/orderby/DefaultLimitSpec.java ## @@ -269,12 +270,22 @@ public int compare(Row left, Row right) private Ordering metricOrdering(final String column, final Comparator comparator) { -return Ordering.from(Comparator.comparing((Row row) -> row.getRaw(column), Comparator.nullsLast(comparator))); +if (NullHandling.sqlCompatible()) { + return Ordering.from(Comparator.comparing((Row row) -> row.getRaw(column), Comparator.nullsFirst(comparator))); +} else { + return Ordering.from(Comparator.comparing((Row row) -> row.getRaw(column), Comparator.nullsLast(comparator))); +} } private Ordering dimensionOrdering(final String dimension, final StringComparator comparator) { -return Ordering.from(Comparator.comparing((Row row) -> row.getDimension(dimension).isEmpty() ? null : row.getDimension(dimension).get(0), Comparator.nullsFirst(comparator))); +return Ordering.from( +Comparator.comparing( +(Row row) -> row.getDimension(dimension).isEmpty() Review comment: Its just a formatting change, no logic changed in this PR. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204801468 ## File path: processing/src/main/java/io/druid/query/groupby/orderby/DefaultLimitSpec.java ## @@ -269,12 +270,22 @@ public int compare(Row left, Row right) private Ordering metricOrdering(final String column, final Comparator comparator) { -return Ordering.from(Comparator.comparing((Row row) -> row.getRaw(column), Comparator.nullsLast(comparator))); +if (NullHandling.sqlCompatible()) { + return Ordering.from(Comparator.comparing((Row row) -> row.getRaw(column), Comparator.nullsFirst(comparator))); +} else { + return Ordering.from(Comparator.comparing((Row row) -> row.getRaw(column), Comparator.nullsLast(comparator))); +} } private Ordering dimensionOrdering(final String dimension, final StringComparator comparator) { -return Ordering.from(Comparator.comparing((Row row) -> row.getDimension(dimension).isEmpty() ? null : row.getDimension(dimension).get(0), Comparator.nullsFirst(comparator))); +return Ordering.from( +Comparator.comparing( +(Row row) -> row.getDimension(dimension).isEmpty() Review comment: Its just a formatting change as the line was longer than 120 cols, no logic changed in this PR. reformatted to make it more readable. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204801468 ## File path: processing/src/main/java/io/druid/query/groupby/orderby/DefaultLimitSpec.java ## @@ -269,12 +270,22 @@ public int compare(Row left, Row right) private Ordering metricOrdering(final String column, final Comparator comparator) { -return Ordering.from(Comparator.comparing((Row row) -> row.getRaw(column), Comparator.nullsLast(comparator))); +if (NullHandling.sqlCompatible()) { + return Ordering.from(Comparator.comparing((Row row) -> row.getRaw(column), Comparator.nullsFirst(comparator))); +} else { + return Ordering.from(Comparator.comparing((Row row) -> row.getRaw(column), Comparator.nullsLast(comparator))); +} } private Ordering dimensionOrdering(final String dimension, final StringComparator comparator) { -return Ordering.from(Comparator.comparing((Row row) -> row.getDimension(dimension).isEmpty() ? null : row.getDimension(dimension).get(0), Comparator.nullsFirst(comparator))); +return Ordering.from( +Comparator.comparing( +(Row row) -> row.getDimension(dimension).isEmpty() Review comment: Its just a formatting change as the line was longer than 120 cols, no logic changed in this PR. reformatted to make it more readable. also remove dupe call to getDimension This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204806961 ## File path: processing/src/main/java/io/druid/segment/virtual/ExpressionColumnValueSelector.java ## @@ -82,6 +82,6 @@ public void inspectRuntimeShape(RuntimeShapeInspector inspector) @Override public boolean isNull() { -return getObject().isNull(); +return getObject().isNumericNull(); Review comment: 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. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204810580 ## File path: processing/src/main/java/io/druid/query/expression/ExprUtils.java ## @@ -73,7 +74,14 @@ public static PeriodGranularity toPeriodGranularity( } else { 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)) { Review comment: done. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204810693 ## File path: processing/src/main/java/io/druid/query/filter/ValueGetter.java ## @@ -27,5 +29,7 @@ // 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. Review comment: done. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204810633 ## File path: processing/src/main/java/io/druid/query/filter/SelectorDimFilter.java ## @@ -90,7 +95,7 @@ public SelectorDimFilter( @Override public DimFilter optimize() { -return new InDimFilter(dimension, ImmutableList.of(value), extractionFn).optimize(); +return new InDimFilter(dimension, Arrays.asList(value), extractionFn).optimize(); Review comment: done. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204810789 ## File path: processing/src/main/java/io/druid/query/groupby/orderby/DefaultLimitSpec.java ## @@ -269,12 +270,22 @@ public int compare(Row left, Row right) private Ordering metricOrdering(final String column, final Comparator comparator) { -return Ordering.from(Comparator.comparing((Row row) -> row.getRaw(column), Comparator.nullsLast(comparator))); +if (NullHandling.sqlCompatible()) { Review comment: added comment. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204810994 ## File path: processing/src/main/java/io/druid/query/topn/TopNMapFn.java ## @@ -56,14 +56,10 @@ return longVal == null ? DimensionHandlerUtils.ZERO_LONG : longVal; }; - private static Function FLOAT_TRANSFORMER = input -> { -final Float floatVal = DimensionHandlerUtils.convertObjectToFloat(input); -return floatVal == null ? DimensionHandlerUtils.ZERO_FLOAT : floatVal; - }; - private static Function DOUBLE_TRANSFORMER = input -> { -final Double doubleValue = DimensionHandlerUtils.convertObjectToDouble(input); -return doubleValue == null ? DimensionHandlerUtils.ZERO_DOUBLE : doubleValue; - }; + private static Function FLOAT_TRANSFORMER = input -> DimensionHandlerUtils.convertObjectToFloat(input); Review comment: done. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204810950 ## File path: processing/src/main/java/io/druid/query/groupby/orderby/DefaultLimitSpec.java ## @@ -269,12 +270,22 @@ public int compare(Row left, Row right) private Ordering metricOrdering(final String column, final Comparator comparator) { -return Ordering.from(Comparator.comparing((Row row) -> row.getRaw(column), Comparator.nullsLast(comparator))); +if (NullHandling.sqlCompatible()) { + return Ordering.from(Comparator.comparing((Row row) -> row.getRaw(column), Comparator.nullsFirst(comparator))); +} else { + return Ordering.from(Comparator.comparing((Row row) -> row.getRaw(column), Comparator.nullsLast(comparator))); +} } private Ordering dimensionOrdering(final String dimension, final StringComparator comparator) { -return Ordering.from(Comparator.comparing((Row row) -> row.getDimension(dimension).isEmpty() ? null : row.getDimension(dimension).get(0), Comparator.nullsFirst(comparator))); +return Ordering.from( +Comparator.comparing( +(Row row) -> row.getDimension(dimension).isEmpty() Review comment: extracted method to avoid calling getDimension twice. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204811034 ## File path: processing/src/main/java/io/druid/query/topn/TopNMapFn.java ## @@ -56,14 +56,10 @@ return longVal == null ? DimensionHandlerUtils.ZERO_LONG : longVal; }; - private static Function FLOAT_TRANSFORMER = input -> { -final Float floatVal = DimensionHandlerUtils.convertObjectToFloat(input); -return floatVal == null ? DimensionHandlerUtils.ZERO_FLOAT : floatVal; - }; - private static Function DOUBLE_TRANSFORMER = input -> { -final Double doubleValue = DimensionHandlerUtils.convertObjectToDouble(input); -return doubleValue == null ? DimensionHandlerUtils.ZERO_DOUBLE : doubleValue; - }; + private static Function FLOAT_TRANSFORMER = input -> DimensionHandlerUtils.convertObjectToFloat(input); + + private static Function DOUBLE_TRANSFORMER = input -> DimensionHandlerUtils.convertObjectToDouble( Review comment: done. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204811109 ## File path: processing/src/main/java/io/druid/segment/virtual/ExpressionColumnValueSelector.java ## @@ -82,6 +82,6 @@ public void inspectRuntimeShape(RuntimeShapeInspector inspector) @Override public boolean isNull() { -return getObject().isNull(); +return getObject().isNumericNull(); Review comment: added comment to code. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204811346 ## File path: processing/src/main/java/io/druid/segment/virtual/SingleLongInputCachingExpressionColumnValueSelector.java ## @@ -150,6 +150,6 @@ private ExprEval eval(final long value) @Override public boolean isNull() { -return getObject().isNull(); +return getObject().isNumericNull(); Review comment: 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. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204811156 ## File path: processing/src/main/java/io/druid/segment/virtual/ExpressionSelectors.java ## @@ -248,23 +248,32 @@ public void inspectRuntimeShape(RuntimeShapeInspector inspector) { final Map> suppliers = Maps.newHashMap(); for (String columnName : Parser.findRequiredBindings(expression)) { - final ColumnCapabilities columnCapabilities = columnSelectorFactory.getColumnCapabilities(columnName); + final ColumnCapabilities columnCapabilities = columnSelectorFactory + .getColumnCapabilities(columnName); final ValueType nativeType = columnCapabilities != null ? columnCapabilities.getType() : null; final Supplier supplier; if (nativeType == ValueType.FLOAT) { -supplier = columnSelectorFactory.makeColumnValueSelector(columnName)::getFloat; +ColumnValueSelector selector = columnSelectorFactory +.makeColumnValueSelector(columnName); +supplier = makeNullableSupplier(selector, selector::getFloat); } else if (nativeType == ValueType.LONG) { -supplier = columnSelectorFactory.makeColumnValueSelector(columnName)::getLong; +ColumnValueSelector selector = columnSelectorFactory +.makeColumnValueSelector(columnName); +supplier = makeNullableSupplier(selector, selector::getLong); } else if (nativeType == ValueType.DOUBLE) { -supplier = columnSelectorFactory.makeColumnValueSelector(columnName)::getDouble; +ColumnValueSelector selector = columnSelectorFactory +.makeColumnValueSelector(columnName); +supplier = makeNullableSupplier(selector, selector::getDouble); } else if (nativeType == ValueType.STRING) { supplier = supplierFromDimensionSelector( -columnSelectorFactory.makeDimensionSelector(new DefaultDimensionSpec(columnName, columnName)) +columnSelectorFactory +.makeDimensionSelector(new DefaultDimensionSpec(columnName, columnName)) ); } else if (nativeType == null) { // Unknown ValueType. Try making an Object selector and see if that gives us anything useful. -supplier = supplierFromObjectSelector(columnSelectorFactory.makeColumnValueSelector(columnName)); +supplier = supplierFromObjectSelector(columnSelectorFactory +.makeColumnValueSelector(columnName)); Review comment: done. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204811220 ## File path: processing/src/main/java/io/druid/segment/virtual/SingleStringInputCachingExpressionColumnValueSelector.java ## @@ -139,7 +139,7 @@ private ExprEval eval() @Override public boolean isNull() { -return eval().isNull(); +return eval().isNumericNull(); Review comment: added comment to code. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204811378 ## File path: server/src/main/java/io/druid/server/listener/announcer/ListeningAnnouncerConfig.java ## @@ -93,7 +93,7 @@ public String getAnnouncementPath(String listenerName) { return ZKPaths.makePath( getListenersPath(), Preconditions.checkNotNull( Review comment: done. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204811510 ## File path: sql/src/main/java/io/druid/sql/calcite/planner/Calcites.java ## @@ -107,6 +108,7 @@ public static SchemaPlus createRootSchema(final Schema druidSchema, final Author public static String escapeStringLiteral(final String s) { +Preconditions.checkNotNull(s); if (s == null) { Review comment: removed the == null check, it was not needed. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204811571 ## File path: sql/src/main/java/io/druid/sql/calcite/rel/DruidSemiJoin.java ## @@ -294,7 +295,11 @@ public RelOptCost computeSelfCost(final RelOptPlanner planner, final RelMetadata for (int i : rightKeys) { 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. Review comment: done. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204811628 ## File path: sql/src/main/java/io/druid/sql/calcite/planner/DruidRexExecutor.java ## @@ -84,7 +84,7 @@ public void reduce( if (sqlTypeName == SqlTypeName.BOOLEAN) { 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()) { Review comment: 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. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204811659 ## File path: sql/src/main/java/io/druid/sql/calcite/rel/DruidSemiJoin.java ## @@ -308,16 +313,18 @@ public RelOptCost computeSelfCost(final RelOptPlanner planner, final RelMetadata for (int i = 0; i < values.size(); i++) { final String value = values.get(i); -subConditions.add( -getCluster().getRexBuilder().makeCall( -SqlStdOperatorTable.EQUALS, -leftExpressions.get(i), -getCluster().getRexBuilder().makeLiteral(value) -) -); +// NULLS are not supposed to match NULLs in a join. So ignore them. Review comment: done. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204812646 ## File path: extensions-core/histogram/src/main/java/io/druid/query/aggregation/histogram/ApproximateHistogramAggregator.java ## @@ -59,7 +60,9 @@ public ApproximateHistogramAggregator( @Override public void aggregate() { -histogram.offer(selector.getFloat()); +if (NullHandling.replaceWithDefault() || !selector.isNull()) { Review comment: sure. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204815667 ## File path: common/src/main/java/io/druid/math/expr/ExprEval.java ## @@ -99,10 +98,10 @@ public Object value() return value; } - public boolean isNull() Review comment: 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. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org
[GitHub] nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 commented on a change in pull request #5958: Part 2 of changes for SQL Compatible Null Handling URL: https://github.com/apache/incubator-druid/pull/5958#discussion_r204816952 ## File path: common/src/main/java/io/druid/math/expr/ExprEval.java ## @@ -245,36 +252,63 @@ public final ExprType type() @Override public final int asInt() { - if (value == null) { + Number number = asNumber(); + if (number == null) { assert NullHandling.replaceWithDefault(); return 0; } - - final Integer theInt = Ints.tryParse(value); - assert NullHandling.replaceWithDefault() || theInt != null; - return theInt == null ? 0 : theInt; + return number.intValue(); } @Override public final long asLong() { - // GuavaUtils.tryParseLong handles nulls, no need for special null handling here. - final Long theLong = GuavaUtils.tryParseLong(value); - assert NullHandling.replaceWithDefault() || theLong != null; - return theLong == null ? 0L : theLong; + Number number = asNumber(); + if (number == null) { +assert NullHandling.replaceWithDefault(); Review comment: 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. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@druid.apache.org For additional commands, e-mail: dev-h...@druid.apache.org