Skip to content

GH-50631: [C++] Scope macOS SDK 11.3 simdjson workaround to bundled simdjson - #50633

Merged
kou merged 1 commit into
apache:mainfrom
Reranko05:gh-50631-macos
Jul 25, 2026
Merged

GH-50631: [C++] Scope macOS SDK 11.3 simdjson workaround to bundled simdjson#50633
kou merged 1 commit into
apache:mainfrom
Reranko05:gh-50631-macos

Conversation

@Reranko05

@Reranko05Reranko05 commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Rationale for this change

The workaround introduced for the macOS 11.3 SDK currently uses a global add_compile_definitions(SIMDJSON_CONCEPT_DISABLED=1).

Since this workaround is only needed when building the bundled simdjson dependency, scope it to the simdjson target inside build_simdjson() instead of applying it globally.

What changes are included in this PR?

  • Move the macOS SDK 11.3 workaround into build_simdjson().
  • Replace the global add_compile_definitions() with target_compile_definitions(simdjson PUBLIC SIMDJSON_CONCEPT_DISABLED=1).

CopilotAI review requested due to automatic review settings July 25, 2026 01:34

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #50631has been automatically assigned in GitHub to PR creator.

@Reranko05

Copy link
Copy Markdown
CollaboratorAuthor

@kou As suggested, I opened an upstream issue for this: simdjson/simdjson#2787

@kou

kou commented Jul 25, 2026

Copy link
Copy Markdown
Member

@github-actions crossbow submit -g r

@koukou added the CI: Extra Run extra CI label Jul 25, 2026
@github-actions

Copy link
Copy Markdown

Revision: fa60e5d

Submitted crossbow builds: ursacomputing/crossbow @ actions-343cdd513e

TaskStatus
r-binary-packagesGitHub Actions
r-recheck-mostGitHub Actions
test-r-alpine-linux-cranGitHub Actions
test-r-arrow-backwards-compatibilityGitHub Actions
test-r-depsource-systemGitHub Actions
test-r-dev-duckdbGitHub Actions
test-r-devdocsGitHub Actions
test-r-extra-packagesGitHub Actions
test-r-fedora-clangGitHub Actions
test-r-gcc-11GitHub Actions
test-r-gcc-12GitHub Actions
test-r-install-localGitHub Actions
test-r-install-local-minsizerelGitHub Actions
test-r-linux-as-cranGitHub Actions
test-r-linux-rchkGitHub Actions
test-r-linux-sanitizersGitHub Actions
test-r-linux-valgrindGitHub Actions
test-r-m1-sanGitHub Actions
test-r-macos-as-cranGitHub Actions
test-r-offline-maximalGitHub Actions
test-r-ubuntu-22.04GitHub Actions
test-r-versionsGitHub Actions
test-r-wasmGitHub Actions

Comment threadcpp/cmake_modules/ThirdpartyToolchain.cmake Outdated
@github-actionsgithub-actionsBot added awaiting committer review Awaiting committer review awaiting changes Awaiting changes and removed awaiting review Awaiting review awaiting committer review Awaiting committer review labels Jul 25, 2026
CopilotAI review requested due to automatic review settings July 25, 2026 09:14

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 25, 2026
@rok

rok commented Jul 25, 2026

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-r-macos-as-cran

@github-actions

Copy link
Copy Markdown

Revision: 4c4d64c

Submitted crossbow builds: ursacomputing/crossbow @ actions-ec06c62a76

TaskStatus
test-r-macos-as-cranGitHub Actions

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment threadcpp/cmake_modules/ThirdpartyToolchain.cmake
@Reranko05

Copy link
Copy Markdown
CollaboratorAuthor

@kou@rok The original simdjson/concepts.h failure is gone after changing the definition to PUBLIC. The Crossbow job now fails later in vector_sort.cc with a different error (std::span<uint64_t>), which seems unrelated to this change. Any thoughts on this new failure?

@rok

rok commented Jul 25, 2026

Copy link
Copy Markdown
Member

@Reranko05 the issue was likely caused by introduction of std::span in #50248, I opened an issue #50636. Otherwise I think the change here is good, but I defer to @kou :)

@Reranko05

Copy link
Copy Markdown
CollaboratorAuthor

@rok Thanks for the review :)

@rok
rok requested a review from CopilotJuly 25, 2026 11:16

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

kou
kou approved these changes Jul 25, 2026

@koukou left a comment

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.

+1

@kou
kou merged commit cc90e40 into apache:mainJul 25, 2026
109 of 115 checks passed
@koukou removed the awaiting change review Awaiting change review label Jul 25, 2026
@github-actionsgithub-actionsBot added the awaiting merge Awaiting merge label Jul 25, 2026
@Reranko05

Copy link
Copy Markdown
CollaboratorAuthor

@kou I reported this upstream as suggested. They closed the issue, since the macOS 11.3 SDK is considered an unsupported configuration in simdjson, so they don't plan to handle it there. We'll likely need to keep the workaround on the Arrow side.

@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit cc90e40.

There was 1 benchmark result indicating a performance regression:

The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting mergeAwaiting mergeCI: ExtraRun extra CIComponent: C++

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Reranko05@kou@rok