HIVE-29488: NPE in Kryo deserializer when query plan contains ExprNodeGenericFuncDesc with Guava collections

[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.

[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.

[zabetak]

I like the fix cause it is generic and improves the encapsulation of ExprNodeGenericFuncDesc class. However, at the same time it changes a bit the existing behavior and increases a bit the memory footprint and GC activity. I saw the comment that changes in this class will not affect the overall behavior of Hive so I am OK to merge the PR as is.

As far as I see there aren't many places where we pass in the constructor something different from an ArrayList so another potential fix would be to change the creation of the IN function call in TypeCheckProcFactory by wrapping children into an ArrayList. This is less general but closer to the root cause that led to this issue.

I am OK with any of the above options so the approval remains no matter which we pick. The remaining comments are mostly nits that we don't necessarily have to address (or defer in follow-up).

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[zabetak]

Many thanks to @thomasrebele @nareshpr @deniskuzZ @ayushtkn for pushing this work forward, reviewing and providing valuable feedback!