HIVE-29488: KryoException: NullPointerException: Cannot invoke "java.util.Collection.isEmpty()" because "this.delegate" is null

[thomasrebele]

See HIVE-29488.

Thank you @nareshpr for providing an initial version of the q file test and a first version of the fix!

What changes were proposed in this pull request?

Put the children of ExprNodeGenericFuncDesc in their own list object.

Why are the changes needed?

Fixes an NPE due to the Kryo library when CBO is disabled.

Does this PR introduce any user-facing change?

No

How was this patch tested?

A q file test was added.

[ayushtkn]

why you need a new ArrayList<>(children) here? why it can't be just

    this.children = children == null ? List.of() : children;

[thomasrebele]

The new ArrayList<>(children) is required, because otherwise the NPE occurs. I've seen that some callers of getChildren modify the list, e.g., DynamicPartitionPruningOptimization, so I've I opted for new ArrayList<>() instead of List.of().

[nareshpr]

@ayushtkn if we don't explicitly convert it to ArrayList, kryo cannot determine the actual runtime List object for ExprNodeGenericFuncDesc.children and uses AbstractMapBasedMultimap$WrappedCollection which is throwing NPE at deserializer in Tez Task.

Explicit cast ensure kryo knows its ArrayList and won't use AbstractMapBasedMultimap$WrappedCollection avoiding this NPE.

[nareshpr]

@thomasrebele I suspect its more of Kryo-Guava deseralizer issue when children object is not null. Do you think we need to convert null to empty ?

[thomasrebele]

It is safer to avoid null for children, as there are several places without null check, e.g., in getExprString. The children are exposed to other classes by getChildren(), so it's simpler to just use an empty list instead of adding null checks everywhere.

[thomasrebele]

The test TestVectorizationContext had failed, because it changed the children list after creating the ExprNodeGenericFuncDesc. I checked the code, and this modification-after-instantiation seems to be limited to the test class. There are a few candidates that in principle could modify the list, but I don't think that happens in the code:

• VectorizationContext#getWhenExpression: passes a sublist, which in principle could be modifiable. It seems it is only used for transforming the ExprNode to a VectorExpression
• ExprNodeDescExprFactory#replaceFieldNamesInStruct passes the children of another ExprNodeGenericFuncDesc. The caller seems to transform the original expr node into a new one; I think the original expr will not be used afterwards
• StatsRulesProcFactory.JoinStatsRule#process passes some object from JoinDesc#getResidualFilterExprs. AFAIK, the class StatsRulesProcFactory just visits but does not modify the expr nodes

I therefore propose to change TestVectorizationContext so that it takes into account that ExprNodeGenericFuncDesc makes a copy of the children list.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[deniskuzZ]

@thomasrebele, why not use jdk21 List.of() instead?

[thomasrebele]

I've seen that some callers of getChildren modify the list, e.g., DynamicPartitionPruningOptimization, so I've I opted for new ArrayList<>() instead of List.of().