Skip to content

[arrow] Update to 6.0.0#21113

Merged
BillyONeal merged 24 commits intomicrosoft:masterfrom
ianmcook:arrow-6.0.0
Dec 8, 2021
Merged

[arrow] Update to 6.0.0#21113
BillyONeal merged 24 commits intomicrosoft:masterfrom
ianmcook:arrow-6.0.0

Conversation

@ianmcook
Copy link
Copy Markdown
Contributor

@ianmcook ianmcook commented Nov 1, 2021

Updates the arrow port to version 6.0.0

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one portfile where deprecated functions are used.

Details

If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)

In the ports that use the new function, you have to add the corresponding dependencies:

{
  "name": "vcpkg-cmake",
  "host": true
},
{
  "name": "vcpkg-cmake-config",
  "host": true
}

The following files are affected:

  • ports/arrow/portfile.cmake

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one portfile where deprecated functions are used.

Details

If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)

In the ports that use the new function, you have to add the corresponding dependencies:

{
  "name": "vcpkg-cmake",
  "host": true
},
{
  "name": "vcpkg-cmake-config",
  "host": true
}

The following files are affected:

  • ports/arrow/portfile.cmake

@JonLiu1993 JonLiu1993 self-assigned this Nov 2, 2021
@JonLiu1993 JonLiu1993 added the category:port-update The issue is with a library, which is requesting update new revision label Nov 2, 2021
@JonLiu1993
Copy link
Copy Markdown
Contributor

@ianmcook ,Could you please take a look:
https://dev.azure.com/vcpkg/public/_build/results?buildId=62270&view=artifacts&pathAsName=false&type=publishedArtifacts

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one portfile where deprecated functions are used.

Details

If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)

In the ports that use the new function, you have to add the corresponding dependencies:

{
  "name": "vcpkg-cmake",
  "host": true
},
{
  "name": "vcpkg-cmake-config",
  "host": true
}

The following files are affected:

  • ports/arrow/portfile.cmake

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one portfile where deprecated functions are used.

Details

If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)

In the ports that use the new function, you have to add the corresponding dependencies:

{
  "name": "vcpkg-cmake",
  "host": true
},
{
  "name": "vcpkg-cmake-config",
  "host": true
}

The following files are affected:

  • ports/arrow/portfile.cmake

@ianmcook ianmcook marked this pull request as ready for review November 6, 2021 13:18
@ianmcook
Copy link
Copy Markdown
Contributor Author

ianmcook commented Nov 6, 2021

@GPSnoopy here's the Arrow 6.0.0 update PR. Let me know if you encounter any issues when trying this with ParquetSharp.

Comment thread ports/arrow/portfile.cmake Outdated
Comment thread ports/arrow/portfile.cmake Outdated
Comment thread ports/arrow/portfile.cmake Outdated
@JonLiu1993
Copy link
Copy Markdown
Contributor

@ianmcook ,Thanks for your contribution, Are you tested the features locally?

@jgiannuzzi
Copy link
Copy Markdown
Contributor

@GPSnoopy here's the Arrow 6.0.0 update PR. Let me know if you encounter any issues when trying this with ParquetSharp.

Thanks @ianmcook, I'll start an Arrow 6.0.0 branch for ParquetSharp and let you know how it goes!

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 66c39e113a348a6ae80419fae5a629b951b00f1a -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/a-/arrow.json b/versions/a-/arrow.json
index 868bab0..353219b 100644
--- a/versions/a-/arrow.json
+++ b/versions/a-/arrow.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "92c56e8363c93912ff2f3deefa2106301616d4c4",
+      "git-tree": "52cc17e9e181c645ab3c6b52675f64fdbb5a1850",
       "version": "6.0.0",
       "port-version": 0
     },

@microsoft microsoft deleted a comment from JonLiu1993 Nov 12, 2021
@ianmcook
Copy link
Copy Markdown
Contributor Author

Are you tested the features locally?

@JonLiu1993 I installed the following successfully in local environments:

arrow[core,csv,dataset,filesystem,json,mimalloc,orc,parquet,s3]:x64-windows-static
arrow[core,csv,dataset,filesystem,flight,jemalloc,json,mimalloc,orc,parquet,s3]:x64-osx

There are some issues with s3 on x64-windows that I am troubleshooting.

The flight feature is experimental and does not build on Windows.

@ianmcook
Copy link
Copy Markdown
Contributor Author

ianmcook commented Nov 12, 2021

The issue with s3 on x64-windows is caused by these lines in AWSSDKConfig.cmake:
https://github.com/aws/aws-sdk-cpp/blob/2072c7cb48679d870aea3a3f6224306ba5ee66ef/cmake/AWSSDKConfig.cmake#L58-L62
That causes find_library() later in the file to look in the wrong place (bin instead of lib) and throw the error

AWS SDK for C++ headers found, but we were unable to locate the binaries

With those lines removed, the install is successful. But since it's not possible to remove those lines directly, how can we stop them from running when find_package(AWSSDK) is called in ThirdpartyToolchain.cmake? Setting AWSSDK_INSTALL_AS_SHARED_LIBS to OFF there (via a patch) has no effect; it is overridden and ends up being ON in AWSSDKConfig.cmake. The same happens for other variables. Any ideas for how to solve this? @kou please let me know if you have any suggestions, thanks.

