Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ARROW-1142: [C++] Port over compression toolchain and interfaces from parquet-cpp, use Arrow-style error handling #771

Closed
wants to merge 8 commits into from
Closed

ARROW-1142: [C++] Port over compression toolchain and interfaces from parquet-cpp, use Arrow-style error handling #771

wants to merge 8 commits into from

Conversation

wesm
Copy link
Member

@wesm wesm commented Jun 23, 2017

No description provided.

wesm added 2 commits June 23, 2017 12:24
@wesm
Port over compression toolchain and interfaces from parquet-cpp, adap…
da31c2c 
…t to Arrow-style error handling

Change-Id: I7db868884ba173c2c0c3cac0148bd5c218a52db7
@wesm
Clean up build dependencies
198dee1 
Change-Id: I8b5435349957fe626becce7a147ff3e0044d0b6d
@wesm wesm force-pushed the import-parquet-compression branch from 0911b60 to 198dee1 Compare June 23, 2017 16:24
wesm added 3 commits June 23, 2017 13:15
@wesm
Add some license headers, use Apache Kudu approach for managing a lot…
cbbaecf 
… of RAT exclusions

Change-Id: Ibd8326781632b7ca01731cb6cdf756c17e10ae36
@wesm
Clean up RAT exclusions, use absolute paths
2ef43de 
Change-Id: I2e1d272ec4d86a7027179290ceed0e3dcaebe3fe
@wesm
cpplint
31b2705 
Change-Id: Ib29b616aef7899f0cf679a671867d6a50bddd5e5
@wesm
Copy link
Member Author

wesm commented Jun 23, 2017 •
edited
Loading

Appveyor build: https://ci.appveyor.com/project/wesm/arrow/build/1.0.535

@wesm
Revert bash equality test
732e426 
Change-Id: I081e5e38cda4e95655647cae703dabe72179ac8c
@wesm
Copy link
Member Author

wesm commented Jun 23, 2017

Appveyor build: https://ci.appveyor.com/project/wesm/arrow/build/1.0.540. Now just waiting on Travis CI

