Copilot commented on code in PR #922:
URL: https://github.com/apache/incubator-graphar/pull/922#discussion_r3215989128
##########
.github/workflows/python-wheel-workflow.yml:
##########
@@ -176,15 +173,13 @@ jobs:
shell: bash
run: |
set -euxo pipefail
- mamba install -y -c conda-forge arrow-cpp
- # Note: CONDA_PREFIX may be unset unless conda is activated in this
shell.
- # setup-miniconda exports CONDA (base install prefix), which is
sufficient here.
- echo "CMAKE_PREFIX_PATH=$CONDA" >> "$GITHUB_ENV"
+ brew install apache-arrow
+ echo "CMAKE_PREFIX_PATH=/usr/local" >> "$GITHUB_ENV"
# Optional sanity check: ensure Arrow dylib isn't built for a newer
macOS than deployment target.
if command -v otool >/dev/null 2>&1; then
- ls -lah "$CONDA/lib" || true
- if [ -f "$CONDA/lib/libarrow.dylib" ]; then
- otool -l "$CONDA/lib/libarrow.dylib" | (grep -A3 -E
'LC_BUILD_VERSION|LC_VERSION_MIN_MACOSX' || true)
+ ls -lah /usr/local/lib || true
+ if [ -f "/usr/local/lib/libarrow.dylib" ]; then
+ otool -l "/usr/local/lib/libarrow.dylib" | (grep -A3 -E
'LC_BUILD_VERSION|LC_VERSION_MIN_MACOSX' || true)
fi
Review Comment:
On `macos-latest` (arm64), Homebrew installs under `/opt/homebrew` (not
`/usr/local`). Hardcoding `CMAKE_PREFIX_PATH=/usr/local` and checking
`/usr/local/lib/libarrow.dylib` will typically prevent CMake from finding Arrow
and will make the sanity check miss the installed dylib. Prefer using `$(brew
--prefix)` (and its `lib` dir) to set `CMAKE_PREFIX_PATH` and the otool path.
##########
.github/workflows/python-wheel-workflow.yml:
##########
@@ -121,7 +118,7 @@ jobs:
CIBW_MANYLINUX_X86_64_IMAGE: ${{ matrix.os == 'linux' &&
format('manylinux{0}', matrix.manylinux) || '' }}
CIBW_MANYLINUX_AARCH64_IMAGE: ${{ matrix.os == 'linux' &&
format('manylinux{0}', matrix.manylinux) || '' }}
CIBW_ENVIRONMENT_WINDOWS: DISTUTILS_USE_SDK=1 MSSdk=1
- CIBW_ENVIRONMENT_MACOS: ${{ matrix.os == 'macos' &&
format('MACOSX_DEPLOYMENT_TARGET={0} CMAKE_OSX_DEPLOYMENT_TARGET={0}
CFLAGS=-mmacosx-version-min={0} CXXFLAGS=-mmacosx-version-min={0}
LDFLAGS=-mmacosx-version-min={0}', matrix.deployment-target) || '' }}
+ CIBW_ENVIRONMENT_MACOS: ${{ matrix.os == 'macos' &&
'MACOSX_DEPLOYMENT_TARGET=$(sw_vers -productVersion)
CMAKE_OSX_DEPLOYMENT_TARGET=$(sw_vers -productVersion)
CFLAGS=-mmacosx-version-min=$(sw_vers -productVersion)
CXXFLAGS=-mmacosx-version-min=$(sw_vers -productVersion)
LDFLAGS=-mmacosx-version-min=$(sw_vers -productVersion)' || '' }}
Review Comment:
Setting `MACOSX_DEPLOYMENT_TARGET`/`-mmacosx-version-min` to the runner’s
current OS version makes wheel tags depend on whatever `macos-latest` is at
build time and reduces compatibility with older macOS versions. Consider
restoring a fixed minimum deployment target (e.g., the previously used 11.0) so
the produced wheels remain broadly installable and reproducible.
##########
cpp/src/graphar/filesystem.cc:
##########
@@ -93,6 +94,23 @@ static Status CastToLargeOffsetArray(
namespace graphar {
namespace ds = arrow::dataset;
+namespace {
+Status EnsureDatasetScannerInitialized() {
+ static bool initialized = false;
+ static Status init_status = Status::OK();
+ if (!initialized) {
+ auto st = arrow::compute::Initialize();
+ if (!st.ok()) {
+ init_status = Status::ArrowError(st.ToString());
+ } else {
+ arrow::dataset::internal::Initialize();
+ }
+ initialized = true;
+ }
+ return init_status;
Review Comment:
This one-time initialization uses `static bool initialized` without
synchronization. If `ReadFileToTable()` is called from multiple threads, this
introduces a data race around `initialized`/`init_status` (undefined behavior).
Use `std::once_flag`/`std::call_once` (and store the init status inside the
call_once) to make initialization thread-safe.
##########
cpp/src/graphar/arrow/chunk_reader.cc:
##########
@@ -110,6 +115,11 @@ Status CastStringToLargeString(const
std::shared_ptr<arrow::Array>& in,
Status CastTableWithSchema(const std::shared_ptr<arrow::Table>& table,
const std::shared_ptr<arrow::Schema>& schema,
std::shared_ptr<arrow::Table>* out_table) {
+ static bool initialized = false;
+ if (!initialized) {
+ RETURN_NOT_ARROW_OK(arrow::compute::Initialize());
+ initialized = true;
+ }
Review Comment:
This compute initialization uses a non-atomic `static bool` guard. If
`CastTableWithSchema()` is called concurrently, this is a data race (undefined
behavior). Use `std::once_flag`/`std::call_once` (or reuse an existing
thread-safe init helper) instead.
##########
cpp/src/graphar/high-level/graph_reader.cc:
##########
@@ -940,6 +942,12 @@ Result<std::shared_ptr<EdgesCollection>>
EdgesCollection::Make(
const std::string& edge_type, const std::string& dst_type,
AdjListType adj_list_type, const IdType vertex_chunk_begin,
const IdType vertex_chunk_end) noexcept {
+ static bool initialized = false;
+ if (!initialized) {
+ RETURN_NOT_ARROW_OK(arrow::compute::Initialize());
+ arrow::dataset::internal::Initialize();
+ initialized = true;
+ }
Review Comment:
This lazy initialization uses `static bool initialized` without
synchronization. If `EdgesCollection::Make()` can be invoked concurrently, this
is a data race (undefined behavior). Prefer `std::once_flag`/`std::call_once`
for thread-safe one-time initialization.
##########
.github/workflows/pre-commit.yml:
##########
@@ -41,9 +41,15 @@ jobs:
runs-on: ubuntu-slim
Review Comment:
`runs-on: ubuntu-slim` is not a valid GitHub-hosted runner label, so this
workflow will never start. Use a supported runner (e.g., `ubuntu-latest` /
`ubuntu-22.04` / `ubuntu-24.04`) or a self-hosted runner label.
##########
cpp/src/graphar/expression.cc:
##########
@@ -22,64 +22,83 @@
namespace graphar {
+Status EnsureComputeInitialized() {
+ static bool initialized = false;
+ if (!initialized) {
+ RETURN_NOT_ARROW_OK(arrow::compute::Initialize());
+ initialized = true;
+ }
+ return Status::OK();
Review Comment:
This lazy initialization uses a non-atomic `static bool` guard. If multiple
threads call `EnsureComputeInitialized()` concurrently, this is a data race
(undefined behavior) and can result in double-initialization or worse. Use
`std::once_flag`/`std::call_once` (or another thread-safe one-time init)
instead of `static bool`.
##########
cpp/src/graphar/arrow/chunk_reader.cc:
##########
@@ -73,6 +73,11 @@ Result<std::shared_ptr<arrow::Schema>> LabelToSchema(
Status GeneralCast(const std::shared_ptr<arrow::Array>& in,
const std::shared_ptr<arrow::DataType>& to_type,
std::shared_ptr<arrow::Array>* out) {
+ static bool initialized = false;
+ if (!initialized) {
+ RETURN_NOT_ARROW_OK(arrow::compute::Initialize());
+ initialized = true;
+ }
Review Comment:
This compute initialization uses a non-atomic `static bool` guard. If
`GeneralCast()` is called concurrently, this is a data race (undefined
behavior). Use `std::once_flag`/`std::call_once` (or reuse an existing
thread-safe init helper) instead.
##########
cpp/src/graphar/reader_util.cc:
##########
@@ -45,6 +45,11 @@ namespace graphar::util {
Status CheckFilterOptions(
const FilterOptions& filter_options,
const std::shared_ptr<PropertyGroup>& property_group) noexcept {
+ static bool initialized = false;
+ if (!initialized) {
+ RETURN_NOT_ARROW_OK(arrow::compute::Initialize());
+ initialized = true;
+ }
Review Comment:
This lazy initialization uses a non-atomic `static bool` guard. If
`CheckFilterOptions()` is called from multiple threads, this is a data race
(undefined behavior). Use `std::once_flag`/`std::call_once` (or reuse a shared,
thread-safe init helper) instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
