HIVE-29488: KryoException: NullPointerException: Cannot invoke "java.util.Collection.isEmpty()" because "this.delegate" is null by thomasrebele · Pull Request #6352 · apache/hive

thomasrebele · 2026-03-06T16:05:58Z

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 · 2026-03-06T17:10:33Z

ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeGenericFuncDesc.java

    assert (genericUDF != null);
    this.genericUDF = genericUDF;
-    this.children = children;
+    this.children = children == null ? new ArrayList<>() : new ArrayList<>(children);


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

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

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().

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

@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 ?

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.

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.

Indeed, though I'll leave the null-check for another PR.

…util.Collection.isEmpty()" because "this.delegate" is null Based on a fix by Naresh Panchetty Ramanaiah.

thomasrebele · 2026-03-09T11:42:10Z

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.

deniskuzZ · 2026-03-10T23:10:57Z

ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeGenericFuncDesc.java

    assert (genericUDF != null);
    this.genericUDF = genericUDF;
-    this.children = children;
+    this.children = children == null ? new ArrayList<>() : new ArrayList<>(children);


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

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().

I've created HIVE-29505 to make the children immutable. So I propose to postpone using List.of() until HIVE-29505 has been implemented.

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

zabetak · 2026-03-13T11:24:49Z

ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeGenericFuncDesc.java

+  /**
+   * Constructor.
+   *
+   * @param children the children; a copy is made, so later changes to the passed list
+   *                 do not affect the children of this instance
+   */


The doc seems a bit repetitive. We could possibly just put the mention about copy once over the field declaration:

private List<ExprNodeDesc> children;

I would prefer to to see such information in the IDE when hovering over the constructor.

zabetak · 2026-03-13T11:44:34Z

ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeGenericFuncDesc.java

    assert (genericUDF != null);
    this.genericUDF = genericUDF;
-    this.children = children;
+    this.children = children == null ? new ArrayList<>() : new ArrayList<>(children);


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.

zabetak · 2026-03-13T11:45:13Z

ql/src/test/queries/clientpositive/kryo_expr_children.q

+select * from tab t1 left join tab t2
+on t1.attr = t2.attr and t2.attr in ( trim(t1.attr), '*');
+
+DROP TABLE tab;


DROP is not needed cause it is taken care by the test framework.

I added it after seeing such a DROP TABLE statement in another q file. I'll drop it (the statement).

zabetak · 2026-03-13T12:07:18Z

ql/src/test/queries/clientpositive/kryo_expr_children.q

+
+CREATE TABLE tab(attr varchar(5));
+
+-- test case for HIVE-29488


Since the actual problem is in plan serialization/deserialization should we add a unit test in TestSerializationUtilities?

Indeed, that sounds like a good idea. I've added a test there and removed the q file test.

sonarqubecloud · 2026-03-13T21:32:37Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

asf-ci-hive added the tests pending label Mar 6, 2026

ayushtkn reviewed Mar 6, 2026

View reviewed changes

asf-ci-hive added tests unstable and removed tests pending labels Mar 6, 2026

HIVE-29488: KryoException: NullPointerException: Cannot invoke "java.…

513255d

…util.Collection.isEmpty()" because "this.delegate" is null Based on a fix by Naresh Panchetty Ramanaiah.

thomasrebele force-pushed the tr/HIVE-29488 branch from 3b152b0 to 513255d Compare March 9, 2026 11:35

asf-ci-hive added tests pending and removed tests unstable labels Mar 9, 2026

asf-ci-hive added tests passed and removed tests pending labels Mar 9, 2026

deniskuzZ reviewed Mar 10, 2026

View reviewed changes

zabetak approved these changes Mar 13, 2026

View reviewed changes

Add test case in TestSerializationUtilities

c3d1f2f

asf-ci-hive added tests pending and removed tests passed labels Mar 13, 2026

Remove the q file

802e8d3

asf-ci-hive added tests failed and removed tests pending labels Mar 13, 2026

Fix imports

66ca175

asf-ci-hive added tests pending and removed tests failed labels Mar 13, 2026

asf-ci-hive added tests unstable and removed tests pending labels Mar 13, 2026


		CREATE TABLE tab(attr varchar(5));

		-- test case for HIVE-29488

Conversation

thomasrebele commented Mar 6, 2026

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nareshpr Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasrebele commented Mar 9, 2026

Uh oh!

deniskuzZ Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zabetak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Mar 13, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nareshpr Mar 6, 2026 •

edited

Loading

deniskuzZ Mar 10, 2026 •

edited

Loading