Skip to content

Conversation

@LuciferYang
Copy link
Contributor

@LuciferYang LuciferYang commented Jan 27, 2023

What changes were proposed in this pull request?

The main changes of this pr as follows:

  • Remove explicit import scala.collection.Map and use the default immutable.Map in util.JsonProtocolSuite, and also clean up the redundant toMap conversion after remove explicit import scala.collection.Map

  • Fix two assert conditions: change assert(e1.x === e1.x) to assert(e1.x === e2.x), the originally assert is always true.

Why are the changes needed?

  • Use immutable.Map by default in util.JsonProtocolSuite and remove redundant toMap conversion for code cleanup
  • fix two wrong assert conditions in util.JsonProtocolSuite

Does this PR introduce any user-facing change?

No, just for test

How was this patch tested?

  • Pass GitHub Actions
  • Manual test with Scala 2.13
gh pr checkout 39772
dev/change-scala-version.sh 2.13 
mvn clean install -pl core -am -Pscala-2.13 -Dtest=none -DwildcardSuites=org.apache.spark.util.JsonProtocolSuite
Run completed in 4 seconds, 319 milliseconds.
Total number of tests run: 34
Suites: completed 2, aborted 0
Tests: succeeded 34, failed 0, canceled 0, ignored 0, pending 0
All tests passed.

all test passed

@github-actions github-actions bot added the CORE label Jan 27, 2023
assertEquals(e1.executorInfo, e2.executorInfo)
case (e1: SparkListenerExecutorRemoved, e2: SparkListenerExecutorRemoved) =>
assert(e1.executorId === e1.executorId)
assert(e1.executorId === e2.executorId)
Copy link
Member

Choose a reason for hiding this comment

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

According to the PR description, after fixing this, toMap causes failure, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, two things have been done in this pr : one is code cleanup, the other is bug fix.

Is it better to divide this pr into two parts?

Sorry, my descriptive ability is poor. It seems that there is a misunderstanding ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the PR description, after fixing this, toMap causes failure, right?

After fixing this, toMap is redundant collection conversion

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Since this is related to Map, please test Scala 2.13 together.

@LuciferYang
Copy link
Contributor Author

Since this is related to Map, please test Scala 2.13 together.

done

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM (Pending CIs)

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-42216][CORE][TESTS] Use immutable.Map by default and fix two check conditions in util.JsonProtocolSuite [SPARK-42216][CORE][TESTS] Fix two check conditions and remove redundant toMap in util.JsonProtocolSuite Jan 28, 2023
@LuciferYang
Copy link
Contributor Author

Thanks @dongjoon-hyun

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants