Skip to content

[SPARK-56840][SQL][FOLLOW-UP] Add a real NullIf repro test#55883

Open
sunchao wants to merge 4 commits into
apache:masterfrom
sunchao:dev/chao/codex/nullif-real-repro
Open

[SPARK-56840][SQL][FOLLOW-UP] Add a real NullIf repro test#55883
sunchao wants to merge 4 commits into
apache:masterfrom
sunchao:dev/chao/codex/nullif-real-repro

Conversation

@sunchao
Copy link
Copy Markdown
Member

@sunchao sunchao commented May 14, 2026

What changes were proposed in this pull request?

Add a focused Catalyst regression test that constructs builtin nullif with an unresolved nested field reference while ALWAYS_INLINE_COMMON_EXPR=true. This reproduces the eager left.dataType access that motivated SPARK-56840 and guards the fixed construction path directly.

Why are the changes needed?

The original SPARK-56840 fix was merged with an end-to-end repro that also passes without the fix, so it does not prove the bug boundary. This follow-up adds a real negative/positive regression test that fails before commit 5949ab30b41 with Invalid call to dataType on unresolved object and passes with the fix.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

  • build/sbt 'catalyst/testOnly org.apache.spark.sql.catalyst.expressions.NullExpressionsSuite -- -z "NullIf accepts unresolved nested fields during inlined function construction"'
  • Verified the same focused test fails on pre-fix parent 9ab8e43a940 with UnresolvedException: Invalid call to dataType on unresolved object and passes on current apache/master.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Codex

}
}

test("NullIf accepts unresolved nested fields during inlined function construction") {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we add a test prefix, SPARK-56840: ?

Copy link
Copy Markdown
Contributor

@peter-toth peter-toth May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the ticket prefixes were missing from https://github.com/apache/spark/pull/55838/changes too, but we can add them in this follow-up.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add it to the 2 new tests of https://github.com/apache/spark/pull/55838/changes as well?

@dongjoon-hyun
Copy link
Copy Markdown
Member

cc @peter-toth

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants