Skip to content

[SPARK-59687][SQL] Use exists instead of find(...).isDefined/isEmpty in catalyst and SQL core - #58941

Open
uros-b wants to merge 2 commits into
apache:masterfrom
uros-b:windowexpr-find-to-exists
Open

uros-b wants to merge 2 commits into
apache:masterfrom
uros-b:windowexpr-find-to-exists

Conversation

@uros-b

@uros-b uros-b commented Sep 21, 2026

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Several places in catalyst and SQL core test for a matching node/element with find(pred) followed by an Option presence check. On TreeNode (and Scala collections) exists(pred) is the equivalent, returning the Boolean directly. This replaces the pattern in four spots:

  • WindowExpression.hasWindowExpression: e.find(_.isInstanceOf[WindowExpression]).isDefined -> e.exists(...)
  • AliasHelper.getAliasMap: a.child.find(_.isInstanceOf[AggregateExpression]).isEmpty -> !a.child.exists(...)
  • Analyzer.ResolveSQLFunctions.hasSQLFunctionExpression: _.find(_.isInstanceOf[SQLFunctionExpression]).nonEmpty -> _.exists(...)
  • V2TableRefreshUtil.containsCommand: plan.find(_.isInstanceOf[Command]).isDefined -> plan.exists(...)

Why are the changes needed?

find(pred).isDefined / find(pred).nonEmpty are equivalent to exists(pred), and find(pred).isEmpty to !exists(pred) -- the same short-circuiting traversal, but find allocates an Option for the first match only for it to be discarded by the presence check. exists returns the Boolean directly and reads more clearly. Behavior is unchanged.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests. This is a behavior-preserving refactor; the catalyst and sql (core) modules compile cleanly.

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

Generated-by: Claude Code (Opus 4.8)

@uros-b uros-b changed the title [SPARK-59687][SQL] Use exists instead of find(...).isDefined in WindowExpression.hasWindowExpression [SPARK-59687][SQL] Use exists instead of find(...).isDefined/isEmpty in catalyst and SQL core Sep 21, 2026

@uros-b uros-b left a comment

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.

This should be good to go, I don't think there are any additional occurrences in sql catalyst / core

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.

2 participants