Skip to content

Conversation

@huaxingao
Copy link
Contributor

What changes were proposed in this pull request?

I found a few unnecessary MiMa excludes when auditing SQL binary incompatible changes.

Why are the changes needed?

These MiMa excludes are not required any more, so remove.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Manually tested

ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.ml.regression.RandomForestRegressionModel.setMinInstancesPerNode"),
ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.ml.regression.RandomForestRegressionModel.setNumTrees"),

// [SPARK-26124] Update plugins, including MiMa
Copy link
Member

Choose a reason for hiding this comment

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

@huaxingao, can you point out why this is not needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these are not needed any more because of the following was added in #23086

    // Data Source V2 API changes
    (problem: Problem) => problem match {
      case MissingClassProblem(cls) =>
        !cls.fullName.startsWith("org.apache.spark.sql.sources.v2")
      case MissingTypesProblem(newCls, _) =>
        !newCls.fullName.startsWith("org.apache.spark.sql.sources.v2")
      case InheritedNewAbstractMethodProblem(cls, _) =>
        !cls.fullName.startsWith("org.apache.spark.sql.sources.v2")
      case DirectMissingMethodProblem(meth) =>
        !meth.owner.fullName.startsWith("org.apache.spark.sql.sources.v2")
      case ReversedMissingMethodProblem(meth) =>
        !meth.owner.fullName.startsWith("org.apache.spark.sql.sources.v2")
      case _ => true
    }

Then, because of #25700

org.apache.spark.sql.sources.v2.reader -> org.apache.spark.sql.connector.read

I think we can actually change the above code to the following

    (problem: Problem) => problem match {
      case MissingClassProblem(cls) =>
        !cls.fullName.startsWith("org.apache.spark.sql.sources.v2")
      case _ => true

@SparkQA
Copy link

SparkQA commented Feb 28, 2020

Test build #119058 has finished for PR 27729 at commit 35af07d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@huaxingao
Copy link
Contributor Author

also cc @gatorsmile @cloud-fan

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

Okay, LGTM

@HyukjinKwon
Copy link
Member

Merged to master and branch-3.0.

HyukjinKwon pushed a commit that referenced this pull request Feb 28, 2020
### What changes were proposed in this pull request?
I found a few unnecessary MiMa excludes when auditing SQL binary incompatible changes.

### Why are the changes needed?
These MiMa excludes are not required any more, so remove.

### Does this PR introduce any user-facing change?
No

### How was this patch tested?
Manually tested

Closes #27729 from huaxingao/mima.

Authored-by: Huaxin Gao <[email protected]>
Signed-off-by: HyukjinKwon <[email protected]>
(cherry picked from commit c0d4cc3)
Signed-off-by: HyukjinKwon <[email protected]>
@huaxingao
Copy link
Contributor Author

@HyukjinKwon
You are too fast :)
I am actually thinking of making one more change for this PR. I will change

    // Data Source V2 API changes
    (problem: Problem) => problem match {
      case MissingClassProblem(cls) =>
        !cls.fullName.startsWith("org.apache.spark.sql.sources.v2")
      case MissingTypesProblem(newCls, _) =>
        !newCls.fullName.startsWith("org.apache.spark.sql.sources.v2")
      case InheritedNewAbstractMethodProblem(cls, _) =>
        !cls.fullName.startsWith("org.apache.spark.sql.sources.v2")
      case DirectMissingMethodProblem(meth) =>
        !meth.owner.fullName.startsWith("org.apache.spark.sql.sources.v2")
      case ReversedMissingMethodProblem(meth) =>
        !meth.owner.fullName.startsWith("org.apache.spark.sql.sources.v2")
      case _ => true
    }

to

    // Data Source V2 API changes
    (problem: Problem) => problem match {
      case MissingClassProblem(cls) =>
        !cls.fullName.startsWith("org.apache.spark.sql.sources.v2")
      case _ => true

because of #25700
I guess I will have a follow-up?

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Feb 28, 2020

Sure, I think it might be best to make it as a followup of SPARK-28998.

This PR alone can be better separate anyway because these Mima exclusions in this PR were added as of SPARK-26124.

@huaxingao huaxingao deleted the mima branch February 28, 2020 06:40
sjincho pushed a commit to sjincho/spark that referenced this pull request Apr 15, 2020
### What changes were proposed in this pull request?
I found a few unnecessary MiMa excludes when auditing SQL binary incompatible changes.

### Why are the changes needed?
These MiMa excludes are not required any more, so remove.

### Does this PR introduce any user-facing change?
No

### How was this patch tested?
Manually tested

Closes apache#27729 from huaxingao/mima.

Authored-by: Huaxin Gao <[email protected]>
Signed-off-by: HyukjinKwon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants