Skip to content

Conversation

@HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Jul 27, 2020

What changes were proposed in this pull request?

This PR proposes to set the minimum Arrow version as 1.0.0 to minimise the maintenance overhead and keep the minimal version up to date.

Other required changes to support 1.0.0 were already made in SPARK-32451.

Why are the changes needed?

R side, people rather aggressively encourage people to use the latest version, and SparkR vectorization is very experimental that was added from Spark 3.0.

Also, we're technically not testing old Arrow versions in SparkR for now.

Does this PR introduce any user-facing change?

Yes, users wouldn't be able to use SparkR with old Arrow.

How was this patch tested?

GitHub Actions and AppVeyor are already testing them.

@SparkQA
Copy link

SparkQA commented Jul 27, 2020

Test build #126601 has finished for PR 29253 at commit e2f5bb9.

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

appveyor.yml Outdated
# This environment variable works around to test SparkR against a higher version.
R_REMOTES_NO_ERRORS_FROM_WARNINGS: true
# AppVeyor does not have python3 yet which is used by default.
PYSPARK_PYTHON: python
Copy link
Member Author

Choose a reason for hiding this comment

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

I am adding this change together to fix AppVeyor build.

@SparkQA
Copy link

SparkQA commented Jul 27, 2020

Test build #126604 has finished for PR 29253 at commit 3d2406b.

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

e1071,
survival,
arrow (>= 0.15.1)
arrow (>= 1.0.0)
Copy link
Member

Choose a reason for hiding this comment

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

Oh, nice. Are we going to drop Arrow 0.x officially?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, I think at least it's okay for SparkR for now.

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. It would be great if we can enforce Arrow 1.0 for all languages.

appveyor.yml Outdated
# AppVeyor does not have python3 yet which is used by default.
PYSPARK_PYTHON: python
# TODO(SPARK-32453): Remove SPARK_SCALA_VERSION environment and let load-spark-env scripts detect it.
SPARK_SCALA_VERSION: 2.12
Copy link
Member

Choose a reason for hiding this comment

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

Just a question. Is this enough to fix AppVeyor build?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm testing now.. :). If it needs a more complicated fix, I'll just revert this bit and merge.

@dongjoon-hyun
Copy link
Member

I love to have this patch on master. Unfortunately, AppVeyor seems to be delayed because it's in the queue still. :(

@SparkQA
Copy link

SparkQA commented Jul 27, 2020

Test build #126607 has finished for PR 29253 at commit 23aba83.

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

@HyukjinKwon
Copy link
Member Author

I am going to investigate AppVeyor test failures at HyukjinKwon#13

@HyukjinKwon
Copy link
Member Author

I am merging this since the tests already passed. The last commit just reverted some additional changes.

@HyukjinKwon
Copy link
Member Author

Thank you @viirya and @dongjoon-hyun for reviewing it. Merged to master.

@dongjoon-hyun
Copy link
Member

Thanks!

@HyukjinKwon HyukjinKwon deleted the SPARK-32452 branch July 27, 2020 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants