ARROW-15700: [C++] Compilation error on Ubuntu 18.04 #12448

jvanstraten · 2022-02-17T12:04:35Z

Modified Substrait consumer interaction with libprotobuf to support versions down to 3.0.0, which is the minimum required due to Substrait's usage of proto3 syntax.

Tested locally with:

export ARROW_PROTOBUF_URL=https://github.com/protocolbuffers/protobuf/releases/download/v3.0.0/protobuf-cpp-3.0.0.tar.gz
cmake \
  --preset ninja-debug \
  -DProtobuf_SOURCE=BUNDLED \
  -DARROW_PROTOBUF_BUILD_VERSION=v3.0.0 \
  -DARROW_PROTOBUF_BUILD_SHA256_CHECKSUM=318e8f375fb4e5333975a40e0d1215e855b4a8c581d692eb0eb7df70db1a8d4e

(Is there an easier way to do this without modifying versions.txt or 751fb9d? Also, the env var is needed only because Google isn't at all consistent with their release file naming that far back.)

It'd also be nice to add this to CI, but it's probably excessive to always run for a PR, unless combined with some other run.

…rsions.txt

github-actions · 2022-02-17T12:04:59Z

https://issues.apache.org/jira/browse/ARROW-15700

github-actions · 2022-02-17T12:05:00Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

pitrou · 2022-02-17T12:30:42Z

cpp/src/arrow/engine/substrait/expression_internal.cc

+  // yet.
  template <typename ScalarWithBufferValue>
-  Status FromBuffer(void (substrait::Expression::Literal::*set)(std::string&&),
+  Status FromBuffer(void (substrait::Expression::Literal::*set)(const std::string&),


You could also take a lambda and let C++ resolve the best overload there:

template <typename LitteralSetter> Status FromBuffer(LitteralSetter&& set_lit, const ScalarWithBufferValue& scalar_with_buffer) { set_lit(lit_, scalar_with_buffer.value->ToString()); return Status::OK(); } Status Visit(const StringScalar& s) { return FromBuffer([](Lit* lit, std::string s) { lit->set_string(std::move(s)); }); }

That yields some positively ugly code IMO, but here it is: 7ffd90f

If the lambda is too ugly, we can also remove the FromBuffer helper, which will add at most one line of code to each caller. In both cases the bottom line is that we don't hardcode the actual protobuf method signature, so we're more robust against signature changes.

It's fine, for now anyway. IMO it's more important that builds start working on master again. If it gets annoying it can be cleaned up later.

pitrou · 2022-02-17T15:29:42Z

cpp/cmake_modules/ThirdpartyToolchain.cmake


-  # For debugging
-  message(STATUS "${_VARIABLE_NAME}: ${_VARIABLE_VALUE}")
+  # Allow values to be overridden to test specific versions during development


Do we really want to keep this snippet? Also @kou

It doesn't really belong in this PR, so I'm fine with removing it, but it does make it harder to verify compatibility with 3.0.0 for future changes manually or using CI (assuming there is not already a better way for overriding versions.txt).

Which protobuf version is in 18.04? That is exercised in nightly builds.

Huh, I thought I looked at this earlier and it was 3.12 or something, but I must have misread, because it's actually 3.0.0. So that's easy.

Reverted here: f3b23a0

This feature will be useful when when we use custom ARROW_XXX_URL environment variable to use other version.
But mixing environment variables and CMake variables confuses us. So another approach may be better.

pitrou · 2022-02-17T15:57:10Z

@github-actions crossbow submit -g cpp

github-actions · 2022-02-17T15:59:04Z

Revision: 7ffd90f

Submitted crossbow builds: ursacomputing/crossbow @ actions-1647

Task	Status
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-debian-10-cpp-amd64
test-debian-10-cpp-i386
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-33-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-21.04-cpp

jvanstraten · 2022-02-17T16:48:15Z

[ 22%] Generating generated/substrait/capabilities.pb.h
/build/cpp/src/arrow/engine/generated/: No such file or directory
make[2]: *** [src/arrow/engine/CMakeFiles/substrait_gen.dir/build.make:78: src/arrow/engine/generated/substrait/capabilities.pb.h] Error 1

https://github.com/ursacomputing/crossbow/runs/5234942880?check_suite_focus=true

That's a new one... Looking at it now.

…t directory

jvanstraten · 2022-02-17T17:19:20Z

Presumably 2144b75 will solve the above, but I wasn't able to reproduce the problem locally.

…nging versions.txt" This reverts commit 751fb9d.

jvanstraten · 2022-02-17T18:28:36Z

@github-actions crossbow submit -g cpp

github-actions · 2022-02-17T18:29:54Z

Revision: f3b23a0

Submitted crossbow builds: ursacomputing/crossbow @ actions-1650

Task	Status
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-debian-10-cpp-amd64
test-debian-10-cpp-i386
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-33-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-21.04-cpp

westonpace · 2022-02-17T18:31:49Z

cpp/src/arrow/engine/CMakeLists.txt

  list(APPEND SUBSTRAIT_SUPPRESSED_WARNINGS "/wd4251")
 endif()

+file(MAKE_DIRECTORY "${SUBSTRAIT_GEN_DIR}/substrait")


Why is this needed? Did the older version of protoc not create the output directory?

I'm not sure, to be honest. See failing crossbox build above, which I wasn't able to reproduce. I figured it can't hurt to explicitly create the directory, so why not.

CMake is sometimes finicky with non-existing directories, so I'm not surprised.

westonpace

Thanks for looking into this. It will be nice to support Ubuntu 18.04 out of the box.

pitrou

+1 as well

ursabot · 2022-02-17T21:41:05Z

Benchmark runs are scheduled for baseline = faef18b and contender = 7d16a78. 7d16a78 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.68% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.13% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Modified Substrait consumer interaction with libprotobuf to support versions down to 3.0.0, which is the minimum required due to Substrait's usage of proto3 syntax. Tested locally with: ``` export ARROW_PROTOBUF_URL=https://github.com/protocolbuffers/protobuf/releases/download/v3.0.0/protobuf-cpp-3.0.0.tar.gz cmake \ --preset ninja-debug \ -DProtobuf_SOURCE=BUNDLED \ -DARROW_PROTOBUF_BUILD_VERSION=v3.0.0 \ -DARROW_PROTOBUF_BUILD_SHA256_CHECKSUM=318e8f375fb4e5333975a40e0d1215e855b4a8c581d692eb0eb7df70db1a8d4e ``` (Is there an easier way to do this without modifying versions.txt or 751fb9d? Also, the env var is needed only because Google isn't at all consistent with their release file naming that far back.) It'd also be nice to add this to CI, but it's probably excessive to always run for a PR, unless combined with some other run. Closes apache#12448 from jvanstraten/ARROW-15700-Compilation-error-on-Ubuntu-18-04 Authored-by: Jeroen van Straten <[email protected]> Signed-off-by: Weston Pace <[email protected]> (cherry picked from commit 7d16a78)

@bkietz

* ARROW-15238: [C++] ARROW_ENGINE module with substrait consumer Continuation of apache#11707. I'm taking over from @bkietz for now because he's unavailable right now for personal reasons. Closes apache#12279 from jvanstraten/substrait-consumer Lead-authored-by: Benjamin Kietzman <[email protected]> Co-authored-by: Jeroen van Straten <[email protected]> Co-authored-by: Weston Pace <[email protected]> Signed-off-by: Weston Pace <[email protected]> * ARROW-15700: [C++] Compilation error on Ubuntu 18.04 Modified Substrait consumer interaction with libprotobuf to support versions down to 3.0.0, which is the minimum required due to Substrait's usage of proto3 syntax. Tested locally with: ``` export ARROW_PROTOBUF_URL=https://github.com/protocolbuffers/protobuf/releases/download/v3.0.0/protobuf-cpp-3.0.0.tar.gz cmake \ --preset ninja-debug \ -DProtobuf_SOURCE=BUNDLED \ -DARROW_PROTOBUF_BUILD_VERSION=v3.0.0 \ -DARROW_PROTOBUF_BUILD_SHA256_CHECKSUM=318e8f375fb4e5333975a40e0d1215e855b4a8c581d692eb0eb7df70db1a8d4e ``` (Is there an easier way to do this without modifying versions.txt or 751fb9d? Also, the env var is needed only because Google isn't at all consistent with their release file naming that far back.) It'd also be nice to add this to CI, but it's probably excessive to always run for a PR, unless combined with some other run. Closes apache#12448 from jvanstraten/ARROW-15700-Compilation-error-on-Ubuntu-18-04 Authored-by: Jeroen van Straten <[email protected]> Signed-off-by: Weston Pace <[email protected]> (cherry picked from commit 7d16a78) * ARROW-15258: [C++] Easy options to create a source node from a table This PR includes the addition of `TableSourceNode` to create a `ExecNode` easily using a table as the data source. ### TODO - [x] Fix test case for chunk_size Closes apache#12267 from vibhatha/arrow-15258-rb Authored-by: Vibhatha Abeykoon <[email protected]> Signed-off-by: Weston Pace <[email protected]> (cherry picked from commit fffdca2) * ARROW-15709: [C++] Compilation of ARROW_ENGINE fails if doing an "inline" build This should fix: - inline builds in general (ARROW-15709); - [weird stuff with inline builds causing non-tracked files to be deleted](https://github.com/apache/arrow;/pull/12444#issuecomment-1043143303) that the [previous fix](apache#12444) for the above [caused](apache#12454) - dependencies on git for downloading dependencies (ARROW-15760); - the build process for Substrait previously being treated as something too special to use Arrow's normal method for dealing with third-party dependencies (i.e. `ThirdpartyToolchain.cmake`) --- Initial attempt at making something functional to solve this issue properly. The use of `add_arrow_lib` in `ThirdpartyToolchain.cmake` is certainly odd, and I'm sure I'm not following best practices in that file in general. I could use some advice on what the proper way to do this would be. Some of the issues: - The CMake property specifying that a path refers to a generated file is scoped only to the current CMake file, so only moving the `externalproject_add` over to `ThirdpartyToolchain.cmake` resulted in the `add_arrow_lib` in `src/arrow/engine` failing due to missing source files. An object library didn't seem to resolve it either, and there are probably portability issues with that anyway, so that's why I ended up just using `add_arrow_lib`. - Unlike all the other third-party dependencies (AFAICT), Substrait can't currently be installed, so `Substrait_SOURCE=SYSTEM` makes no sense. In the end I just decided to override it, but that's probably not ideal. - Substrait doesn't have releases yet, so I had to resort to a git hash instead. </details> Closes apache#12457 from jvanstraten/ARROW-15709-Compilation-of-ARROW-ENGINE-fails-if-doi Authored-by: Jeroen van Straten <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]> (cherry picked from commit 89cc6b3) * ARROW-15830: [C++] Ensure target directory exists before running Substrait generation Closes apache#12548 from pitrou/ARROW-15830-ubuntu-cpp-bundled Authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]> (cherry picked from commit e989fb3) * ARROW-15850: [C++] Engine substrait headers missing from install Closes apache#12569 from westonpace/feature/ARROW-15850--substrait-headers-missing Authored-by: Weston Pace <[email protected]> Signed-off-by: David Li <[email protected]> (cherry picked from commit 8fce593) * Enable TPCH Q6 & Q1 Co-authored-by: Benjamin Kietzman <[email protected]> Co-authored-by: Jeroen van Straten <[email protected]> Co-authored-by: Weston Pace <[email protected]> Co-authored-by: Vibhatha Abeykoon <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]>

jvanstraten added 3 commits February 17, 2022 12:17

Fix Substrait consumer compilation for libprotobuf version 3.0.0

9550f0e

Demand at least libprotobuf 3.0.0 when ARROW_ENGINE is enabled

ee33fcf

Allow bundled toolchain versions to be overridden without changing ve…

751fb9d

…rsions.txt

github-actions bot added the Component: C++ label Feb 17, 2022

jvanstraten mentioned this pull request Feb 17, 2022

ARROW-15700: [C++] Compilation error on Ubuntu 18.04 #12446

Closed

pitrou reviewed Feb 17, 2022

View reviewed changes

Ensure string-like data is moved if protobuf version

7ffd90f

pitrou reviewed Feb 17, 2022

View reviewed changes

Attempt to solve inconsistent build failure due to non-existant outpu…

2144b75

…t directory

Revert "Allow bundled toolchain versions to be overridden without cha…

f3b23a0

…nging versions.txt" This reverts commit 751fb9d.

westonpace reviewed Feb 17, 2022

View reviewed changes

westonpace approved these changes Feb 17, 2022

View reviewed changes

pitrou approved these changes Feb 17, 2022

View reviewed changes

westonpace closed this in 7d16a78 Feb 17, 2022

ARROW-15700: [C++] Compilation error on Ubuntu 18.04 #12448

ARROW-15700: [C++] Compilation error on Ubuntu 18.04 #12448

Uh oh!

Conversation

jvanstraten commented Feb 17, 2022

Uh oh!

github-actions bot commented Feb 17, 2022

Uh oh!

github-actions bot commented Feb 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Feb 17, 2022

Uh oh!

github-actions bot commented Feb 17, 2022

Uh oh!

jvanstraten commented Feb 17, 2022

Uh oh!

jvanstraten commented Feb 17, 2022

Uh oh!

jvanstraten commented Feb 17, 2022

Uh oh!

github-actions bot commented Feb 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

ursabot commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pitrou Feb 17, 2022 •

edited

Loading

ursabot commented Feb 17, 2022 •

edited

Loading