feat: support x86-64 and arm64 architectures of macOS#166
feat: support x86-64 and arm64 architectures of macOS#166SteNicholas wants to merge 1 commit intoalibaba:mainfrom
Conversation
35f6e5d to
cdabab7
Compare
There was a problem hiding this comment.
Pull request overview
Adds macOS/aarch64 support with a more minimal feature set by updating build/link logic, CI, and documentation.
Changes:
- Adjusts CMake/linker behavior for Apple (exported symbols list, different link flags, and test link behavior).
- Updates tests and some I/O code to use explicit
int64_tliterals/casts for portability. - Adds macOS CI jobs and documents macOS build requirements and Linux-only optional components.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| test/inte/scan_inte_test.cpp | Replaces long literals with explicit int64_t to avoid platform-dependent widths. |
| test/inte/read_inte_with_index_test.cpp | Makes BIGINT literals explicit int64_t for consistent behavior across arch/platforms. |
| src/paimon/testing/utils/CMakeLists.txt | Switches test link flags for Apple vs non-Apple. |
| src/paimon/symbols.list | Adds macOS exported symbols list file for controlling shared library exports. |
| src/paimon/global_index/lumina/lumina_global_index_test.cpp | Uses explicit int64_t literal in predicate construction. |
| src/paimon/fs/jindo/CMakeLists.txt | Adds jindosdk::c_sdk to dependencies/link libraries. |
| src/paimon/format/parquet/predicate_pushdown_test.cpp | Uses explicit int64_t literals in predicates. |
| src/paimon/format/parquet/predicate_converter_test.cpp | Updates literals (but introduces a value mismatch in one assertion). |
| src/paimon/format/parquet/CMakeLists.txt | Adds Apple-specific test link flags. |
| src/paimon/format/orc/predicate_pushdown_test.cpp | Uses explicit int64_t literals in predicates. |
| src/paimon/format/orc/predicate_converter_test.cpp | Uses explicit int64_t literals in predicates. |
| src/paimon/format/orc/CMakeLists.txt | Adds Apple-specific test link flags. |
| src/paimon/format/lance/CMakeLists.txt | Adds Apple-specific test link flags. |
| src/paimon/format/blob/blob_format_writer.cpp | Changes read chunk sizing logic with explicit 32-bit casts. |
| src/paimon/format/blob/CMakeLists.txt | Adds Apple-specific test link flags. |
| src/paimon/format/avro/avro_input_stream_impl.cpp | Adds explicit size_t casts in std::min call. |
| src/paimon/format/avro/CMakeLists.txt | Adds Apple-specific test link flags. |
| src/paimon/core/utils/field_mapping_test.cpp | Uses explicit int64_t literals in BIGINT predicates. |
| src/paimon/core/table/source/table_read_test.cpp | Uses explicit int64_t literals in BIGINT predicates. |
| src/paimon/common/reader/prefetch_file_batch_reader_impl_test.cpp | Uses explicit int64_t literals in predicates. |
| src/paimon/common/reader/predicate_batch_reader_test.cpp | Uses explicit int64_t literals in predicates. |
| src/paimon/common/predicate/predicate_validator_test.cpp | Uses explicit int64_t literals in BIGINT predicates. |
| src/paimon/common/predicate/predicate_test.cpp | Uses explicit int64_t literals in BIGINT predicates/literal vectors. |
| src/paimon/common/predicate/literal_converter_test.cpp | Uses explicit int64_t literals; fixes some INFINITY/NAN casts. |
| src/paimon/common/memory/memory_pool.cpp | Changes Realloc alignment parameter type to uint64_t. |
| src/paimon/common/logging/logging.cpp | Uses a fixed name for glog initialization on macOS. |
| src/paimon/common/format/file_format_factory.cpp | Replaces dynamic_cast with static_cast for factory downcast. |
| src/paimon/CMakeLists.txt | Adds Apple-specific whole-archive/force-load behavior for tests. |
| docs/source/building.rst | Documents macOS build requirements and Linux-only optional components. |
| cmake_modules/ThirdpartyToolchain.cmake | Updates third-party build/install steps for macOS (Jindo/ORC/Boost/Arrow/Protobuf). |
| cmake_modules/SetupCxxFlags.cmake | Disables -Wglobal-constructors (now -Wno-global-constructors). |
| cmake_modules/BuildUtils.cmake | Adds Apple-specific shared link options (-dead_strip) vs ELF-only flags. |
| ci/scripts/build_paimon.sh | Selects different CMake feature flags on macOS vs non-macOS in CI. |
| CMakeLists.txt | Adds macOS exported symbols list usage; defaults Lumina/Lucene OFF. |
| .github/workflows/gcc_test.yaml | Splits Ubuntu vs macOS jobs; adds macOS job. |
| .github/workflows/clang_test.yaml | Splits Ubuntu vs macOS jobs; adds macOS job. |
Comments suppressed due to low confidence (1)
src/paimon/symbols.list:1
- For
-Wl,-exported_symbols_list(ld64), the exported symbols list typically matches Mach-O symbol names (often with a leading underscore) and is usually enumerated explicitly or via supported patterns. The pattern*paimon*may not match the actual exported symbol names (and could accidentally export nothing or export far more than intended). Consider switching to explicit exported symbol entries (with correct Mach-O symbol spelling) or a known-good wildcard pattern (e.g.,_paimon_*if that matches your ABI symbols).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
src/paimon/testing/utils/CMakeLists.txt:1
-Wl,-force_load,<file>expects a static archive (.a). Here it force-loads$<TARGET_FILE:paimon_local_file_system_shared>which is presumably a shared library (.dylib), and that can fail on macOS linkers. If the intent is to pull in all objects for registration, force-load the corresponding static target (e.g.,paimon_local_file_system_static) or drop-force_loadand link the shared library normally.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/paimon/testing/utils/CMakeLists.txt:1
- On macOS, "-Wl,-force_load,..." is intended for static archives (.a). Passing a shared library target file (likely .dylib) can fail or be ignored by the linker, causing missing-symbol issues at link/runtime. Consider switching to the static target here (e.g., "paimon_local_file_system_static") or adjusting the linkage so the required objects are pulled in without relying on force-loading a dylib.
src/paimon/symbols.list:1 - macOS "-exported_symbols_list" expects exact exported symbol names (typically with a leading underscore), not wildcard/glob patterns. Using "paimon" is unlikely to work and may result in exporting nothing (or failing the link). Replace this with a concrete list of exported symbols (one per line) or use a different export control mechanism appropriate for Apple toolchains.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
ed966b0 to
e7e38f8
Compare
3b95a16 to
f9cc572
Compare
| "-Wl,--no-whole-archive" | ||
| ${GTEST_LINK_TOOLCHAIN}) | ||
| if(APPLE) | ||
| add_paimon_test(avro_format_test |
There was a problem hiding this comment.
Maybe you can put the logic to determine whether it is a Mac system into the add_paimon_test macro, the main difference is --whole-archive --no-as-needed vs -force_load
| } | ||
| auto file_format_factory = | ||
| dynamic_cast<FileFormatFactory*>(factory_creator->Create(identifier)); | ||
| auto file_format_factory = static_cast<FileFormatFactory*>(factory_creator->Create(identifier)); |
There was a problem hiding this comment.
Can it work without modifying static_cast by using a linker script approach?
Purpose
Linked issue: close #132
Support x86-64 and arm64 architectures of macOS to create a fairly minimal build, which does not support
PAIMON_ENABLE_LANCE,PAIMON_ENABLE_LUMINAandPAIMON_ENABLE_LUCENE.Tests
clang-test-macos-x86-64API and Format
Introduce
PAIMON_ENABLE_MACOSto enable experimental macOS support.Documentation
Introduce experimental macOS support in
building.rst.