agavra commented on code in PR #9829:
URL: https://github.com/apache/pinot/pull/9829#discussion_r1025866202
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/operands/FilterOperand.java:
##########
@@ -230,14 +230,41 @@ public Predicate(List<RexExpression> functionOperands,
DataSchema dataSchema) {
"Expected 2 function ops for Predicate but got:" +
functionOperands.size());
_lhs = TransformOperand.toTransformOperand(functionOperands.get(0),
dataSchema);
_rhs = TransformOperand.toTransformOperand(functionOperands.get(1),
dataSchema);
- if (_lhs._resultType != null && _lhs._resultType !=
DataSchema.ColumnDataType.OBJECT) {
- _resultType = _lhs._resultType;
- } else if (_rhs._resultType != null && _rhs._resultType !=
DataSchema.ColumnDataType.OBJECT) {
- _resultType = _rhs._resultType;
+ _resultType = resolveResultType(_lhs._resultType, _rhs._resultType);
+ }
+
+ private static DataSchema.ColumnDataType
resolveResultType(DataSchema.ColumnDataType lhsType,
+ DataSchema.ColumnDataType rhsType) {
+ if (lhsType == null) {
+ return rhsType;
+ } else if (rhsType == null) {
+ return lhsType;
} else {
- // TODO: we should correctly throw exception here. Currently exception
thrown during constructor is not
- // piped back to query dispatcher, thus we set it to null and
deliberately make the processing throw exception.
- _resultType = null;
+ if (isSuperType(lhsType, rhsType)) {
+ return lhsType;
+ } else if (isSuperType(rhsType, lhsType)) {
+ return rhsType;
+ } else {
+ return null;
Review Comment:
can we add the TODO back here?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/operands/FilterOperand.java:
##########
@@ -230,14 +230,41 @@ public Predicate(List<RexExpression> functionOperands,
DataSchema dataSchema) {
"Expected 2 function ops for Predicate but got:" +
functionOperands.size());
_lhs = TransformOperand.toTransformOperand(functionOperands.get(0),
dataSchema);
_rhs = TransformOperand.toTransformOperand(functionOperands.get(1),
dataSchema);
- if (_lhs._resultType != null && _lhs._resultType !=
DataSchema.ColumnDataType.OBJECT) {
- _resultType = _lhs._resultType;
- } else if (_rhs._resultType != null && _rhs._resultType !=
DataSchema.ColumnDataType.OBJECT) {
- _resultType = _rhs._resultType;
+ _resultType = resolveResultType(_lhs._resultType, _rhs._resultType);
+ }
+
+ private static DataSchema.ColumnDataType
resolveResultType(DataSchema.ColumnDataType lhsType,
+ DataSchema.ColumnDataType rhsType) {
+ if (lhsType == null) {
+ return rhsType;
+ } else if (rhsType == null) {
+ return lhsType;
} else {
- // TODO: we should correctly throw exception here. Currently exception
thrown during constructor is not
- // piped back to query dispatcher, thus we set it to null and
deliberately make the processing throw exception.
- _resultType = null;
+ if (isSuperType(lhsType, rhsType)) {
+ return lhsType;
+ } else if (isSuperType(rhsType, lhsType)) {
+ return rhsType;
+ } else {
+ return null;
+ }
+ }
+ }
+
+ private static boolean isSuperType(DataSchema.ColumnDataType superType,
DataSchema.ColumnDataType subType) {
Review Comment:
hmm, this seems pretty hacky, I think this solution needs a bit of tweaking
(I think it handles Objects in the opposite way you intended) but what do you
think of this:
```java
public static void main(String[] args) {
foo(ColumnDataType.LONG, ColumnDataType.INT);
foo(ColumnDataType.STRING, ColumnDataType.INT);
foo(ColumnDataType.OBJECT, ColumnDataType.STRING);
foo(ColumnDataType.LONG, ColumnDataType.DOUBLE);
}
static void foo(ColumnDataType lType, ColumnDataType rType) {
Class<?> lClazz =
Optional.ofNullable(lType.getNullPlaceholder()).orElse(new Object()).getClass();
Class<?> rClazz =
Optional.ofNullable(rType.getNullPlaceholder()).orElse(new Object()).getClass();
Ordering<Class<?>> numerics = Ordering.explicit(
Integer.class,
Long.class,
Float.class,
Double.class,
BigDecimal.class
);
if (Number.class.isAssignableFrom(lClazz) &&
Number.class.isAssignableFrom(rClazz)) {
System.out.println(numerics.max(lClazz, rClazz));
} else if (lClazz.isAssignableFrom(rClazz)) {
System.out.println(lClazz);
} else if (rClazz.isAssignableFrom(lClazz)) {
System.out.println(rClazz);
} else {
System.out.println("null");
}
}
```
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/operands/FilterOperand.java:
##########
@@ -230,14 +230,41 @@ public Predicate(List<RexExpression> functionOperands,
DataSchema dataSchema) {
"Expected 2 function ops for Predicate but got:" +
functionOperands.size());
_lhs = TransformOperand.toTransformOperand(functionOperands.get(0),
dataSchema);
_rhs = TransformOperand.toTransformOperand(functionOperands.get(1),
dataSchema);
- if (_lhs._resultType != null && _lhs._resultType !=
DataSchema.ColumnDataType.OBJECT) {
- _resultType = _lhs._resultType;
- } else if (_rhs._resultType != null && _rhs._resultType !=
DataSchema.ColumnDataType.OBJECT) {
- _resultType = _rhs._resultType;
+ _resultType = resolveResultType(_lhs._resultType, _rhs._resultType);
+ }
+
+ private static DataSchema.ColumnDataType
resolveResultType(DataSchema.ColumnDataType lhsType,
+ DataSchema.ColumnDataType rhsType) {
+ if (lhsType == null) {
+ return rhsType;
+ } else if (rhsType == null) {
+ return lhsType;
} else {
- // TODO: we should correctly throw exception here. Currently exception
thrown during constructor is not
- // piped back to query dispatcher, thus we set it to null and
deliberately make the processing throw exception.
- _resultType = null;
+ if (isSuperType(lhsType, rhsType)) {
+ return lhsType;
+ } else if (isSuperType(rhsType, lhsType)) {
+ return rhsType;
+ } else {
+ return null;
+ }
+ }
+ }
+
+ private static boolean isSuperType(DataSchema.ColumnDataType superType,
DataSchema.ColumnDataType subType) {
Review Comment:
also we should extract this into `ColumnDataType` and add a unit test for it
specifically
--
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]
