GH-43095: [C++] Update bundled vendor/datetime to support for building with libc++ and C++20

[ben-freist]

Rationale for this change

We can't build with libc++ and C++20:

CMake command line:

cmake -DARROW_ENABLE_THREADING=OFF \
-DARROW_JEMALLOC=OFF \
-DCMAKE_CXX_STANDARD=20 \
-DCXX_ONLY_FLAGS="-stdlib=libc++" \
-DCMAKE_TOOLCHAIN_FILE=toolchain.cmake --preset ninja-debug-minimal ../cpp/

Error log:

In file included from ~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/tz.cpp:90:
~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/tz_private.h:295:12: error: use of overloaded operator '<<' is ambiguous (with operand types 'std::ostream' (aka 'basic_ostream<char>') and 'const sys_seconds' (aka 'const time_point<std::chrono::system_clock, std::chrono::duration<long long, std::ratio<1, 1>>>'))
295 | os << t.timepoint << "Z ";
| ~~ ^ ~~~~~~~~~~~
/usr/lib/llvm-17/bin/../include/c++/v1/__chrono/ostream.h:46:1: note: candidate function [with _CharT = char, _Traits = std::char_traits<char>, _Duration = std::chrono::duration<long long>]
46 | operator<<(basic_ostream<_CharT, _Traits>& __os, const sys_time<_Duration> __tp) {
| ^
~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/date.h:4214:1: note: candidate function [with CharT = char, Traits = std::char_traits<char>, Duration = std::chrono::duration<long long>]
4214 | operator<<(std::basic_ostream<CharT, Traits>& os, const sys_time<Duration>& tp)

What changes are included in this PR?

Update the bundled vendor/datetime because the upstream has changes for this case:
HowardHinnant/date#827

Are these changes tested?

Are there any user-facing changes?

• GitHub Issue: [C++] Failed to build vendored/datetime/tz.cpp with C++20 and libc++ #43095

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[github-actions]

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

[kou]

Hmm. Our C++20 CI job doesn't report this error: https://github.com/ursacomputing/crossbow/actions/runs/9736169142/job/26866418106

What is the difference between the job and you? g++ and clang++?

[ben-freist]

Thank you for the quick response! I can reproduce the behaviour like this

cmake -DARROW_ENABLE_THREADING=OFF \
-DARROW_JEMALLOC=OFF \
-DCMAKE_CXX_STANDARD=20 \
-DCXX_ONLY_FLAGS="-stdlib=libc++" \
-DCMAKE_TOOLCHAIN_FILE=toolchain.cmake --preset ninja-debug-minimal ../cpp/

Where toolchain.cmake looks like this:

set(CMAKE_SYSTEM_NAME Linux)
set(CMAKE_SYSTEM_PROCESSOR X86)
set(CMAKE_C_COMPILER clang-17 CACHE INTERNAL "C compiler")
set(CMAKE_CXX_COMPILER clang++-17 CACHE INTERNAL "C++ compiler")

Looks like libstdc++ doesn't have the stream insertion operators yet.

[ben-freist]

And it doesn't look like they are coming to libstdc++ soon, this compiles with gcc trunk, but fails when you select clang and use libc++: https://godbolt.org/z/ehnTdsK1e

Somehow reproducing it like this only works for clang-17, but the problem is also there for clang-18.

[kou]

OK.
The upstream of cpp/src/arrow/vendored/datetime/ is https://github.com/HowardHinnant/date . Could you fix this case in the upstream and then update our vendored version like #35612?

[ben-freist]

The library is part of the standard for c++ 20 and some compilers now, so I think the proper fix would be not to use it anymore.
Do you think it's good enough to handle it like this for now?

[kou]

The library is part of the standard for c++ 20 and some compilers now, so I think the proper fix would be not to use it anymore.

How can we do this? Does using arrow_vendored::date::year_month_day = std::chrono::year_month_day; or something work?

Do you think it's good enough to handle it like this for now?

What does "this" refer? Changing our bundled date library directly? If so, it's not acceptable. We don't want to maintain these changes in our side. We should upstream these changes.

[ben-freist]

I've bumped the version of vendored/datetime. At the moment the arrow specific patches are in a separate commit.
No changes in the upstream library were necessary.

[kou]

The upstream already has changes for this, right?

[ben-freist]

Yep, I think this is the relevant PR: HowardHinnant/date#827

[kou]

+1

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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

[ben-freist]

Hey,
thank you for merging this!
It would be nice if this change could be pushed to a new conan recipe. Do you know when this might happen?
Should I create a separate issue for that?

[kou]

We'll release 18.0.0 in this month. Then conan will add support for 18.0.0.

[ben-freist]

Rationale for this change

We can't build with libc++ and C++20:

CMake command line:

cmake -DARROW_ENABLE_THREADING=OFF \
-DARROW_JEMALLOC=OFF \
-DCMAKE_CXX_STANDARD=20 \
-DCXX_ONLY_FLAGS="-stdlib=libc++" \
-DCMAKE_TOOLCHAIN_FILE=toolchain.cmake --preset ninja-debug-minimal ../cpp/

Error log:

In file included from ~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/tz.cpp:90:
~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/tz_private.h:295:12: error: use of overloaded operator '<<' is ambiguous (with operand types 'std::ostream' (aka 'basic_ostream<char>') and 'const sys_seconds' (aka 'const time_point<std::chrono::system_clock, std::chrono::duration<long long, std::ratio<1, 1>>>'))
295 | os << t.timepoint << "Z ";
| ~~ ^ ~~~~~~~~~~~
/usr/lib/llvm-17/bin/../include/c++/v1/__chrono/ostream.h:46:1: note: candidate function [with _CharT = char, _Traits = std::char_traits<char>, _Duration = std::chrono::duration<long long>]
46 | operator<<(basic_ostream<_CharT, _Traits>& __os, const sys_time<_Duration> __tp) {
| ^
~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/date.h:4214:1: note: candidate function [with CharT = char, Traits = std::char_traits<char>, Duration = std::chrono::duration<long long>]
4214 | operator<<(std::basic_ostream<CharT, Traits>& os, const sys_time<Duration>& tp)

What changes are included in this PR?

Update the bundled vendor/datetime because the upstream has changes for this case:
HowardHinnant/date#827

Are these changes tested?

Are there any user-facing changes?

• GitHub Issue: [C++] Failed to build vendored/datetime/tz.cpp with C++20 and libc++ #43095

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[github-actions]

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

[kou]

Hmm. Our C++20 CI job doesn't report this error: https://github.com/ursacomputing/crossbow/actions/runs/9736169142/job/26866418106

What is the difference between the job and you? g++ and clang++?

[ben-freist]

Thank you for the quick response! I can reproduce the behaviour like this

cmake -DARROW_ENABLE_THREADING=OFF \
-DARROW_JEMALLOC=OFF \
-DCMAKE_CXX_STANDARD=20 \
-DCXX_ONLY_FLAGS="-stdlib=libc++" \
-DCMAKE_TOOLCHAIN_FILE=toolchain.cmake --preset ninja-debug-minimal ../cpp/

Where toolchain.cmake looks like this:

set(CMAKE_SYSTEM_NAME Linux)
set(CMAKE_SYSTEM_PROCESSOR X86)
set(CMAKE_C_COMPILER clang-17 CACHE INTERNAL "C compiler")
set(CMAKE_CXX_COMPILER clang++-17 CACHE INTERNAL "C++ compiler")

Looks like libstdc++ doesn't have the stream insertion operators yet.

[ben-freist]

And it doesn't look like they are coming to libstdc++ soon, this compiles with gcc trunk, but fails when you select clang and use libc++: https://godbolt.org/z/ehnTdsK1e

Somehow reproducing it like this only works for clang-17, but the problem is also there for clang-18.

[kou]

OK.
The upstream of cpp/src/arrow/vendored/datetime/ is https://github.com/HowardHinnant/date . Could you fix this case in the upstream and then update our vendored version like #35612?

[ben-freist]

The library is part of the standard for c++ 20 and some compilers now, so I think the proper fix would be not to use it anymore.
Do you think it's good enough to handle it like this for now?

[kou]

The library is part of the standard for c++ 20 and some compilers now, so I think the proper fix would be not to use it anymore.

How can we do this? Does using arrow_vendored::date::year_month_day = std::chrono::year_month_day; or something work?

Do you think it's good enough to handle it like this for now?

What does "this" refer? Changing our bundled date library directly? If so, it's not acceptable. We don't want to maintain these changes in our side. We should upstream these changes.

[ben-freist]

I've bumped the version of vendored/datetime. At the moment the arrow specific patches are in a separate commit.
No changes in the upstream library were necessary.

[kou]

The upstream already has changes for this, right?

[ben-freist]

Yep, I think this is the relevant PR: HowardHinnant/date#827

[kou]

+1

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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

[ben-freist]

Hey,
thank you for merging this!
It would be nice if this change could be pushed to a new conan recipe. Do you know when this might happen?
Should I create a separate issue for that?

[kou]

We'll release 18.0.0 in this month. Then conan will add support for 18.0.0.

[ben-freist]

Rationale for this change

We can't build with libc++ and C++20:

CMake command line:

cmake -DARROW_ENABLE_THREADING=OFF \
-DARROW_JEMALLOC=OFF \
-DCMAKE_CXX_STANDARD=20 \
-DCXX_ONLY_FLAGS="-stdlib=libc++" \
-DCMAKE_TOOLCHAIN_FILE=toolchain.cmake --preset ninja-debug-minimal ../cpp/

Error log:

In file included from ~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/tz.cpp:90:
~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/tz_private.h:295:12: error: use of overloaded operator '<<' is ambiguous (with operand types 'std::ostream' (aka 'basic_ostream<char>') and 'const sys_seconds' (aka 'const time_point<std::chrono::system_clock, std::chrono::duration<long long, std::ratio<1, 1>>>'))
295 | os << t.timepoint << "Z ";
| ~~ ^ ~~~~~~~~~~~
/usr/lib/llvm-17/bin/../include/c++/v1/__chrono/ostream.h:46:1: note: candidate function [with _CharT = char, _Traits = std::char_traits<char>, _Duration = std::chrono::duration<long long>]
46 | operator<<(basic_ostream<_CharT, _Traits>& __os, const sys_time<_Duration> __tp) {
| ^
~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/date.h:4214:1: note: candidate function [with CharT = char, Traits = std::char_traits<char>, Duration = std::chrono::duration<long long>]
4214 | operator<<(std::basic_ostream<CharT, Traits>& os, const sys_time<Duration>& tp)

What changes are included in this PR?

Update the bundled vendor/datetime because the upstream has changes for this case:
HowardHinnant/date#827

Are these changes tested?

Are there any user-facing changes?

• GitHub Issue: [C++] Failed to build vendored/datetime/tz.cpp with C++20 and libc++ #43095

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[github-actions]

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

[kou]

Hmm. Our C++20 CI job doesn't report this error: https://github.com/ursacomputing/crossbow/actions/runs/9736169142/job/26866418106

What is the difference between the job and you? g++ and clang++?

[ben-freist]

Thank you for the quick response! I can reproduce the behaviour like this

cmake -DARROW_ENABLE_THREADING=OFF \
-DARROW_JEMALLOC=OFF \
-DCMAKE_CXX_STANDARD=20 \
-DCXX_ONLY_FLAGS="-stdlib=libc++" \
-DCMAKE_TOOLCHAIN_FILE=toolchain.cmake --preset ninja-debug-minimal ../cpp/

Where toolchain.cmake looks like this:

set(CMAKE_SYSTEM_NAME Linux)
set(CMAKE_SYSTEM_PROCESSOR X86)
set(CMAKE_C_COMPILER clang-17 CACHE INTERNAL "C compiler")
set(CMAKE_CXX_COMPILER clang++-17 CACHE INTERNAL "C++ compiler")

Looks like libstdc++ doesn't have the stream insertion operators yet.

[ben-freist]

And it doesn't look like they are coming to libstdc++ soon, this compiles with gcc trunk, but fails when you select clang and use libc++: https://godbolt.org/z/ehnTdsK1e

Somehow reproducing it like this only works for clang-17, but the problem is also there for clang-18.

[kou]

OK.
The upstream of cpp/src/arrow/vendored/datetime/ is https://github.com/HowardHinnant/date . Could you fix this case in the upstream and then update our vendored version like #35612?

[ben-freist]

The library is part of the standard for c++ 20 and some compilers now, so I think the proper fix would be not to use it anymore.
Do you think it's good enough to handle it like this for now?

[kou]

The library is part of the standard for c++ 20 and some compilers now, so I think the proper fix would be not to use it anymore.

How can we do this? Does using arrow_vendored::date::year_month_day = std::chrono::year_month_day; or something work?

Do you think it's good enough to handle it like this for now?

What does "this" refer? Changing our bundled date library directly? If so, it's not acceptable. We don't want to maintain these changes in our side. We should upstream these changes.

[ben-freist]

I've bumped the version of vendored/datetime. At the moment the arrow specific patches are in a separate commit.
No changes in the upstream library were necessary.

[kou]

The upstream already has changes for this, right?

[ben-freist]

Yep, I think this is the relevant PR: HowardHinnant/date#827

[kou]

+1

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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

[ben-freist]

Hey,
thank you for merging this!
It would be nice if this change could be pushed to a new conan recipe. Do you know when this might happen?
Should I create a separate issue for that?

[kou]

We'll release 18.0.0 in this month. Then conan will add support for 18.0.0.

[ben-freist]

Rationale for this change

We can't build with libc++ and C++20:

CMake command line:

cmake -DARROW_ENABLE_THREADING=OFF \
-DARROW_JEMALLOC=OFF \
-DCMAKE_CXX_STANDARD=20 \
-DCXX_ONLY_FLAGS="-stdlib=libc++" \
-DCMAKE_TOOLCHAIN_FILE=toolchain.cmake --preset ninja-debug-minimal ../cpp/

Error log:

In file included from ~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/tz.cpp:90:
~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/tz_private.h:295:12: error: use of overloaded operator '<<' is ambiguous (with operand types 'std::ostream' (aka 'basic_ostream<char>') and 'const sys_seconds' (aka 'const time_point<std::chrono::system_clock, std::chrono::duration<long long, std::ratio<1, 1>>>'))
295 | os << t.timepoint << "Z ";
| ~~ ^ ~~~~~~~~~~~
/usr/lib/llvm-17/bin/../include/c++/v1/__chrono/ostream.h:46:1: note: candidate function [with _CharT = char, _Traits = std::char_traits<char>, _Duration = std::chrono::duration<long long>]
46 | operator<<(basic_ostream<_CharT, _Traits>& __os, const sys_time<_Duration> __tp) {
| ^
~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/date.h:4214:1: note: candidate function [with CharT = char, Traits = std::char_traits<char>, Duration = std::chrono::duration<long long>]
4214 | operator<<(std::basic_ostream<CharT, Traits>& os, const sys_time<Duration>& tp)

What changes are included in this PR?

Update the bundled vendor/datetime because the upstream has changes for this case:
HowardHinnant/date#827

Are these changes tested?

Are there any user-facing changes?

• GitHub Issue: [C++] Failed to build vendored/datetime/tz.cpp with C++20 and libc++ #43095

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[github-actions]

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

[kou]

Hmm. Our C++20 CI job doesn't report this error: https://github.com/ursacomputing/crossbow/actions/runs/9736169142/job/26866418106

What is the difference between the job and you? g++ and clang++?

[ben-freist]

Thank you for the quick response! I can reproduce the behaviour like this

cmake -DARROW_ENABLE_THREADING=OFF \
-DARROW_JEMALLOC=OFF \
-DCMAKE_CXX_STANDARD=20 \
-DCXX_ONLY_FLAGS="-stdlib=libc++" \
-DCMAKE_TOOLCHAIN_FILE=toolchain.cmake --preset ninja-debug-minimal ../cpp/

Where toolchain.cmake looks like this:

set(CMAKE_SYSTEM_NAME Linux)
set(CMAKE_SYSTEM_PROCESSOR X86)
set(CMAKE_C_COMPILER clang-17 CACHE INTERNAL "C compiler")
set(CMAKE_CXX_COMPILER clang++-17 CACHE INTERNAL "C++ compiler")

Looks like libstdc++ doesn't have the stream insertion operators yet.

[ben-freist]

And it doesn't look like they are coming to libstdc++ soon, this compiles with gcc trunk, but fails when you select clang and use libc++: https://godbolt.org/z/ehnTdsK1e

Somehow reproducing it like this only works for clang-17, but the problem is also there for clang-18.

[kou]

OK.
The upstream of cpp/src/arrow/vendored/datetime/ is https://github.com/HowardHinnant/date . Could you fix this case in the upstream and then update our vendored version like #35612?

[ben-freist]

The library is part of the standard for c++ 20 and some compilers now, so I think the proper fix would be not to use it anymore.
Do you think it's good enough to handle it like this for now?

[kou]

The library is part of the standard for c++ 20 and some compilers now, so I think the proper fix would be not to use it anymore.

How can we do this? Does using arrow_vendored::date::year_month_day = std::chrono::year_month_day; or something work?

Do you think it's good enough to handle it like this for now?

What does "this" refer? Changing our bundled date library directly? If so, it's not acceptable. We don't want to maintain these changes in our side. We should upstream these changes.

[ben-freist]

I've bumped the version of vendored/datetime. At the moment the arrow specific patches are in a separate commit.
No changes in the upstream library were necessary.

[kou]

The upstream already has changes for this, right?

[ben-freist]

Yep, I think this is the relevant PR: HowardHinnant/date#827

[kou]

+1

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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

[ben-freist]

Hey,
thank you for merging this!
It would be nice if this change could be pushed to a new conan recipe. Do you know when this might happen?
Should I create a separate issue for that?

[kou]

We'll release 18.0.0 in this month. Then conan will add support for 18.0.0.

[ben-freist]

Rationale for this change

We can't build with libc++ and C++20:

CMake command line:

cmake -DARROW_ENABLE_THREADING=OFF \
-DARROW_JEMALLOC=OFF \
-DCMAKE_CXX_STANDARD=20 \
-DCXX_ONLY_FLAGS="-stdlib=libc++" \
-DCMAKE_TOOLCHAIN_FILE=toolchain.cmake --preset ninja-debug-minimal ../cpp/

Error log:

In file included from ~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/tz.cpp:90:
~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/tz_private.h:295:12: error: use of overloaded operator '<<' is ambiguous (with operand types 'std::ostream' (aka 'basic_ostream<char>') and 'const sys_seconds' (aka 'const time_point<std::chrono::system_clock, std::chrono::duration<long long, std::ratio<1, 1>>>'))
295 | os << t.timepoint << "Z ";
| ~~ ^ ~~~~~~~~~~~
/usr/lib/llvm-17/bin/../include/c++/v1/__chrono/ostream.h:46:1: note: candidate function [with _CharT = char, _Traits = std::char_traits<char>, _Duration = std::chrono::duration<long long>]
46 | operator<<(basic_ostream<_CharT, _Traits>& __os, const sys_time<_Duration> __tp) {
| ^
~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/date.h:4214:1: note: candidate function [with CharT = char, Traits = std::char_traits<char>, Duration = std::chrono::duration<long long>]
4214 | operator<<(std::basic_ostream<CharT, Traits>& os, const sys_time<Duration>& tp)

What changes are included in this PR?

Update the bundled vendor/datetime because the upstream has changes for this case:
HowardHinnant/date#827

Are these changes tested?

Are there any user-facing changes?

• GitHub Issue: [C++] Failed to build vendored/datetime/tz.cpp with C++20 and libc++ #43095

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[github-actions]

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

[kou]

Hmm. Our C++20 CI job doesn't report this error: https://github.com/ursacomputing/crossbow/actions/runs/9736169142/job/26866418106

What is the difference between the job and you? g++ and clang++?

[ben-freist]

Thank you for the quick response! I can reproduce the behaviour like this

cmake -DARROW_ENABLE_THREADING=OFF \
-DARROW_JEMALLOC=OFF \
-DCMAKE_CXX_STANDARD=20 \
-DCXX_ONLY_FLAGS="-stdlib=libc++" \
-DCMAKE_TOOLCHAIN_FILE=toolchain.cmake --preset ninja-debug-minimal ../cpp/

Where toolchain.cmake looks like this:

set(CMAKE_SYSTEM_NAME Linux)
set(CMAKE_SYSTEM_PROCESSOR X86)
set(CMAKE_C_COMPILER clang-17 CACHE INTERNAL "C compiler")
set(CMAKE_CXX_COMPILER clang++-17 CACHE INTERNAL "C++ compiler")

Looks like libstdc++ doesn't have the stream insertion operators yet.

[ben-freist]

And it doesn't look like they are coming to libstdc++ soon, this compiles with gcc trunk, but fails when you select clang and use libc++: https://godbolt.org/z/ehnTdsK1e

Somehow reproducing it like this only works for clang-17, but the problem is also there for clang-18.

[kou]

OK.
The upstream of cpp/src/arrow/vendored/datetime/ is https://github.com/HowardHinnant/date . Could you fix this case in the upstream and then update our vendored version like #35612?

[ben-freist]

The library is part of the standard for c++ 20 and some compilers now, so I think the proper fix would be not to use it anymore.
Do you think it's good enough to handle it like this for now?

[kou]

The library is part of the standard for c++ 20 and some compilers now, so I think the proper fix would be not to use it anymore.

How can we do this? Does using arrow_vendored::date::year_month_day = std::chrono::year_month_day; or something work?

Do you think it's good enough to handle it like this for now?

What does "this" refer? Changing our bundled date library directly? If so, it's not acceptable. We don't want to maintain these changes in our side. We should upstream these changes.

[ben-freist]

I've bumped the version of vendored/datetime. At the moment the arrow specific patches are in a separate commit.
No changes in the upstream library were necessary.

[kou]

The upstream already has changes for this, right?

[ben-freist]

Yep, I think this is the relevant PR: HowardHinnant/date#827

[kou]

+1

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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

[ben-freist]

Hey,
thank you for merging this!
It would be nice if this change could be pushed to a new conan recipe. Do you know when this might happen?
Should I create a separate issue for that?

[kou]

We'll release 18.0.0 in this month. Then conan will add support for 18.0.0.

[ben-freist]

Rationale for this change

We can't build with libc++ and C++20:

CMake command line:

cmake -DARROW_ENABLE_THREADING=OFF \
-DARROW_JEMALLOC=OFF \
-DCMAKE_CXX_STANDARD=20 \
-DCXX_ONLY_FLAGS="-stdlib=libc++" \
-DCMAKE_TOOLCHAIN_FILE=toolchain.cmake --preset ninja-debug-minimal ../cpp/

Error log:

In file included from ~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/tz.cpp:90:
~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/tz_private.h:295:12: error: use of overloaded operator '<<' is ambiguous (with operand types 'std::ostream' (aka 'basic_ostream<char>') and 'const sys_seconds' (aka 'const time_point<std::chrono::system_clock, std::chrono::duration<long long, std::ratio<1, 1>>>'))
295 | os << t.timepoint << "Z ";
| ~~ ^ ~~~~~~~~~~~
/usr/lib/llvm-17/bin/../include/c++/v1/__chrono/ostream.h:46:1: note: candidate function [with _CharT = char, _Traits = std::char_traits<char>, _Duration = std::chrono::duration<long long>]
46 | operator<<(basic_ostream<_CharT, _Traits>& __os, const sys_time<_Duration> __tp) {
| ^
~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/date.h:4214:1: note: candidate function [with CharT = char, Traits = std::char_traits<char>, Duration = std::chrono::duration<long long>]
4214 | operator<<(std::basic_ostream<CharT, Traits>& os, const sys_time<Duration>& tp)

What changes are included in this PR?

Update the bundled vendor/datetime because the upstream has changes for this case:
HowardHinnant/date#827

Are these changes tested?

Are there any user-facing changes?

• GitHub Issue: [C++] Failed to build vendored/datetime/tz.cpp with C++20 and libc++ #43095

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[github-actions]

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

[kou]

Hmm. Our C++20 CI job doesn't report this error: https://github.com/ursacomputing/crossbow/actions/runs/9736169142/job/26866418106

What is the difference between the job and you? g++ and clang++?

[ben-freist]

Thank you for the quick response! I can reproduce the behaviour like this

cmake -DARROW_ENABLE_THREADING=OFF \
-DARROW_JEMALLOC=OFF \
-DCMAKE_CXX_STANDARD=20 \
-DCXX_ONLY_FLAGS="-stdlib=libc++" \
-DCMAKE_TOOLCHAIN_FILE=toolchain.cmake --preset ninja-debug-minimal ../cpp/

Where toolchain.cmake looks like this:

set(CMAKE_SYSTEM_NAME Linux)
set(CMAKE_SYSTEM_PROCESSOR X86)
set(CMAKE_C_COMPILER clang-17 CACHE INTERNAL "C compiler")
set(CMAKE_CXX_COMPILER clang++-17 CACHE INTERNAL "C++ compiler")

Looks like libstdc++ doesn't have the stream insertion operators yet.

[ben-freist]

And it doesn't look like they are coming to libstdc++ soon, this compiles with gcc trunk, but fails when you select clang and use libc++: https://godbolt.org/z/ehnTdsK1e

Somehow reproducing it like this only works for clang-17, but the problem is also there for clang-18.

[kou]

OK.
The upstream of cpp/src/arrow/vendored/datetime/ is https://github.com/HowardHinnant/date . Could you fix this case in the upstream and then update our vendored version like #35612?

[ben-freist]

The library is part of the standard for c++ 20 and some compilers now, so I think the proper fix would be not to use it anymore.
Do you think it's good enough to handle it like this for now?

[kou]

The library is part of the standard for c++ 20 and some compilers now, so I think the proper fix would be not to use it anymore.

How can we do this? Does using arrow_vendored::date::year_month_day = std::chrono::year_month_day; or something work?

Do you think it's good enough to handle it like this for now?

What does "this" refer? Changing our bundled date library directly? If so, it's not acceptable. We don't want to maintain these changes in our side. We should upstream these changes.

[ben-freist]

I've bumped the version of vendored/datetime. At the moment the arrow specific patches are in a separate commit.
No changes in the upstream library were necessary.

[kou]

The upstream already has changes for this, right?

[ben-freist]

Yep, I think this is the relevant PR: HowardHinnant/date#827

[kou]

+1

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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

[ben-freist]

Hey,
thank you for merging this!
It would be nice if this change could be pushed to a new conan recipe. Do you know when this might happen?
Should I create a separate issue for that?

[kou]

We'll release 18.0.0 in this month. Then conan will add support for 18.0.0.

[ben-freist]

Rationale for this change

We can't build with libc++ and C++20:

CMake command line:

cmake -DARROW_ENABLE_THREADING=OFF \
-DARROW_JEMALLOC=OFF \
-DCMAKE_CXX_STANDARD=20 \
-DCXX_ONLY_FLAGS="-stdlib=libc++" \
-DCMAKE_TOOLCHAIN_FILE=toolchain.cmake --preset ninja-debug-minimal ../cpp/

Error log:

In file included from ~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/tz.cpp:90:
~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/tz_private.h:295:12: error: use of overloaded operator '<<' is ambiguous (with operand types 'std::ostream' (aka 'basic_ostream<char>') and 'const sys_seconds' (aka 'const time_point<std::chrono::system_clock, std::chrono::duration<long long, std::ratio<1, 1>>>'))
295 | os << t.timepoint << "Z ";
| ~~ ^ ~~~~~~~~~~~
/usr/lib/llvm-17/bin/../include/c++/v1/__chrono/ostream.h:46:1: note: candidate function [with _CharT = char, _Traits = std::char_traits<char>, _Duration = std::chrono::duration<long long>]
46 | operator<<(basic_ostream<_CharT, _Traits>& __os, const sys_time<_Duration> __tp) {
| ^
~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/date.h:4214:1: note: candidate function [with CharT = char, Traits = std::char_traits<char>, Duration = std::chrono::duration<long long>]
4214 | operator<<(std::basic_ostream<CharT, Traits>& os, const sys_time<Duration>& tp)

What changes are included in this PR?

Update the bundled vendor/datetime because the upstream has changes for this case:
HowardHinnant/date#827

Are these changes tested?

Are there any user-facing changes?

• GitHub Issue: [C++] Failed to build vendored/datetime/tz.cpp with C++20 and libc++ #43095

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[github-actions]

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

[kou]

Hmm. Our C++20 CI job doesn't report this error: https://github.com/ursacomputing/crossbow/actions/runs/9736169142/job/26866418106

What is the difference between the job and you? g++ and clang++?

[ben-freist]

Thank you for the quick response! I can reproduce the behaviour like this

cmake -DARROW_ENABLE_THREADING=OFF \
-DARROW_JEMALLOC=OFF \
-DCMAKE_CXX_STANDARD=20 \
-DCXX_ONLY_FLAGS="-stdlib=libc++" \
-DCMAKE_TOOLCHAIN_FILE=toolchain.cmake --preset ninja-debug-minimal ../cpp/

Where toolchain.cmake looks like this:

set(CMAKE_SYSTEM_NAME Linux)
set(CMAKE_SYSTEM_PROCESSOR X86)
set(CMAKE_C_COMPILER clang-17 CACHE INTERNAL "C compiler")
set(CMAKE_CXX_COMPILER clang++-17 CACHE INTERNAL "C++ compiler")

Looks like libstdc++ doesn't have the stream insertion operators yet.

[ben-freist]

And it doesn't look like they are coming to libstdc++ soon, this compiles with gcc trunk, but fails when you select clang and use libc++: https://godbolt.org/z/ehnTdsK1e

Somehow reproducing it like this only works for clang-17, but the problem is also there for clang-18.

[kou]

OK.
The upstream of cpp/src/arrow/vendored/datetime/ is https://github.com/HowardHinnant/date . Could you fix this case in the upstream and then update our vendored version like #35612?

[ben-freist]

The library is part of the standard for c++ 20 and some compilers now, so I think the proper fix would be not to use it anymore.
Do you think it's good enough to handle it like this for now?

[kou]

The library is part of the standard for c++ 20 and some compilers now, so I think the proper fix would be not to use it anymore.

How can we do this? Does using arrow_vendored::date::year_month_day = std::chrono::year_month_day; or something work?

Do you think it's good enough to handle it like this for now?

What does "this" refer? Changing our bundled date library directly? If so, it's not acceptable. We don't want to maintain these changes in our side. We should upstream these changes.

[ben-freist]

I've bumped the version of vendored/datetime. At the moment the arrow specific patches are in a separate commit.
No changes in the upstream library were necessary.

[kou]

The upstream already has changes for this, right?

[ben-freist]

Yep, I think this is the relevant PR: HowardHinnant/date#827

[kou]

+1

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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

[ben-freist]

Hey,
thank you for merging this!
It would be nice if this change could be pushed to a new conan recipe. Do you know when this might happen?
Should I create a separate issue for that?

[kou]

We'll release 18.0.0 in this month. Then conan will add support for 18.0.0.

[ben-freist]

Rationale for this change

We can't build with libc++ and C++20:

CMake command line:

cmake -DARROW_ENABLE_THREADING=OFF \
-DARROW_JEMALLOC=OFF \
-DCMAKE_CXX_STANDARD=20 \
-DCXX_ONLY_FLAGS="-stdlib=libc++" \
-DCMAKE_TOOLCHAIN_FILE=toolchain.cmake --preset ninja-debug-minimal ../cpp/

Error log:

In file included from ~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/tz.cpp:90:
~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/tz_private.h:295:12: error: use of overloaded operator '<<' is ambiguous (with operand types 'std::ostream' (aka 'basic_ostream<char>') and 'const sys_seconds' (aka 'const time_point<std::chrono::system_clock, std::chrono::duration<long long, std::ratio<1, 1>>>'))
295 | os << t.timepoint << "Z ";
| ~~ ^ ~~~~~~~~~~~
/usr/lib/llvm-17/bin/../include/c++/v1/__chrono/ostream.h:46:1: note: candidate function [with _CharT = char, _Traits = std::char_traits<char>, _Duration = std::chrono::duration<long long>]
46 | operator<<(basic_ostream<_CharT, _Traits>& __os, const sys_time<_Duration> __tp) {
| ^
~/.conan2/p/b/arrowe39f77e638649/b/src/cpp/src/arrow/vendored/datetime/date.h:4214:1: note: candidate function [with CharT = char, Traits = std::char_traits<char>, Duration = std::chrono::duration<long long>]
4214 | operator<<(std::basic_ostream<CharT, Traits>& os, const sys_time<Duration>& tp)

What changes are included in this PR?

Update the bundled vendor/datetime because the upstream has changes for this case:
HowardHinnant/date#827

Are these changes tested?

Are there any user-facing changes?

• GitHub Issue: [C++] Failed to build vendored/datetime/tz.cpp with C++20 and libc++ #43095

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[github-actions]

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

[kou]

Hmm. Our C++20 CI job doesn't report this error: https://github.com/ursacomputing/crossbow/actions/runs/9736169142/job/26866418106

What is the difference between the job and you? g++ and clang++?

[ben-freist]

Thank you for the quick response! I can reproduce the behaviour like this

cmake -DARROW_ENABLE_THREADING=OFF \
-DARROW_JEMALLOC=OFF \
-DCMAKE_CXX_STANDARD=20 \
-DCXX_ONLY_FLAGS="-stdlib=libc++" \
-DCMAKE_TOOLCHAIN_FILE=toolchain.cmake --preset ninja-debug-minimal ../cpp/

Where toolchain.cmake looks like this:

set(CMAKE_SYSTEM_NAME Linux)
set(CMAKE_SYSTEM_PROCESSOR X86)
set(CMAKE_C_COMPILER clang-17 CACHE INTERNAL "C compiler")
set(CMAKE_CXX_COMPILER clang++-17 CACHE INTERNAL "C++ compiler")

Looks like libstdc++ doesn't have the stream insertion operators yet.

[ben-freist]

And it doesn't look like they are coming to libstdc++ soon, this compiles with gcc trunk, but fails when you select clang and use libc++: https://godbolt.org/z/ehnTdsK1e

Somehow reproducing it like this only works for clang-17, but the problem is also there for clang-18.

[kou]

OK.
The upstream of cpp/src/arrow/vendored/datetime/ is https://github.com/HowardHinnant/date . Could you fix this case in the upstream and then update our vendored version like #35612?

[ben-freist]

The library is part of the standard for c++ 20 and some compilers now, so I think the proper fix would be not to use it anymore.
Do you think it's good enough to handle it like this for now?

[kou]

The library is part of the standard for c++ 20 and some compilers now, so I think the proper fix would be not to use it anymore.

How can we do this? Does using arrow_vendored::date::year_month_day = std::chrono::year_month_day; or something work?

Do you think it's good enough to handle it like this for now?

What does "this" refer? Changing our bundled date library directly? If so, it's not acceptable. We don't want to maintain these changes in our side. We should upstream these changes.

[ben-freist]

I've bumped the version of vendored/datetime. At the moment the arrow specific patches are in a separate commit.
No changes in the upstream library were necessary.

[kou]

The upstream already has changes for this, right?

[ben-freist]

Yep, I think this is the relevant PR: HowardHinnant/date#827

[kou]

+1

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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

[ben-freist]

Hey,
thank you for merging this!
It would be nice if this change could be pushed to a new conan recipe. Do you know when this might happen?
Should I create a separate issue for that?

[kou]

We'll release 18.0.0 in this month. Then conan will add support for 18.0.0.