Skip to content

Conversation

@mgaido91
Copy link
Contributor

What changes were proposed in this pull request?

Saveable interface introduces formatVersion which is protected and it is used nowhere. So the PR proposes to remove it.

How was this patch tested?

existing tests

@SparkQA
Copy link

SparkQA commented Oct 25, 2018

Test build #98039 has finished for PR 22830 at commit 7f17c6e.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 26, 2018

Test build #98082 has finished for PR 22830 at commit 492f6ed.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 26, 2018

Test build #98088 has finished for PR 22830 at commit e9d9e0e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class AvroDataToCatalyst(

@viirya
Copy link
Member

viirya commented Oct 27, 2018

cc @mengxr any idea why we have this formatVersion?

@dongjoon-hyun
Copy link
Member

Retest this please.

}

override protected def formatVersion: String = "1.0"

Copy link
Member

Choose a reason for hiding this comment

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

This seems visible to the users. Although this occurs in Spark 3.0.0, I guess we had better do deprecation processes first for this kind of removals.

cc @srowen , @mengxr , @rxin .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean by visible to users? It is protected, it is visible only if a user extends the class...

@rxin
Copy link
Contributor

rxin commented Oct 29, 2018

Who introduced this? We should ask the person that introduced it whether it can be removed.

@SparkQA
Copy link

SparkQA commented Oct 29, 2018

Test build #98179 has finished for PR 22830 at commit e9d9e0e.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class AvroDataToCatalyst(

@mgaido91
Copy link
Contributor Author

this was introduced by @jkbradley in 975bcef.

@mgaido91
Copy link
Contributor Author

retest this please

@rxin
Copy link
Contributor

rxin commented Oct 29, 2018

Perhaps @jkbradley and @mengxr can comment on it. If the trait is inheritable, then protected still means it is part of the API contract.

@SparkQA
Copy link

SparkQA commented Oct 29, 2018

Test build #98194 has finished for PR 22830 at commit e9d9e0e.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class AvroDataToCatalyst(

@mgaido91
Copy link
Contributor Author

retest this please

@SparkQA
Copy link

SparkQA commented Oct 29, 2018

Test build #98220 has finished for PR 22830 at commit e9d9e0e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class AvroDataToCatalyst(

@srowen
Copy link
Member

srowen commented Mar 3, 2019

Just saw this after reviewing a lot of old PRs. Yes this can be removed. It's old and not used, and think it was superseded by the "SaveLoadVx_0" class pattern. I think it can be removed in Spark 3 as it's not user visible.

@SparkQA
Copy link

SparkQA commented Mar 8, 2019

Test build #103166 has finished for PR 22830 at commit db63b5b.

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

@srowen
Copy link
Member

srowen commented Mar 9, 2019

Merged to master

@srowen srowen closed this in 25bcf59 Mar 9, 2019
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.

6 participants