zabetak commented on code in PR #6352:
URL: https://github.com/apache/hive/pull/6352#discussion_r2930688438
##########
ql/src/test/queries/clientpositive/kryo_expr_children.q:
##########
@@ -0,0 +1,9 @@
+set hive.cbo.enable=false;
+
+CREATE TABLE tab(attr varchar(5));
+
+-- test case for HIVE-29488
+select * from tab t1 left join tab t2
+on t1.attr = t2.attr and t2.attr in ( trim(t1.attr), '*');
+
+DROP TABLE tab;
Review Comment:
DROP is not needed cause it is taken care by the test framework.
##########
ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeGenericFuncDesc.java:
##########
@@ -94,7 +94,7 @@ public ExprNodeGenericFuncDesc(ObjectInspector oi, GenericUDF
genericUDF,
ObjectInspectorUtils.getWritableObjectInspector(oi);
assert (genericUDF != null);
this.genericUDF = genericUDF;
- this.children = children;
+ this.children = children == null ? new ArrayList<>() : new
ArrayList<>(children);
Review Comment:
Another approach would be to check if `children == null` and throw NPE or
`IllegalArgumentException`. Anyways, I think that we don't have any such calls
currently at the code so choosing between exception, `null`, or `new
ArrayList<>()` is rather subtle details.
##########
ql/src/test/queries/clientpositive/kryo_expr_children.q:
##########
@@ -0,0 +1,9 @@
+set hive.cbo.enable=false;
+
+CREATE TABLE tab(attr varchar(5));
+
+-- test case for HIVE-29488
Review Comment:
Since the actual problem is in plan serialization/deserialization should we
add a unit test in `TestSerializationUtilities`?
##########
ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeGenericFuncDesc.java:
##########
@@ -75,6 +75,12 @@ public class ExprNodeGenericFuncDesc extends ExprNodeDesc
implements
public ExprNodeGenericFuncDesc() {;
}
+ /**
+ * Constructor.
+ *
+ * @param children the children; a copy is made, so later changes to the
passed list
+ * do not affect the children of this instance
+ */
Review Comment:
The doc seems a bit repetitive. We could possibly just put the mention about
copy once over the field declaration:
```java
private List<ExprNodeDesc> children;
```
--
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]
