Skip to content

[SPARK-52226] [SQL] Fix unusual equality checks in three operators #50949

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+4 −4
Open

[SPARK-52226] [SQL] Fix unusual equality checks in three operators #50949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/BatchScanExec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ case class BatchScanExec(
// TODO: unify the equal/hashCode implementation for all data source v2 query plans.
override def equals(other: Any): Boolean = other match {
case other: BatchScanExec =>
this.batch != null && this.batch == other.batch &&
this.runtimeFilters == other.runtimeFilters &&
this.getClass == other.getClass && this.batch != null &&
Copy link
Author

@li-boxuan li-boxuan May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Even though this is a case class, there's no final modifier, making it still possible to be extended. It seems to me that we should either mark this class as final, or explicitly check class equivalence here to prevent misuse (like this PR), or remove this override completely, so that Scala compiler would automatically generate an equals method that does proper class equivalence check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually Scala case class should not be extended, so this class check is not needed.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the reply.

Usually Scala case class should not be extended

I agree, with the caveat that Scala compiler doesn't prohibit a normal class from inheriting a case class. But anyways, it would be such a corner case that we could close this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Usually Scala case class should not be extended, so this class check is not needed.

@cloud-fan Perhaps we can set a rule in the Maven configuration to forbid case-class inheritance? With the help of a wartremover wart which was authored by me and recently added to the plugin.

In Gluten we used to occasionally receive PRs that unintentionally violated the rule. The violations may be missed out in code review easily.

this.batch == other.batch && this.runtimeFilters == other.runtimeFilters &&
this.spjParams == other.spjParams
case _ =>
false
Expand Down
2 changes: 1 addition & 1 deletion ...ore/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ContinuousScanExec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ case class ContinuousScanExec(

// TODO: unify the equal/hashCode implementation for all data source v2 query plans.
override def equals(other: Any): Boolean = other match {
case other: ContinuousScanExec => this.stream == other.stream
case other: ContinuousScanExec => this.getClass == other.getClass && this.stream == other.stream
case _ => false
}

Expand Down
2 changes: 1 addition & 1 deletion ...ore/src/main/scala/org/apache/spark/sql/execution/datasources/v2/MicroBatchScanExec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ case class MicroBatchScanExec(

// TODO: unify the equal/hashCode implementation for all data source v2 query plans.
override def equals(other: Any): Boolean = other match {
case other: MicroBatchScanExec => this.stream == other.stream
case other: MicroBatchScanExec => this.getClass == other.getClass && this.stream == other.stream
case _ => false
}

Expand Down