@wesm
Install compression.h
44e77a6 
Change-Id: I465b018871a404207b56a466d716c2a5655447a3
@wesm wesm mentioned this pull request Jun 23, 2017
Closed
cpcloud
cpcloud reviewed Jun 23, 2017
View reviewed changes
cpp/cmake_modules/ThirdpartyToolchain.cmake
if (NOT DEFINED ENV{BOOST_ROOT})
# Since we have to set this in the environment, we check whether
# $BOOST_ROOT is defined inside here
set(ENV{BOOST_ROOT} "$ENV{ARROW_BUILD_TOOLCHAIN}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to leave BOOST_ROOT out of this? My build environment uses the OS boost and conda stuff for everything else. I'm also happy to explicitly set BOOST_ROOT if we want to tie boost to the build toolchain.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you don't have boost installed in your toolchain, this is actually fine (I think). Does CMake fail to find Boost when you set $ARROW_BUILD_TOOLCHAIN?

cpcloud
cpcloud reviewed Jun 23, 2017
View reviewed changes
cpp/cmake_modules/ThirdpartyToolchain.cmake

set(Boost_DEBUG TRUE)
set(Boost_USE_MULTITHREADED ON)
set(Boost_ADDITIONAL_VERSIONS
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible to add 1.64.0, and 1.64 here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is moved code from the original root CMakeLists.txt, but sure

cpcloud
cpcloud reviewed Jun 23, 2017
View reviewed changes
cpp/cmake_modules/ThirdpartyToolchain.cmake Outdated
if (ARROW_BOOST_HEADER_ONLY)
find_package(Boost)
else()
find_package(Boost COMPONENTS system filesystem regex REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should regex be in there?

cpp/cmake_modules/ThirdpartyToolchain.cmake
-BUILD_CONFIG_TESTS=OFF
-DINSTALL_HEADERS=ON
-DCMAKE_CXX_FLAGS=${GFLAGS_CMAKE_CXX_FLAGS})
if (CMAKE_VERSION VERSION_GREATER "3.2")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this if be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

(and any others that might also be around)

@wesm
Copy link
Member Author

wesm commented Jun 23, 2017

Opened https://issues.apache.org/jira/browse/ARROW-1148 about raising the minimum CMake version to 3.2, can do that in a follow up patch

@wesm
Boost toolchain tweaks
b7609f8 
Change-Id: Id6781f80f4767ba3af8c0e2822322940f94685e7
@wesm
Copy link
Member Author

wesm commented Jun 23, 2017

Here's the Appveyor build: https://ci.appveyor.com/project/wesm/arrow/build/1.0.546. Bombs away

@asfgit asfgit closed this in 98f7cac Jun 23, 2017
@wesm wesm deleted the import-parquet-compression branch June 23, 2017 23:07
xhochy
xhochy reviewed Jun 25, 2017
View reviewed changes
.readthedocs.yml
@@ -1,2 +1,19 @@
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
Copy link
Member

Choose a reason for hiding this comment

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

We should remove this file as we don't use readthedocs anymore.

asfgit pushed a commit to apache/parquet-cpp that referenced this pull request Jun 25, 2017
@wesm
PARQUET-1044: Use compression libraries from Apache Arrow
491182c 
Depends on apache/arrow#771; will update Arrow version once that's merged

Author: Wes McKinney <[email protected]>

Closes #362 from wesm/move-compression-to-arrow and squashes the following commits:

b055b69 [Wes McKinney] Check status, do not use /WX with Arrow EP build on MSVC
6e9b9cd [Wes McKinney] Fixes for macOS and Windows
6d39a39 [Wes McKinney] Fix function name
a0bfa95 [Wes McKinney] Untabify file
e095b10 [Wes McKinney] Use compression toolchain from Arrow
wesm added a commit to wesm/arrow that referenced this pull request Sep 2, 2018
@wesm
PARQUET-1044: Use compression libraries from Apache Arrow
aed1e85 
Depends on apache#771; will update Arrow version once that's merged

Author: Wes McKinney <[email protected]>

Closes apache#362 from wesm/move-compression-to-arrow and squashes the following commits:

b055b69 [Wes McKinney] Check status, do not use /WX with Arrow EP build on MSVC
6e9b9cd [Wes McKinney] Fixes for macOS and Windows
6d39a39 [Wes McKinney] Fix function name
a0bfa95 [Wes McKinney] Untabify file
e095b10 [Wes McKinney] Use compression toolchain from Arrow
wesm added a commit to wesm/arrow that referenced this pull request Sep 4, 2018
@wesm
PARQUET-1044: Use compression libraries from Apache Arrow
904129b 
Depends on apache#771; will update Arrow version once that's merged

Author: Wes McKinney <[email protected]>

Closes apache#362 from wesm/move-compression-to-arrow and squashes the following commits:

b055b69 [Wes McKinney] Check status, do not use /WX with Arrow EP build on MSVC
6e9b9cd [Wes McKinney] Fixes for macOS and Windows
6d39a39 [Wes McKinney] Fix function name
a0bfa95 [Wes McKinney] Untabify file
e095b10 [Wes McKinney] Use compression toolchain from Arrow

Change-Id: I381e1e2f8f8621bfdb4a20900eba1218de29855f
wesm added a commit to wesm/arrow that referenced this pull request Sep 6, 2018
@wesm
PARQUET-1044: Use compression libraries from Apache Arrow
15eee49 
Depends on apache#771; will update Arrow version once that's merged

Author: Wes McKinney <[email protected]>

Closes apache#362 from wesm/move-compression-to-arrow and squashes the following commits:

b055b69 [Wes McKinney] Check status, do not use /WX with Arrow EP build on MSVC
6e9b9cd [Wes McKinney] Fixes for macOS and Windows
6d39a39 [Wes McKinney] Fix function name
a0bfa95 [Wes McKinney] Untabify file
e095b10 [Wes McKinney] Use compression toolchain from Arrow

Change-Id: I381e1e2f8f8621bfdb4a20900eba1218de29855f
wesm added a commit to wesm/arrow that referenced this pull request Sep 7, 2018
@wesm
PARQUET-1044: Use compression libraries from Apache Arrow
c38bc9d 
Depends on apache#771; will update Arrow version once that's merged

Author: Wes McKinney <[email protected]>

Closes apache#362 from wesm/move-compression-to-arrow and squashes the following commits:

b055b69 [Wes McKinney] Check status, do not use /WX with Arrow EP build on MSVC
6e9b9cd [Wes McKinney] Fixes for macOS and Windows
6d39a39 [Wes McKinney] Fix function name
a0bfa95 [Wes McKinney] Untabify file
e095b10 [Wes McKinney] Use compression toolchain from Arrow

Change-Id: I381e1e2f8f8621bfdb4a20900eba1218de29855f
wesm added a commit to wesm/arrow that referenced this pull request Sep 8, 2018
@wesm
PARQUET-1044: Use compression libraries from Apache Arrow
b0414cc 
Depends on apache#771; will update Arrow version once that's merged

Author: Wes McKinney <[email protected]>

Closes apache#362 from wesm/move-compression-to-arrow and squashes the following commits:

b055b69 [Wes McKinney] Check status, do not use /WX with Arrow EP build on MSVC
6e9b9cd [Wes McKinney] Fixes for macOS and Windows
6d39a39 [Wes McKinney] Fix function name
a0bfa95 [Wes McKinney] Untabify file
e095b10 [Wes McKinney] Use compression toolchain from Arrow

Change-Id: I381e1e2f8f8621bfdb4a20900eba1218de29855f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xhochy xhochy xhochy left review comments

@cpcloud cpcloud cpcloud approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wesm @xhochy @cpcloud