-
Notifications
You must be signed in to change notification settings - Fork 67
Add iceberg_arrow library #6
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.
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
Merged
Merged
Changes from 20 commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
65021dc
Add iceberg_arrow library
wgtmac f84b307
fix linux build
wgtmac 2cfd715
use internal cmake variables
wgtmac 10ef3e5
fix windows build
wgtmac 200bd2f
fix msvc build to not add _static suffix
wgtmac b750758
remove MSVC_TOOLCHAIN
wgtmac 4e766d9
simplify ThirdpartyToolchain.cmake
wgtmac 2866d6e
use same target
wgtmac 5183981
simplify vendored lib
wgtmac 312b13d
use PackageNameConfig
wgtmac 3c5bdfb
Update README.md
wgtmac a6abfd4
Update cmake_modules/ThirdpartyToolchain.cmake
wgtmac 8c09208
Update src/arrow/CMakeLists.txt
wgtmac 80e0a4b
let libiceberg_arrow depend on libiceberg_core
wgtmac 5a064c9
add back MSVC_TOOLCHAIN
wgtmac 93c4228
try to fix windows build
wgtmac fdc0294
add visibility.h
wgtmac 3f6df87
move iceberg_core location
wgtmac cbd661d
link core to puffin & arrow
wgtmac 38a9259
let iceberg_arrow to call api from iceberg_core
wgtmac 59bb135
remove api folder
wgtmac 64c8e29
fix lint
wgtmac 0d337bd
use generate_export_header
wgtmac 149ed21
fix cmake lint
wgtmac 2cb3dc9
add STATIC_DEFINE
wgtmac d88b5ee
try to fix windows export
wgtmac 79bba23
try to fix windows export
wgtmac be17441
try to fix windows export
wgtmac 254bebe
use instead of
wgtmac 24acdac
remove definition of LIB_NAME_shared_EXPORTS
wgtmac e31ae78
Revert "remove definition of LIB_NAME_shared_EXPORTS"
wgtmac 4553485
remove shared_EXPORTS
wgtmac 9c065c6
fix lint
wgtmac 343aa9b
Update cmake_modules/BuildUtils.cmake
wgtmac File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| #pragma once | ||
|
|
||
| #if defined(_WIN32) || defined(__CYGWIN__) | ||
| # ifdef ICEBERG_STATIC | ||
| # define ICEBERG_EXPORT | ||
| # else | ||
| # ifdef ICEBERG_EXPORTING | ||
| # define ICEBERG_EXPORT __declspec(dllexport) | ||
| # else | ||
| # define ICEBERG_EXPORT __declspec(dllimport) | ||
| # endif | ||
| # endif | ||
| #else | ||
| # define ICEBERG_EXPORT | ||
| #endif | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| # Licensed to the Apache Software Foundation (ASF) under one | ||
| # or more contributor license agreements. See the NOTICE file | ||
| # distributed with this work for additional information | ||
| # regarding copyright ownership. The ASF licenses this file | ||
| # to you under the Apache License, Version 2.0 (the | ||
| # "License"); you may not use this file except in compliance | ||
| # with the License. You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, | ||
| # software distributed under the License is distributed on an | ||
| # "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| # KIND, either express or implied. See the License for the | ||
| # specific language governing permissions and limitations | ||
| # under the License. | ||
|
|
||
| # Accumulate all dependencies to provide suitable static link parameters to the | ||
| # third party libraries. | ||
| set(ICEBERG_SYSTEM_DEPENDENCIES) | ||
| set(ICEBERG_ARROW_INSTALL_INTERFACE_LIBS) | ||
|
|
||
| # ---------------------------------------------------------------------- | ||
| # Versions and URLs for toolchain builds | ||
|
|
||
| set(ICEBERG_ARROW_BUILD_VERSION "18.1.0") | ||
| set(ICEBERG_ARROW_BUILD_SHA256_CHECKSUM | ||
| "2dc8da5f8796afe213ecc5e5aba85bb82d91520eff3cf315784a52d0fa61d7fc") | ||
|
|
||
| if(DEFINED ENV{ICEBERG_ARROW_URL}) | ||
| set(ARROW_SOURCE_URL "$ENV{ICEBERG_ARROW_URL}") | ||
| else() | ||
| set(ARROW_SOURCE_URL | ||
| "https://www.apache.org/dyn/closer.lua?action=download&filename=/arrow/arrow-${ICEBERG_ARROW_BUILD_VERSION}/apache-arrow-${ICEBERG_ARROW_BUILD_VERSION}.tar.gz" | ||
| "https://downloads.apache.org/arrow/arrow-${ICEBERG_ARROW_BUILD_VERSION}/apache-arrow-${ICEBERG_ARROW_BUILD_VERSION}.tar.gz" | ||
| ) | ||
| endif() | ||
|
|
||
| # ---------------------------------------------------------------------- | ||
| # FetchContent | ||
|
|
||
| include(FetchContent) | ||
| set(FC_DECLARE_COMMON_OPTIONS) | ||
| if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.28) | ||
| list(APPEND FC_DECLARE_COMMON_OPTIONS EXCLUDE_FROM_ALL TRUE) | ||
| endif() | ||
|
|
||
| macro(prepare_fetchcontent) | ||
| set(BUILD_SHARED_LIBS OFF) | ||
| set(BUILD_STATIC_LIBS ON) | ||
| set(CMAKE_COMPILE_WARNING_AS_ERROR FALSE) | ||
| set(CMAKE_EXPORT_NO_PACKAGE_REGISTRY TRUE) | ||
| set(CMAKE_POSITION_INDEPENDENT_CODE ON) | ||
| endmacro() | ||
|
|
||
| # ---------------------------------------------------------------------- | ||
| # Apache Arrow | ||
|
|
||
| function(resolve_arrow_dependency) | ||
| prepare_fetchcontent() | ||
|
|
||
| set(ARROW_BUILD_SHARED | ||
| OFF | ||
| CACHE BOOL "" FORCE) | ||
| set(ARROW_BUILD_STATIC | ||
| ON | ||
| CACHE BOOL "" FORCE) | ||
| set(ARROW_FILESYSTEM | ||
| OFF | ||
| CACHE BOOL "" FORCE) | ||
| set(ARROW_SIMD_LEVEL | ||
| "NONE" | ||
| CACHE STRING "" FORCE) | ||
| set(ARROW_RUNTIME_SIMD_LEVEL | ||
| "NONE" | ||
| CACHE STRING "" FORCE) | ||
| set(ARROW_POSITION_INDEPENDENT_CODE | ||
| ON | ||
| CACHE BOOL "" FORCE) | ||
| set(ARROW_DEPENDENCY_SOURCE | ||
| "AUTO" | ||
| CACHE STRING "" FORCE) | ||
|
|
||
| fetchcontent_declare(Arrow | ||
wgtmac marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ${FC_DECLARE_COMMON_OPTIONS} | ||
| URL ${ARROW_SOURCE_URL} | ||
| URL_HASH "SHA256=${ICEBERG_ARROW_BUILD_SHA256_CHECKSUM}" | ||
| SOURCE_SUBDIR | ||
| cpp | ||
| FIND_PACKAGE_ARGS | ||
| NAMES | ||
| Arrow | ||
| CONFIG) | ||
|
|
||
| # Add Arrow cmake modules to the search path | ||
| list(PREPEND CMAKE_MODULE_PATH | ||
| ${CMAKE_CURRENT_BINARY_DIR}/_deps/arrow-src/cpp/cmake_modules) | ||
wgtmac marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| fetchcontent_makeavailable(Arrow) | ||
|
|
||
| if(arrow_SOURCE_DIR) | ||
| if(NOT TARGET Arrow::arrow_static) | ||
| add_library(Arrow::arrow_static INTERFACE IMPORTED) | ||
| target_link_libraries(Arrow::arrow_static INTERFACE arrow_static) | ||
| target_include_directories(Arrow::arrow_static | ||
| INTERFACE ${arrow_BINARY_DIR}/src | ||
| ${arrow_SOURCE_DIR}/cpp/src) | ||
| endif() | ||
|
|
||
| set(ARROW_VENDORED TRUE) | ||
| set_target_properties(arrow_static PROPERTIES OUTPUT_NAME "iceberg_vendored_arrow") | ||
| install(TARGETS arrow_static | ||
| EXPORT iceberg_targets | ||
| RUNTIME DESTINATION "${ICEBERG_INSTALL_BINDIR}" | ||
| ARCHIVE DESTINATION "${ICEBERG_INSTALL_LIBDIR}" | ||
| LIBRARY DESTINATION "${ICEBERG_INSTALL_LIBDIR}") | ||
| else() | ||
| set(ARROW_VENDORED FALSE) | ||
| list(APPEND ICEBERG_SYSTEM_DEPENDENCIES Arrow) | ||
| endif() | ||
|
|
||
| set(ICEBERG_SYSTEM_DEPENDENCIES | ||
| ${ICEBERG_SYSTEM_DEPENDENCIES} | ||
| PARENT_SCOPE) | ||
| set(ARROW_VENDORED | ||
| ${ARROW_VENDORED} | ||
| PARENT_SCOPE) | ||
| endfunction() | ||
|
|
||
| if(ICEBERG_ARROW) | ||
| resolve_arrow_dependency() | ||
| endif() | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to prepare this for each shared library. (We need
ICEBERG_CORE_EXPORT/ICEBERG_PUFFIN_EXPORT/ICEBERG_ARROW_EXPORT.)Because
libiceberg_arrow.dllwill use exported symbols inlibiceberg_core.dll. In this case,libiceberg_arrow.dlluses_declspec(dllexport)forlibiceberg_arrow.dllsymbols and_declspec(dllimport)forlibiceberg_core.dllsymbols.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. What if we define
ICEBERG_EXPORTINGto all oflibiceberg_core.dll,libiceberg_arrow.dllandlibiceberg_puffin.dll? I think we can simply keep the same visibility to all libraries for now because thecorelibrary provides the API and core logic whilepuffinandarrowprovide concrete implementations of thecorelibrary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it causes link errors. If
libiceberg_arrow.dlluses alibiceberg_core.dllsymbol,libiceberg_arrow.dlluses__declspec(dllexport)-edlibiceberg_core.dllsymbols...Can we add a simple function to
libiceberg_core.dlland use it fromlibiceberg_arrow.dll?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try it on my Windows PC tonight. Thanks!
EDIT: I have added a new function
DemoTable::name()in demo_table.h and called it in demo_arrow.cc. It does not have linking error. @kouUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is relevant to how we manage the public headers from different iceberg libraries. As per previous discussion, we use a separate
apifolder holds all public header files from all libraries (i.e. core, puffin, arrow, etc.). IIUC, the current approach makes it difficult to manage the visibility of the header file across libraries. I'd propose to remove the currentapifolder and let each library manage its own header files. WDYT? @kou @raulcd @zhjwpku @gaborkaszab @lidavidm @pitrouThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having separate directories per library is reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. It may have a real world problem because
libiceberg_arrow.libis always used withlibiceberg_core.lib...FYI: If we use the same
ICEBERG_EXPORTINGfor all libraries,libiceberg_arrow.libalso exports symbols inlibiceberg_core.dllbutlibiceberg_arrow.dlldoesn't have them:If we use separated
_EXPORTINGmacro for each library,DemoTablesymbols aren't exported bylibiceberg_arrow.dll:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the
apifolder and move the header files to each library instead.