Skip to content

Comments

[SPARK-43230][CONNECT] Simplify DataFrameNaFunctions.fillna#40898

Closed
zhengruifeng wants to merge 2 commits intoapache:masterfrom
zhengruifeng:connect_simplify_fillna
Closed

[SPARK-43230][CONNECT] Simplify DataFrameNaFunctions.fillna#40898
zhengruifeng wants to merge 2 commits intoapache:masterfrom
zhengruifeng:connect_simplify_fillna

Conversation

@zhengruifeng
Copy link
Contributor

What changes were proposed in this pull request?

add a helper function in DataFrameNaFunctions

Why are the changes needed?

to Simplify DataFrameNaFunctions.fillna

Does this PR introduce any user-facing change?

no, dev-only

How was this patch tested?

existing UTs

Copy link
Contributor

@amaliujia amaliujia left a comment

Choose a reason for hiding this comment

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

LGTM

@zhengruifeng zhengruifeng force-pushed the connect_simplify_fillna branch from a3b7db4 to 16a7e44 Compare April 24, 2023 01:05
@zhengruifeng
Copy link
Contributor Author

ERROR: Comparing client jar: /__w/spark/spark/connector/connect/client/jvm/target/scala-2.12/spark-connect-client-jvm-assembly-3.5.0-SNAPSHOT.jar and and sql jar: /__w/spark/spark/sql/core/target/scala-2.12/spark-sql_2.12-3.5.0-SNAPSHOT.jar 
problems: 
method fillValue(java.lang.Object,scala.Option)org.apache.spark.sql.Dataset in class org.apache.spark.sql.DataFrameNaFunctions does not have a correspondent in client version
Exceptions to binary compatibility can be added in 'CheckConnectJvmClientCompatibility#checkMiMaCompatibility'
connect-client-jvm module mima check failed.
Error: Process completed with exit code 1.

seems Mima also check private[sql]


// DataFrameNaFunctions
ProblemFilters.exclude[Problem]("org.apache.spark.sql.DataFrameNaFunctions.this"),
ProblemFilters.exclude[Problem]("org.apache.spark.sql.DataFrameNaFunctions.fillValue"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LuciferYang it seems the MiMa tests also check private[sql], is this as expected?

Copy link
Contributor

Choose a reason for hiding this comment

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

This behavior is expected now, but I think some refactoring work is needed to automatically filter similar checks, just like what dev/mima check does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@LuciferYang LuciferYang Apr 24, 2023

Choose a reason for hiding this comment

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

Also cc @zhenlineo, @hvanhovell do you have any good suggestions about this

Copy link
Contributor

Choose a reason for hiding this comment

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

#40925 try to solve this issue, but it will make CheckConnectJvmClientCompatibility spend more time and maybe more complex

@zhengruifeng
Copy link
Contributor Author

Thanks for the reviews, merged to master

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants