[SPARK-50113][CONNECT][PYTHON][TESTS] Add @spark_connect_only to check the APIs that only supported with Spark Connect
#48651
Conversation
…ect ONLY_SUPPORTED_WITH_SPARK_CONNECT
| expected_missing_connect_methods = { | ||
| "addArtifact", | ||
| "addArtifacts", | ||
| "addTag", | ||
| "clearProgressHandlers", | ||
| "clearTags", | ||
| "copyFromLocalToFs", | ||
| "getTags", | ||
| "interruptAll", | ||
| "interruptOperation", | ||
| "interruptTag", | ||
| "newSession", | ||
| "registerProgressHandler", | ||
| "removeProgressHandler", | ||
| "removeTag", | ||
| } |
There was a problem hiding this comment.
Now the compatibility check respect the ONLY_SUPPORTED_WITH_SPARK_CONNECT.
cc @HyukjinKwon @hvanhovell FYI
| inspect.isfunction(method) or isinstance(method, functools._lru_cache_wrapper) | ||
| ) and not name.startswith("_"): | ||
| source_lines = inspect.getsource(method).upper() | ||
| if "ONLY_SUPPORTED_WITH_SPARK_CONNECT" in source_lines: |
There was a problem hiding this comment.
Let's probably don't do this for now.
There was a problem hiding this comment.
Agree that it's flaky but I believe this is currently the only way to check functions that only supported from connect.
Should we just close it? Also cc @hvanhovell
There was a problem hiding this comment.
I don't think we should rely on reading/checking the code itself in the test. This isn't what we meant for signature comparison / compatibility test.
There was a problem hiding this comment.
How about using a new decorator @SparkConnectOnly which doesn't suffer from flakiness? So instead of having a list in test_spark_session_compatibility we now spread the items to their declaration sites.
That would make people more aware of adding such a decorator when needed.
There was a problem hiding this comment.
How about using a new decorator @SparkConnectOnly
It sounds like a reasonable suggestion.
Let me create a separate PR and will ping you guys when it's ready.
There was a problem hiding this comment.
@xupefei just applied the comments. Could you review again when you find some time? Thanks!
Also cc @HyukjinKwon now we apply the @spark_connect_only decorator for APIs that are only supported with Spark Connect and use it from the testing instead of string comparison.
itholic
changed the title
ONLY_SUPPORTED_WITH_SPARK_CONNECT@spark_connect_only to check the APIs that only supported with Spark Connect
What changes were proposed in this pull request?
This PR proposes to add
@spark_connect_onlyto check the APIs that only supported with Spark ConnectWhy are the changes needed?
The current compatibility check cannot capture the missing methods that only supported with Spark Connect
Does this PR introduce any user-facing change?
No, it's test-only
How was this patch tested?
Updated the existing UT
Was this patch authored or co-authored using generative AI tooling?
No