-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-30913][SPARK-30841][CORE][SQL][FOLLOWUP] Supplement version information to the configuration of Tests.scala and SQL #28233
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.
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
Conversation
| // on the host. | ||
| val SKIP_VALIDATE_CORES_TESTING = | ||
| ConfigBuilder("spark.testing.skipValidateCores") | ||
| .version("3.1.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SPARK-29154, commit ID: 474b1bb#diff-8b4ea8f3b0cc1e7ce7e943de1abbb165
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? this is an internal testing config, not for users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srowen Thanks for your review. Based on my investigation. In the past, some internal configurations will be changed into public ones.
So it is possible for this configuration to become public in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we don't really need this strictly. It's just a good to know when we make it public later. These versions happened to be marked for all other configurations, and these look the only instances left. Should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
| buildConf("spark.sql.legacy.integerGroupingId") | ||
| .internal() | ||
| .doc("When true, grouping_id() returns int values instead of long values.") | ||
| .version("3.1.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SPARK-30279, commit ID: 71c73d5#diff-9a6b543db706f1a90f790783d6930a13
|
@HyukjinKwon We may tell all the committers to pay attention to the version information when adding configuration when submitting or reviewing. |
|
Test build #121366 has finished for PR 28233 at commit
|
|
retest this please |
|
@beliefer, it's strictly not a must-to-do to specify an added version for internal configurations. We don't really take care about the compatibility in these. It's just a good to know internally, and good to keep it consistent with other configurations. But sure, cc @tgravescs and @maropu FYI. |
|
@HyukjinKwon Thanks. It's true that we don't have to be strict. Let's keep it consistent with other configurations in style. |
|
Test build #121388 has finished for PR 28233 at commit
|
|
Merged to master. |
|
Yea, looks fine to me, too. |
What changes were proposed in this pull request?
I checked all the config of Spark again. find some new commit not add version information.
Test.scala
SQL
The two config only exists in branch master.
Why are the changes needed?
Supplement version information.
Does this PR introduce any user-facing change?
'No'.
How was this patch tested?
Jenkins test.