[SPARK-32755][SQL][FOLLOWUP] Ensure -- method of AttributeSet have same behavior under Scala 2.12 and 2.13
#29689
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LuciferYang
commented
Sep 9, 2020
| // use a Scala 2.12 based code to maintains the insertion order in Scala 2.13 | ||
| case otherSet: AttributeSet => | ||
| new AttributeSet(baseSet -- otherSet.baseSet) | ||
| new AttributeSet(baseSet.clone() --= otherSet.baseSet) |
Contributor
Author
There was a problem hiding this comment.
In Scala 2.12 the implementation of -- is
override def --(xs: GenTraversableOnce[A]): This = clone() --= xs.seq
Contributor
Author
There was a problem hiding this comment.
diff method is defined as Computes the difference of this set and another set, I haven't tested overall behavior of diff, but I think the current changes don't look complicated either
Contributor
There was a problem hiding this comment.
diff returns a set, so it can potentially mess up the ordering.
Contributor
Author
There was a problem hiding this comment.
@hvanhovell Actually, diff returns LinkedHashSet[AttributeEquals] here...
Contributor
Author
There was a problem hiding this comment.
In Scala 2.13
def diff(that: collection.Set[A]): C =
toIterable.foldLeft(empty)((result, elem) => if (that contains elem) result else result += elem)
In Scala 2.12
def diff(that: GenSet[A]): This = this -- that
It looks like they're maintains ordered, but the input parameter needs to be a Set, so
other.map(a => new AttributeEquals(a.toAttribute))
part need call toSet。。。
Contributor
There was a problem hiding this comment.
Ok, then diff looks better.
Contributor
There was a problem hiding this comment.
If we can avoid the conversion it would be nice. The current thing would be fine in that case.
Contributor
Author
There was a problem hiding this comment.
This conversion seems inevitable if use diff :(
Contributor
There was a problem hiding this comment.
Let's use the current method then :).
Contributor
Author
|
cc @cloud-fan @hvanhovell @dbaliafroozeh to help review this ~ |
|
Test build #128436 has finished for PR 29689 at commit
|
Member
|
Jenkins retest this please |
Contributor
|
github action passed, merging to master, thanks! |
Contributor
Author
|
Test build #128457 has finished for PR 29689 at commit
|
srowen
pushed a commit
that referenced
this pull request
Sep 18, 2020
### What changes were proposed in this pull request? After #29660 and #29689 there are 13 remaining failed cases of sql core module with Scala 2.13. The reason for the remaining failed cases is the optimization result of `CostBasedJoinReorder` maybe different with same input in Scala 2.12 and Scala 2.13 if there are more than one same cost candidate plans. In this pr give a way to make the optimization result deterministic as much as possible to pass all remaining failed cases of `sql/core` module in Scala 2.13, the main change of this pr as follow: - Change to use `LinkedHashMap` instead of `Map` to store `foundPlans` in `JoinReorderDP.search` method to ensure same iteration order with same insert order because iteration order of `Map` behave differently under Scala 2.12 and 2.13 - Fixed `StarJoinCostBasedReorderSuite` affected by the above change - Regenerate golden files affected by the above change. ### Why are the changes needed? We need to support a Scala 2.13 build. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? - Scala 2.12: Pass the Jenkins or GitHub Action - Scala 2.13: All tests passed. Do the following: ``` dev/change-scala-version.sh 2.13 mvn clean install -DskipTests -pl sql/core -Pscala-2.13 -am mvn test -pl sql/core -Pscala-2.13 ``` **Before** ``` Tests: succeeded 8485, failed 13, canceled 1, ignored 52, pending 0 *** 13 TESTS FAILED *** ``` **After** ``` Tests: succeeded 8498, failed 0, canceled 1, ignored 52, pending 0 All tests passed. ``` Closes #29711 from LuciferYang/SPARK-32808-3. Authored-by: yangjie01 <[email protected]> Signed-off-by: Sean Owen <[email protected]>
a0x8o
added a commit
to a0x8o/spark
that referenced
this pull request
Sep 18, 2020
### What changes were proposed in this pull request? After apache/spark#29660 and apache/spark#29689 there are 13 remaining failed cases of sql core module with Scala 2.13. The reason for the remaining failed cases is the optimization result of `CostBasedJoinReorder` maybe different with same input in Scala 2.12 and Scala 2.13 if there are more than one same cost candidate plans. In this pr give a way to make the optimization result deterministic as much as possible to pass all remaining failed cases of `sql/core` module in Scala 2.13, the main change of this pr as follow: - Change to use `LinkedHashMap` instead of `Map` to store `foundPlans` in `JoinReorderDP.search` method to ensure same iteration order with same insert order because iteration order of `Map` behave differently under Scala 2.12 and 2.13 - Fixed `StarJoinCostBasedReorderSuite` affected by the above change - Regenerate golden files affected by the above change. ### Why are the changes needed? We need to support a Scala 2.13 build. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? - Scala 2.12: Pass the Jenkins or GitHub Action - Scala 2.13: All tests passed. Do the following: ``` dev/change-scala-version.sh 2.13 mvn clean install -DskipTests -pl sql/core -Pscala-2.13 -am mvn test -pl sql/core -Pscala-2.13 ``` **Before** ``` Tests: succeeded 8485, failed 13, canceled 1, ignored 52, pending 0 *** 13 TESTS FAILED *** ``` **After** ``` Tests: succeeded 8498, failed 0, canceled 1, ignored 52, pending 0 All tests passed. ``` Closes #29711 from LuciferYang/SPARK-32808-3. Authored-by: yangjie01 <[email protected]> Signed-off-by: Sean Owen <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
--method ofAttributeSetbehave differently under Scala 2.12 and 2.13 because--method ofLinkedHashSetin Scala 2.13 can't maintains the insertion order.This pr use a Scala 2.12 based code to ensure
--method of AttributeSet have same behavior under Scala 2.12 and 2.13.Why are the changes needed?
The behavior of
AttributeSetneeds to be compatible with Scala 2.12 and 2.13Does this PR introduce any user-facing change?
No
How was this patch tested?
Scala 2.12: Pass the Jenkins or GitHub Action
Scala 2.13: Manual test sub-suites of
PlanStabilitySuiteBefore ：293 TESTS FAILED
After：13 TESTS FAILED(The remaining failures are not associated with the current issue)