[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
