gianm commented on code in PR #14654:
URL: https://github.com/apache/druid/pull/14654#discussion_r1275277989
##########
processing/src/main/java/org/apache/druid/math/expr/ExprEval.java:
##########
@@ -644,6 +644,37 @@ public static Number computeNumber(@Nullable String value)
return rv;
}
+ /**
+ * Cast an {@link ExprEval} to some {@link ExpressionType} that the value
will be compared with. If the value is not
+ * appropriate to use for comparison after casting, this method returns
null. For example, the
+ * {@link ExpressionType#DOUBLE} value 1.1 when cast to {@link
ExpressionType#LONG} becomes 1L, which is no longer
+ * appropriate to use for equality or range comparisons, while 1.0 is valid.
Review Comment:
Why "or range comparisons"? The value `1.1` is appropriate to use for `long`
typed ranges, because, for example, `2L` is greater than `1.1`. The
open/closedness of the bounds may need to be adjusted but that does not mean
the value is inappropriate.
##########
processing/src/main/java/org/apache/druid/query/filter/RangeFilter.java:
##########
@@ -514,10 +514,12 @@ private Supplier<DruidLongPredicate>
makeLongPredicateSupplier()
if (hasLowerBound()) {
ExprEval<?> lowerCast = lowerEval.castTo(ExpressionType.LONG);
if (lowerCast.isNumericNull()) {
+ // lower value is not null, but isn't convertible to a long, so is
effectively null, we treat this as not
+ // having a lower bound to be consistent with the bound filter, but
perhaps this is controversial...
Review Comment:
Hmm. It seems like, to aid in SQL compatibility, it would be better for this
case to be always-false. (Or, in tri-valued logic, always-`UNKNOWN`). Lower
value that is not convertible to the target type is like having a comparison `x
> NULL`, which is `NULL` (a.k.a. `UNKNOWN`) itself in tri-valued logic.
##########
processing/src/main/java/org/apache/druid/query/filter/RangeFilter.java:
##########
@@ -614,11 +617,21 @@ private Predicate<Object[]>
makeArrayPredicate(TypeSignature<ValueType> inputTyp
final Comparator<Object[]> arrayComparator =
inputType.getNullableStrategy();
final ExpressionType expressionType =
ExpressionType.fromColumnTypeStrict(inputType);
final RangeType rangeType = RangeType.of(hasLowerBound(), lowerOpen,
hasUpperBound(), upperOpen);
- final Object[] lowerBound = hasLowerBound() ?
lowerEval.castTo(expressionType).asArray() : null;
- final Object[] upperBound = hasUpperBound() ?
upperEval.castTo(expressionType).asArray() : null;
+ final Object[] lowerBound = hasLowerBound()
Review Comment:
Love the deranged vibes of nested ternaries, but please rewrite to avoid
them.
##########
processing/src/main/java/org/apache/druid/segment/nested/ScalarLongColumnAndIndexSupplier.java:
##########
@@ -417,9 +417,13 @@ public BitmapColumnIndex forRange(
{
final FixedIndexed<Long> dictionary = longDictionarySupplier.get();
IntIntPair range = dictionary.getRange(
- startValue == null ? null : startValue.longValue(),
Review Comment:
avoid nested ternary
##########
processing/src/main/java/org/apache/druid/query/filter/RangeFilter.java:
##########
@@ -531,8 +533,9 @@ private Supplier<DruidLongPredicate>
makeLongPredicateSupplier()
// upper value is not null, but isn't convertible to a long so is
effectively null, nothing matches
return DruidLongPredicate.ALWAYS_FALSE;
} else {
+ // take the ceil if open or floor if not of the match value as a
double so that we can
Review Comment:
so that we can…?
##########
processing/src/main/java/org/apache/druid/segment/nested/NestedFieldColumnIndexSupplier.java:
##########
@@ -730,9 +730,13 @@ public BitmapColumnIndex forRange(
)
{
return makeRangeIndex(
- startValue != null ? startValue.longValue() : null,
+ startValue == null
Review Comment:
avoid nested ternary
##########
processing/src/main/java/org/apache/druid/query/filter/RangeFilter.java:
##########
@@ -531,8 +533,9 @@ private Supplier<DruidLongPredicate>
makeLongPredicateSupplier()
// upper value is not null, but isn't convertible to a long so is
effectively null, nothing matches
return DruidLongPredicate.ALWAYS_FALSE;
} else {
+ // take the ceil if open or floor if not of the match value as a
double so that we can
Review Comment:
so that we can…?
##########
processing/src/main/java/org/apache/druid/query/filter/RangeFilter.java:
##########
@@ -812,6 +826,25 @@ public String toString()
}
}
+ /**
+ * if casting to a LONG typed ARRAY, first cast to a DOUBLE typed ARRAY and
then {@link Math#ceil(double)} on the
Review Comment:
Would be good to say that the intent of this method is to be like
`castTo(inputType).asArray()`, but with one difference: when the target is
long, and the inputs are decimals, we ceil them instead of flooring them.
##########
processing/src/main/java/org/apache/druid/query/filter/RangeFilter.java:
##########
@@ -812,6 +826,25 @@ public String toString()
}
}
+ /**
+ * if casting to a LONG typed ARRAY, first cast to a DOUBLE typed ARRAY and
then {@link Math#ceil(double)} on the
+ * values to ensure a correct lower bound
+ */
+ private static Object[] castArrayToCeil(ExprEval<?> lowerEval,
ExpressionType inputType)
+ {
+ if (ExpressionType.LONG_ARRAY.equals(inputType)) {
+ final ExprEval<?> doubleArray =
lowerEval.castTo(ExpressionType.DOUBLE_ARRAY);
Review Comment:
This is going to lose some fidelity if the input values are numeric strings
that cannot be represented as doubles. To maintain fidelity, it would be better
to cast each value individually, trying long first (using
`GuavaUtils.tryParseLong`) and then using double only if the coercion to long
can't be done.
##########
processing/src/main/java/org/apache/druid/query/filter/vector/VectorValueMatcherFactory.java:
##########
@@ -27,8 +27,14 @@
public interface VectorValueMatcherFactory
{
+ /**
+ * Specialized value matcher for string equality
+ */
VectorValueMatcher makeMatcher(@Nullable String value);
+ /**
+ * Specialized value matcher for equality
Review Comment:
for what kind of equality? Javadoc should call out when this method would be
used. (Otherwise it's hard for implementers to know what kind of `Object` might
show up.)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
