You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "gianm (via GitHub)" <gi...@apache.org> on 2023/05/03 05:33:10 UTC
[GitHub] [druid] gianm commented on a diff in pull request #14139: fix issues with filtering nulls on values coerced to numeric types
gianm commented on code in PR #14139:
URL: https://github.com/apache/druid/pull/14139#discussion_r1183224432
##########
processing/src/main/java/org/apache/druid/math/expr/Evals.java:
##########
@@ -69,6 +69,20 @@ public static boolean asBoolean(@Nullable String x)
return !NullHandling.isNullOrEquivalent(x) && Boolean.parseBoolean(x);
}
+ public static boolean objectAsBoolean(Object val)
Review Comment:
Javadoc would be good here, calling out what happens if `val` is:
- `null`
- something that is not `null` but doesn't have a clear translation to boolean value
Looking at call sites, there are two right now:
- `ComplexExprEval.asBoolean` if the `val` is from a nested type. This one never passes in `null`. I think the only types it can pass in that aren't enumerated here is `List` and `Map`. Those would both currently return false. is that reasonable behavior?
- `ExpressionFilter` for the new `makeObjectPredicate` implementation. Not sure what kind of stuff might show up here?
##########
processing/src/main/java/org/apache/druid/math/expr/ExprEval.java:
##########
@@ -844,8 +849,12 @@ public final ExprEval castTo(ExpressionType castTo)
case STRING:
return ExprEval.ofStringArray(value == null ? null : new Object[] {value.toString()});
}
+ case COMPLEX:
Review Comment:
need a `break;` above this so `ARRAY` doesn't fall through?
##########
processing/src/main/java/org/apache/druid/math/expr/ExprEval.java:
##########
@@ -1089,8 +1102,12 @@ public final ExprEval castTo(ExpressionType castTo)
case STRING:
return ExprEval.ofStringArray(value == null ? null : new Object[] {value});
}
+ case COMPLEX:
Review Comment:
need a `break;` above this so `ARRAY` doesn't fall through?
##########
processing/src/main/java/org/apache/druid/segment/nested/ScalarDoubleColumn.java:
##########
@@ -39,6 +40,8 @@
public class ScalarDoubleColumn implements NestedCommonFormatColumn
{
+ private final boolean replaceWithDefault = NullHandling.replaceWithDefault();
Review Comment:
why waste a field copying a static?
##########
processing/src/main/java/org/apache/druid/segment/virtual/NestedFieldVirtualColumn.java:
##########
@@ -601,15 +606,76 @@ public VectorValueSelector makeVectorValueSelector(
if (!(theColumn instanceof NestedDataComplexColumn)) {
if (parts.isEmpty()) {
- ColumnCapabilities capabilities = holder.getCapabilities();
if (theColumn instanceof DictionaryEncodedColumn) {
- return ExpressionVectorSelectors.castObjectSelectorToNumeric(
- offset,
- this.columnName,
- theColumn.makeVectorObjectSelector(offset),
- capabilities.toColumnType(),
- expectedType
- );
+ final VectorObjectSelector delegate = theColumn.makeVectorObjectSelector(offset);
+ return new VectorValueSelector()
+ {
+ private int currentOffsetId = ReadableVectorInspector.NULL_ID;
+ private long[] longs = new long[delegate.getMaxVectorSize()];
+ private double[] doubles = new double[delegate.getMaxVectorSize()];
+ private float[] floats = new float[delegate.getMaxVectorSize()];
+ private boolean[] nulls = new boolean[delegate.getMaxVectorSize()];
+
+ @Override
+ public long[] getLongVector()
+ {
+ computeNumbers();
+ return longs;
+ }
+
+ @Override
+ public float[] getFloatVector()
+ {
+ computeNumbers();
+ return floats;
+ }
+
+ @Override
+ public double[] getDoubleVector()
+ {
+ computeNumbers();
+ return doubles;
+ }
+
+ @Nullable
+ @Override
+ public boolean[] getNullVector()
+ {
+ computeNumbers();
+ return nulls;
+ }
+
+ @Override
+ public int getMaxVectorSize()
+ {
+ return delegate.getMaxVectorSize();
+ }
+
+ @Override
+ public int getCurrentVectorSize()
+ {
+ return delegate.getCurrentVectorSize();
+ }
+
+ private void computeNumbers()
+ {
+ if (currentOffsetId != offset.getId()) {
+ currentOffsetId = offset.getId();
+ final Object[] values = delegate.getObjectVector();
+ for (int i = 0; i < values.length; i++) {
+ Number n = ExprEval.computeNumber(Evals.asString(values[i]));
+ if (n != null) {
+ longs[i] = n.longValue();
Review Comment:
It's not likely that all of these are going to be needed by the same query. Could the code be structured to avoid computing them all upfront?
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/ArrayConcatOperatorConversion.java:
##########
@@ -34,14 +34,8 @@ public class ArrayConcatOperatorConversion extends DirectOperatorConversion
.operandTypeChecker(
OperandTypes.sequence(
"(array,array)",
- OperandTypes.or(
- OperandTypes.family(SqlTypeFamily.ARRAY),
- OperandTypes.family(SqlTypeFamily.STRING)
- ),
- OperandTypes.or(
- OperandTypes.family(SqlTypeFamily.ARRAY),
- OperandTypes.family(SqlTypeFamily.STRING)
- )
+ OperandTypes.family(SqlTypeFamily.ANY),
+ OperandTypes.family(SqlTypeFamily.ANY)
Review Comment:
why this change, and similar ones in other files? this seems not as good in terms of validation, since these functions likely can't accept just anything?
##########
processing/src/main/java/org/apache/druid/segment/filter/BoundFilter.java:
##########
@@ -106,22 +106,27 @@ public BitmapColumnIndex getBitmapColumnIndex(ColumnIndexSelector selector)
}
final NumericRangeIndex rangeIndex = indexSupplier.as(NumericRangeIndex.class);
if (rangeIndex != null) {
- final Number lower = boundDimFilter.hasLowerBound() ? Double.parseDouble(boundDimFilter.getLower()) : null;
- final Number upper = boundDimFilter.hasUpperBound() ? Double.parseDouble(boundDimFilter.getUpper()) : null;
- final BitmapColumnIndex rangeBitmaps = rangeIndex.forRange(
- lower,
- boundDimFilter.isLowerStrict(),
- upper,
- boundDimFilter.isUpperStrict()
- );
- if (rangeBitmaps != null) {
- // preserve sad backwards compatible behavior where bound filter matches 'null' if the lower bound is not set
- if (boundDimFilter.hasLowerBound() && !NullHandling.isNullOrEquivalent(boundDimFilter.getLower())) {
- return rangeBitmaps;
- } else {
- return wrapRangeIndexWithNullValueIndex(indexSupplier, rangeBitmaps);
+ try {
+ final Number lower = boundDimFilter.hasLowerBound() ? Double.parseDouble(boundDimFilter.getLower()) : null;
+ final Number upper = boundDimFilter.hasUpperBound() ? Double.parseDouble(boundDimFilter.getUpper()) : null;
+ final BitmapColumnIndex rangeBitmaps = rangeIndex.forRange(
+ lower,
+ boundDimFilter.isLowerStrict(),
+ upper,
+ boundDimFilter.isUpperStrict()
+ );
+ if (rangeBitmaps != null) {
+ // preserve sad backwards compatible behavior where bound filter matches 'null' if the lower bound is not set
+ if (boundDimFilter.hasLowerBound() && !NullHandling.isNullOrEquivalent(boundDimFilter.getLower())) {
+ return rangeBitmaps;
+ } else {
+ return wrapRangeIndexWithNullValueIndex(indexSupplier, rangeBitmaps);
+ }
}
}
+ catch (NumberFormatException ignored) {
+ // bounds are not numeric?
+ }
Review Comment:
I too am wondering why we need this here, and if there's a better way we can handle it.
##########
processing/src/main/java/org/apache/druid/math/expr/ExprEval.java:
##########
@@ -549,10 +551,13 @@ public static ExprEval ofType(@Nullable ExpressionType type, @Nullable Object va
}
return ofDouble(null);
case COMPLEX:
- // json isn't currently defined in druid-core, this can be reworked once
- // https://github.com/apache/druid/pull/13698 is merged (or COMPLEX<json> is promoted to a real built-in type(s)
- if ("json".equals(type.getComplexTypeName())) {
- return ofComplex(type, value);
+ if (ExpressionType.NESTED_DATA.equals(type)) {
+ // is the thing actually nested?
+ final Object unwrapped = StructuredData.unwrap(value);
+ if (unwrapped instanceof Map) {
+ return ofComplex(type, unwrapped);
+ }
+ return bestEffortOf(unwrapped);
Review Comment:
Seems fishy: someone asked for an `ExprEval.of` a `COMPLEX<json>` but will get some other type. Why's this needed? If it's really necessary can we call it out louder?
##########
processing/src/main/java/org/apache/druid/math/expr/ExprEval.java:
##########
@@ -918,8 +927,12 @@ public final ExprEval castTo(ExpressionType castTo)
case STRING:
return ExprEval.ofStringArray(value == null ? null : new Object[] {value.toString()});
}
+ case COMPLEX:
Review Comment:
need a `break;` above this so `ARRAY` doesn't fall through?
##########
processing/src/main/java/org/apache/druid/math/expr/ExprEval.java:
##########
@@ -1314,38 +1337,87 @@ public ExpressionType type()
@Override
public boolean isNumericNull()
{
- return false;
+ if (ExpressionType.NESTED_DATA.equals(expressionType)) {
+ computeNumber();
+ return number == null;
+ }
+ return true;
}
@Override
public int asInt()
{
+ if (ExpressionType.NESTED_DATA.equals(expressionType)) {
+ computeNumber();
+ if (number != null) {
+ return number.intValue();
+ }
+ }
return 0;
}
@Override
public long asLong()
{
+ if (ExpressionType.NESTED_DATA.equals(expressionType)) {
+ computeNumber();
+ if (number != null) {
+ return number.longValue();
+ }
+ }
return 0;
}
@Override
public double asDouble()
{
+ if (ExpressionType.NESTED_DATA.equals(expressionType)) {
+ computeNumber();
+ if (number != null) {
+ return number.doubleValue();
+ }
+ }
return 0;
}
@Override
public boolean asBoolean()
{
+ if (ExpressionType.NESTED_DATA.equals(expressionType)) {
+ Object val = StructuredData.unwrap(value);
+ if (val != null) {
+ return Evals.objectAsBoolean(val);
+ }
+ }
return false;
}
+ private void computeNumber()
+ {
+ if (number == null && value != null) {
Review Comment:
Seems like the intent is we cache the number in `number`. If so— that doesn't work in the case where `value != null` but `number` _is_ `null` (like, maybe `val` is a string that is not parseable as a number).
##########
processing/src/main/java/org/apache/druid/segment/nested/ScalarDoubleColumnAndIndexSupplier.java:
##########
@@ -242,12 +256,24 @@ public double estimateSelectivity(int totalRows)
public <T> T computeBitmapResult(BitmapResultFactory<T> bitmapResultFactory)
{
if (doubleValue == null) {
- if (inputNull) {
+ if (inputNull && NullHandling.sqlCompatible()) {
Review Comment:
Should be consistent between using a field (this could be `inputNull && !replaceWithDefault`) or using `NullHandling`. I don't see a good reason to mix them.
##########
processing/src/main/java/org/apache/druid/math/expr/ExprEval.java:
##########
@@ -1314,38 +1337,87 @@ public ExpressionType type()
@Override
public boolean isNumericNull()
{
- return false;
+ if (ExpressionType.NESTED_DATA.equals(expressionType)) {
+ computeNumber();
+ return number == null;
+ }
+ return true;
}
@Override
public int asInt()
{
+ if (ExpressionType.NESTED_DATA.equals(expressionType)) {
+ computeNumber();
+ if (number != null) {
+ return number.intValue();
+ }
+ }
return 0;
}
@Override
public long asLong()
{
+ if (ExpressionType.NESTED_DATA.equals(expressionType)) {
+ computeNumber();
+ if (number != null) {
+ return number.longValue();
+ }
+ }
return 0;
}
@Override
public double asDouble()
{
+ if (ExpressionType.NESTED_DATA.equals(expressionType)) {
+ computeNumber();
+ if (number != null) {
+ return number.doubleValue();
+ }
+ }
return 0;
}
@Override
public boolean asBoolean()
{
+ if (ExpressionType.NESTED_DATA.equals(expressionType)) {
+ Object val = StructuredData.unwrap(value);
+ if (val != null) {
+ return Evals.objectAsBoolean(val);
+ }
+ }
return false;
}
+ private void computeNumber()
+ {
+ if (number == null && value != null) {
+ Object val = StructuredData.unwrap(value);
+ if (val instanceof Number) {
+ number = (Number) val;
+ } else if (val instanceof Boolean) {
+ number = Evals.asLong((Boolean) val);
+ } else if (val instanceof String) {
+ number = ExprEval.computeNumber((String) val);
+ }
+ }
+ }
+
@Nullable
@Override
public Object[] asArray()
{
- return new Object[0];
+ if (ExpressionType.NESTED_DATA.equals(expressionType)) {
Review Comment:
Just a thought. There's so much of this logic that perhaps it would be better to have `NestedDataExprEval` be its own subclass of `ExprEval`, rather than embedding all this conditional logic into `ComplexExprEval`.
`ComplexExprEval` is a private class anyway, so this could be done transparently to callers outside this file.
##########
processing/src/main/java/org/apache/druid/math/expr/ExprEval.java:
##########
@@ -1314,38 +1337,87 @@ public ExpressionType type()
@Override
public boolean isNumericNull()
{
- return false;
+ if (ExpressionType.NESTED_DATA.equals(expressionType)) {
+ computeNumber();
+ return number == null;
+ }
+ return true;
}
@Override
public int asInt()
{
+ if (ExpressionType.NESTED_DATA.equals(expressionType)) {
+ computeNumber();
+ if (number != null) {
+ return number.intValue();
+ }
+ }
return 0;
}
@Override
public long asLong()
{
+ if (ExpressionType.NESTED_DATA.equals(expressionType)) {
+ computeNumber();
+ if (number != null) {
+ return number.longValue();
+ }
+ }
return 0;
}
@Override
public double asDouble()
{
+ if (ExpressionType.NESTED_DATA.equals(expressionType)) {
+ computeNumber();
+ if (number != null) {
+ return number.doubleValue();
+ }
+ }
return 0;
}
@Override
public boolean asBoolean()
{
+ if (ExpressionType.NESTED_DATA.equals(expressionType)) {
+ Object val = StructuredData.unwrap(value);
+ if (val != null) {
+ return Evals.objectAsBoolean(val);
+ }
+ }
return false;
}
+ private void computeNumber()
+ {
+ if (number == null && value != null) {
+ Object val = StructuredData.unwrap(value);
Review Comment:
Do we need to check `ExpressionType.NESTED_DATA.equals(expressionType)`?
##########
processing/src/test/java/org/apache/druid/segment/filter/BaseFilterTest.java:
##########
@@ -317,6 +319,33 @@ public static Collection<Object[]> makeConstructors()
return Pair.of(new IncrementalIndexStorageAdapter(index), index);
}
)
+ .put(
+ "mmappedAutoTypes",
+ input -> {
+ input.mapSchema(
+ schema ->
+ new IncrementalIndexSchema(
+ schema.getMinTimestamp(),
+ schema.getTimestampSpec(),
+ schema.getGran(),
+ schema.getVirtualColumns(),
+ schema.getDimensionsSpec().withDimensions(
+ schema.getDimensionsSpec()
+ .getDimensions()
+ .stream()
+ .map(
+ dimensionSchema -> new AutoTypeColumnSchema(dimensionSchema.getName())
Review Comment:
nice. excellent test.
##########
processing/src/test/java/org/apache/druid/segment/filter/BaseFilterTest.java:
##########
@@ -816,58 +845,66 @@ private void assertFilterMatches(
final boolean testVectorized
)
{
- Assert.assertEquals(
- "Cursor: " + filter,
- expectedRows,
- selectColumnValuesMatchingFilter(filter, "dim0")
- );
-
- if (testVectorized) {
+ try {
Assert.assertEquals(
- "Cursor (vectorized): " + filter,
+ "Cursor: " + filter,
expectedRows,
- selectColumnValuesMatchingFilterUsingVectorCursor(filter, "dim0")
+ selectColumnValuesMatchingFilter(filter, "dim0")
);
+ if (testVectorized) {
+ Assert.assertEquals(
+ "Cursor (vectorized): " + filter,
+ expectedRows,
+ selectColumnValuesMatchingFilterUsingVectorCursor(filter, "dim0")
+ );
+
+ Assert.assertEquals(
+ "Cursor Virtual Column (vectorized): " + filter,
+ expectedRows,
+ selectColumnValuesMatchingFilterUsingVectorVirtualColumnCursor(filter, "vdim0", "dim0")
+ );
+ }
+
Assert.assertEquals(
- "Cursor Virtual Column (vectorized): " + filter,
+ "Cursor with postFiltering: " + filter,
expectedRows,
- selectColumnValuesMatchingFilterUsingVectorVirtualColumnCursor(filter, "vdim0", "dim0")
+ selectColumnValuesMatchingFilterUsingPostFiltering(filter, "dim0")
);
- }
- Assert.assertEquals(
- "Cursor with postFiltering: " + filter,
- expectedRows,
- selectColumnValuesMatchingFilterUsingPostFiltering(filter, "dim0")
- );
+ if (testVectorized) {
+ Assert.assertEquals(
+ "Cursor with postFiltering (vectorized): " + filter,
+ expectedRows,
+ selectColumnValuesMatchingFilterUsingVectorizedPostFiltering(filter, "dim0")
+ );
+ }
- if (testVectorized) {
Assert.assertEquals(
- "Cursor with postFiltering (vectorized): " + filter,
- expectedRows,
- selectColumnValuesMatchingFilterUsingVectorizedPostFiltering(filter, "dim0")
+ "Filtered aggregator: " + filter,
+ expectedRows.size(),
+ selectCountUsingFilteredAggregator(filter)
);
- }
- Assert.assertEquals(
- "Filtered aggregator: " + filter,
- expectedRows.size(),
- selectCountUsingFilteredAggregator(filter)
- );
+ if (testVectorized) {
+ Assert.assertEquals(
+ "Filtered aggregator (vectorized): " + filter,
+ expectedRows.size(),
+ selectCountUsingVectorizedFilteredAggregator(filter)
+ );
+ }
- if (testVectorized) {
Assert.assertEquals(
- "Filtered aggregator (vectorized): " + filter,
- expectedRows.size(),
- selectCountUsingVectorizedFilteredAggregator(filter)
+ "RowBasedColumnSelectorFactory: " + filter,
+ expectedRows,
+ selectColumnValuesMatchingFilterUsingRowBasedColumnSelectorFactory(filter, "dim0")
);
}
-
- Assert.assertEquals(
- "RowBasedColumnSelectorFactory: " + filter,
- expectedRows,
- selectColumnValuesMatchingFilterUsingRowBasedColumnSelectorFactory(filter, "dim0")
- );
+ catch (ISE ise) {
Review Comment:
Please update the logic to only suppress these `ARRAY<STRING>` arrays when they occur during testing nested columns. Otherwise we might miss `ARRAY<STRING>` related errors that creep in the future in other cases.
##########
processing/src/main/java/org/apache/druid/segment/filter/BoundFilter.java:
##########
@@ -106,22 +106,27 @@ public BitmapColumnIndex getBitmapColumnIndex(ColumnIndexSelector selector)
}
final NumericRangeIndex rangeIndex = indexSupplier.as(NumericRangeIndex.class);
if (rangeIndex != null) {
- final Number lower = boundDimFilter.hasLowerBound() ? Double.parseDouble(boundDimFilter.getLower()) : null;
- final Number upper = boundDimFilter.hasUpperBound() ? Double.parseDouble(boundDimFilter.getUpper()) : null;
- final BitmapColumnIndex rangeBitmaps = rangeIndex.forRange(
- lower,
- boundDimFilter.isLowerStrict(),
- upper,
- boundDimFilter.isUpperStrict()
- );
- if (rangeBitmaps != null) {
- // preserve sad backwards compatible behavior where bound filter matches 'null' if the lower bound is not set
- if (boundDimFilter.hasLowerBound() && !NullHandling.isNullOrEquivalent(boundDimFilter.getLower())) {
- return rangeBitmaps;
- } else {
- return wrapRangeIndexWithNullValueIndex(indexSupplier, rangeBitmaps);
+ try {
+ final Number lower = boundDimFilter.hasLowerBound() ? Double.parseDouble(boundDimFilter.getLower()) : null;
+ final Number upper = boundDimFilter.hasUpperBound() ? Double.parseDouble(boundDimFilter.getUpper()) : null;
+ final BitmapColumnIndex rangeBitmaps = rangeIndex.forRange(
+ lower,
+ boundDimFilter.isLowerStrict(),
+ upper,
+ boundDimFilter.isUpperStrict()
+ );
+ if (rangeBitmaps != null) {
+ // preserve sad backwards compatible behavior where bound filter matches 'null' if the lower bound is not set
+ if (boundDimFilter.hasLowerBound() && !NullHandling.isNullOrEquivalent(boundDimFilter.getLower())) {
+ return rangeBitmaps;
+ } else {
+ return wrapRangeIndexWithNullValueIndex(indexSupplier, rangeBitmaps);
+ }
}
}
+ catch (NumberFormatException ignored) {
+ // bounds are not numeric?
+ }
Review Comment:
I too am wondering why we need this here, and if there's a better way we can handle it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org