@jgiannuzzi
Copy link
Copy Markdown
Contributor

@ianmcook ParquetSharp seems happy with the upgrade to Arrow 6.0.0 so far - here is the draft PR: G-Research/ParquetSharp#230

I had to add /Zc:__cplusplus to make it work on Windows, but this might be due to our heavily-customised cmake setup.

@kou
Copy link
Copy Markdown
Contributor

kou commented Nov 12, 2021

@ianmcook Could you show full build log?

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout a2fcb03749ff5897b5985092934dc6057680c789 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/a-/arrow.json b/versions/a-/arrow.json
index fd5974a..55253ed 100644
--- a/versions/a-/arrow.json
+++ b/versions/a-/arrow.json
@@ -5,6 +5,11 @@
       "version": "6.0.0",
       "port-version": 0
     },
+    {
+      "git-tree": "8a30c7d6553216924aa3a3957ceb87d0fda39592",
+      "version": "5.0.0",
+      "port-version": 2
+    },
     {
       "git-tree": "79938475d53bb40ad7bf8d0fbda9e65f7630dde7",
       "version": "5.0.0",

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout a2fcb03749ff5897b5985092934dc6057680c789 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/a-/arrow.json b/versions/a-/arrow.json
index bae3c3e..55253ed 100644
--- a/versions/a-/arrow.json
+++ b/versions/a-/arrow.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "6c3d306ac3d08f5ce9b9443f741e656cfe24e5ec",
+      "version": "6.0.0",
+      "port-version": 0
+    },
     {
       "git-tree": "8a30c7d6553216924aa3a3957ceb87d0fda39592",
       "version": "5.0.0",
diff --git a/versions/baseline.json b/versions/baseline.json
index 9484ca6..64e156f 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -169,8 +169,8 @@
       "port-version": 0
     },
     "arrow": {
-      "baseline": "5.0.0",
-      "port-version": 2
+      "baseline": "6.0.0",
+      "port-version": 0
     },
     "ashes": {
       "baseline": "2021-06-18",

@ianmcook
Copy link
Copy Markdown
Contributor Author

@jgiannuzzi Thanks! I rebased to pull in #21467.

@JonLiu1993 I rebased to pull in #20289 and re-tested features locally. arrow[core,csv,dataset,filesystem,json,mimalloc,orc,parquet,s3]:x64-windows now succeeds!

I think this is ready to merge.

@JonLiu1993 JonLiu1993 added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Nov 25, 2021
Comment thread ports/arrow/portfile.cmake Outdated
Comment thread ports/arrow/portfile.cmake Outdated
# so we first need to determine whether we're building it
if(ARROW_WITH_THRIFT AND Thrift_SOURCE STREQUAL "AUTO")
find_package(Thrift 0.11.0 MODULE COMPONENTS ${ARROW_THRIFT_REQUIRED_COMPONENTS})
- if(Thrift_FOUND)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we set THRIFT_SOURCE to BUNDLED rather than patching this?

Will BUNDLED end up trying to use a vendored copy of thrift that might conflict with what other vcpkg ports are using?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will BUNDLED end up trying to use a vendored copy of thrift that might conflict with what other vcpkg ports are using?

Yes, that is the concern. We need to do some testing to investigate this further before making the change. I opened #21821 for the arrow port maintainers to investigate this. If it's OK with the vcpkg maintainers, we would prefer to address this after this PR is merged.

@BillyONeal BillyONeal added requires:author-response and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Nov 26, 2021
@JonLiu1993
Copy link
Copy Markdown
Contributor

@BillyONeal, Could you please take a look

@JonLiu1993 JonLiu1993 added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Dec 8, 2021
@BillyONeal BillyONeal merged commit 8da513e into microsoft:master Dec 8, 2021
@BillyONeal
Copy link
Copy Markdown
Member

Thanks for the update and addressing the feedback; sorry it took so long to see that you fixed the main issue :)

jgiannuzzi added a commit to G-Research/ParquetSharp that referenced this pull request Dec 9, 2021
jgiannuzzi added a commit to G-Research/ParquetSharp that referenced this pull request Jan 14, 2022
jgiannuzzi added a commit to G-Research/ParquetSharp that referenced this pull request Jan 14, 2022
* Update to Arrow-6.0.0

* Review enums
* Update ParquetVersion enum
* Update Encoding enum

* Fix unit tests (schema version)

* Enable standard __cpluscplus macro with MSVC

* Bump version to 6.0.0-beta1

* Statistics test updates for Arrow 6.0.0 (#238)

* Re-enable null-count checks in statistics tests

These were previously incorrect for array valued columns but have been fixed in Arrow 6.0.
See https://issues.apache.org/jira/browse/PARQUET-2067.

* Re-enable min/max statistics tests for decimal

These have been fixed since Arrow 4.0.
See https://issues.apache.org/jira/browse/PARQUET-1655

* Get decimal multiplier from column descriptor for min/max statistics tests

* Point to arrow-6.0.0 PR merge on vcpkg master

microsoft/vcpkg#21113

* Point to arrow-6.0.1 PR merge on vcpkg master

microsoft/vcpkg#22084

* Fix unit tests (schema version)

* Bump version to 6.0.1-beta1

Co-authored-by: Adam Reeve <adreeve@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-update The issue is with a library, which is requesting update new revision info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants