You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/11/18 00:40:36 UTC
[GitHub] [pinot] agavra commented on a diff in pull request #9829: [multistage][hotfix] fix filter type hoisting
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org