zabetak commented on code in PR #5577:
URL: https://github.com/apache/hive/pull/5577#discussion_r1904225648
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java:
##########
@@ -1103,6 +1103,9 @@ private VectorExpression
getGenericUDFStructField(ExprNodeFieldDesc exprNodeFiel
private int getStructFieldIndex(ExprNodeFieldDesc exprNodeFieldDesc) throws
HiveException {
ExprNodeDesc structNodeDesc = exprNodeFieldDesc.getDesc();
String fieldName = exprNodeFieldDesc.getFieldName();
+ if (exprNodeFieldDesc.getIsList()) {
+ throw new HiveException("Could not vectorize expression: Vectorizing
complex type LIST is not supported");
Review Comment:
The `Vectorizing complex type..` message is created by
`getValidateDataTypeErrorMsg` during some validation type checks. Why aren't we
hitting these checks before arriving into this method?
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java:
##########
@@ -1103,6 +1103,9 @@ private VectorExpression
getGenericUDFStructField(ExprNodeFieldDesc exprNodeFiel
private int getStructFieldIndex(ExprNodeFieldDesc exprNodeFieldDesc) throws
HiveException {
ExprNodeDesc structNodeDesc = exprNodeFieldDesc.getDesc();
String fieldName = exprNodeFieldDesc.getFieldName();
+ if (exprNodeFieldDesc.getIsList()) {
+ throw new HiveException("Could not vectorize expression: Vectorizing
complex type LIST is not supported");
+ }
StructTypeInfo structTypeInfo = (StructTypeInfo)
structNodeDesc.getTypeInfo();
Review Comment:
The fact that we are hitting a `ClassCastException` without the extra if
check above is a bit worrisome. It means that the new logic has some impact on
the vectorization codepath. Why are we hitting this now?
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/type/RexNodeExprFactory.java:
##########
@@ -613,6 +617,14 @@ protected RexNode createConstantExpr(TypeInfo typeInfo,
Object constantValue)
TypeConverter.convert(typeInfo, rexBuilder.getTypeFactory()), false);
}
+ /**
+ * Special operator that is used as syntactic sugar to change the type of
collection
+ * expressions in order to perform field access over them.
+ */
+ public static final SqlOperator COMPONENT_ACCESS =
Review Comment:
Most of the existing Hive specific operators/functions are under
`org.apache.hadoop.hive.ql.optimizer.calcite.reloperators` package (e.g.,
`HiveDateAddSqlOperator`). Consider moving the new operator into a separate
class under that package.
Having one class per function is not ideal but it may be a good idea to
follow the existing paradigm at least till we decide to perform a holistic
refactoring that puts them all together.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/type/RexNodeExprFactory.java:
##########
@@ -622,11 +634,13 @@ protected RexNode createNestedColumnRefExpr(
if (expr.getType().isStruct()) {
// regular case of accessing nested field in a column
return rexBuilder.makeFieldAccess(expr, fieldName, true);
+ } else if (expr.getType().getComponentType() != null) {
+ RexNode wrap =
+ rexBuilder.makeCall(expr.getType().getComponentType(),
COMPONENT_ACCESS, Collections.singletonList(expr));
+ return createNestedColumnRefExpr(typeInfo, wrap, fieldName, isList);
} else {
- // This may happen for schema-less tables, where columns are dynamically
- // supplied by serdes.
- throw new CalciteSemanticException("Unexpected rexnode : "
- + expr.getClass().getCanonicalName(),
UnsupportedFeature.Schema_less_table);
+ // Safe exception. Shouldn't Ideally come here.
Review Comment:
The comment does not provide much info; consider dropping it.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/type/RexNodeExprFactory.java:
##########
@@ -622,11 +634,13 @@ protected RexNode createNestedColumnRefExpr(
if (expr.getType().isStruct()) {
// regular case of accessing nested field in a column
return rexBuilder.makeFieldAccess(expr, fieldName, true);
+ } else if (expr.getType().getComponentType() != null) {
+ RexNode wrap =
+ rexBuilder.makeCall(expr.getType().getComponentType(),
COMPONENT_ACCESS, Collections.singletonList(expr));
+ return createNestedColumnRefExpr(typeInfo, wrap, fieldName, isList);
} else {
- // This may happen for schema-less tables, where columns are dynamically
- // supplied by serdes.
- throw new CalciteSemanticException("Unexpected rexnode : "
- + expr.getClass().getCanonicalName(),
UnsupportedFeature.Schema_less_table);
Review Comment:
Can we remove the `UnsupportedFeature.Schema_less_table` from the
enumeration.
--
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]
