You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "tobim (via GitHub)" <gi...@apache.org> on 2024/03/07 08:20:08 UTC
[PR] GH-40398: [C++] Expose protobuf dependency if HDFS or ORC are enabled [arrow]
tobim opened a new pull request, #40399:
URL: https://github.com/apache/arrow/pull/40399
### Rationale for this change
When not bundling dependencies, `libarrow.a` depends on protobuf symbols if built with ORC or HDFS support. However this dependency was not looked up in ArrowConfig.cmake, causing client projects using linking to `arrow_static` to fail in configuration.
### What changes are included in this PR?
This change makes it so that the dependency on `protobuf` is propagated to the installed `ArrowConfig.cmake` file when necessary.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40398: [C++] Expose protobuf dependency if HDFS or ORC are enabled [arrow]
Posted by "tobim (via GitHub)" <gi...@apache.org>.
tobim commented on code in PR #40399:
URL: https://github.com/apache/arrow/pull/40399#discussion_r1516573059
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1812,12 +1812,14 @@ if(ARROW_WITH_PROTOBUF)
else()
set(ARROW_PROTOBUF_REQUIRED_VERSION "2.6.1")
endif()
- if(ARROW_FLIGHT)
- set(ARROW_PROTOBUF_ARROW_CMAKE_PACKAGE_NAME "ArrowFlight")
- set(ARROW_PROTOBUF_ARROW_PC_PACKAGE_NAME "arrow-flight")
- else()
+ if(ARROW_ORC
+ OR ARROW_HDFS
Review Comment:
Right, HDFS is not needed, but ORC and opentelemetry are.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40398: [C++] Expose protobuf dependency if HDFS or ORC are enabled [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40399:
URL: https://github.com/apache/arrow/pull/40399#issuecomment-1982893147
:warning: GitHub issue #40398 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40398: [C++] Expose protobuf dependency if opentelemetry or ORC are enabled [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #40399:
URL: https://github.com/apache/arrow/pull/40399#discussion_r1517365967
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1812,12 +1812,12 @@ if(ARROW_WITH_PROTOBUF)
else()
set(ARROW_PROTOBUF_REQUIRED_VERSION "2.6.1")
endif()
- if(ARROW_FLIGHT)
- set(ARROW_PROTOBUF_ARROW_CMAKE_PACKAGE_NAME "ArrowFlight")
- set(ARROW_PROTOBUF_ARROW_PC_PACKAGE_NAME "arrow-flight")
- else()
+ if(ARROW_ORC OR ARROW_WITH_OPENTELEMETRY)
set(ARROW_PROTOBUF_ARROW_CMAKE_PACKAGE_NAME "Arrow")
set(ARROW_PROTOBUF_ARROW_PC_PACKAGE_NAME "arrow")
+ elseif(ARROW_FLIGHT)
Review Comment:
OK. If you don't want to use `else()` here, could you add a new `else()` clause with `FATAL_ERROR` to detect unexpected case something like the following?
```cmake
if(ARROW_ORC OR ARROW_WITH_OPENTELEMETRY)
# ...
elseif(ARROW_FLIGHT)
# ...
else()
message(FATAL_ERROR "...")
endif()
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40398: [C++] Expose protobuf dependency if opentelemetry or ORC are enabled [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #40399:
URL: https://github.com/apache/arrow/pull/40399#issuecomment-1999625350
We can resolve these failures by rebasing on main. But I'll merge this.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40398: [C++] Expose protobuf dependency if opentelemetry or ORC are enabled [arrow]
Posted by "tobim (via GitHub)" <gi...@apache.org>.
tobim commented on code in PR #40399:
URL: https://github.com/apache/arrow/pull/40399#discussion_r1517359718
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1812,12 +1812,12 @@ if(ARROW_WITH_PROTOBUF)
else()
set(ARROW_PROTOBUF_REQUIRED_VERSION "2.6.1")
endif()
- if(ARROW_FLIGHT)
- set(ARROW_PROTOBUF_ARROW_CMAKE_PACKAGE_NAME "ArrowFlight")
- set(ARROW_PROTOBUF_ARROW_PC_PACKAGE_NAME "arrow-flight")
- else()
+ if(ARROW_ORC OR ARROW_WITH_OPENTELEMETRY)
set(ARROW_PROTOBUF_ARROW_CMAKE_PACKAGE_NAME "Arrow")
set(ARROW_PROTOBUF_ARROW_PC_PACKAGE_NAME "arrow")
+ elseif(ARROW_FLIGHT)
Review Comment:
I think that is too much implicit logic.
In theory it should even be a list of `ARROW_PROTOBUF_ARROW_PC_PACKAGE_NAMES`, but in practice it is not a problem because `arrow-flight` links to `arrow`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40398: [C++] Expose protobuf dependency if opentelemetry or ORC are enabled [arrow]
Posted by "tobim (via GitHub)" <gi...@apache.org>.
tobim commented on PR #40399:
URL: https://github.com/apache/arrow/pull/40399#issuecomment-1999440972
The CI failures seem unrelated. Anything I can do to move this forward?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40398: [C++] Expose protobuf dependency if opentelemetry or ORC are enabled [arrow]
Posted by "tobim (via GitHub)" <gi...@apache.org>.
tobim commented on code in PR #40399:
URL: https://github.com/apache/arrow/pull/40399#discussion_r1517390831
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1812,12 +1812,12 @@ if(ARROW_WITH_PROTOBUF)
else()
set(ARROW_PROTOBUF_REQUIRED_VERSION "2.6.1")
endif()
- if(ARROW_FLIGHT)
- set(ARROW_PROTOBUF_ARROW_CMAKE_PACKAGE_NAME "ArrowFlight")
- set(ARROW_PROTOBUF_ARROW_PC_PACKAGE_NAME "arrow-flight")
- else()
+ if(ARROW_ORC OR ARROW_WITH_OPENTELEMETRY)
set(ARROW_PROTOBUF_ARROW_CMAKE_PACKAGE_NAME "Arrow")
set(ARROW_PROTOBUF_ARROW_PC_PACKAGE_NAME "arrow")
+ elseif(ARROW_FLIGHT)
Review Comment:
This is a good idea. Added.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40398: [C++] Expose protobuf dependency if opentelemetry or ORC are enabled [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #40399:
URL: https://github.com/apache/arrow/pull/40399#issuecomment-2002054625
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 5baca0f16e924c42741729f041b31a02883548b9.
There were 2 benchmark results indicating a performance regression:
- Commit Run on `ursa-i9-9960x` at [2024-03-15 14:29:13Z](https://conbench.ursa.dev/compare/runs/5ffe4dc71b1545cf82829e8ad91f30e9...327b90b5aca74a7f970906ac439c36bc/)
- [`file-read` (R) with compression=lz4, dataset=nyctaxi_2010-01, file_type=feather, language=R, output_type=table](https://conbench.ursa.dev/compare/benchmarks/065f3dea8d2c7cc38000d7bde0365ce6...065f468e5eeb7dbc80008f95f161aa49)
- [`file-read` (R) with compression=uncompressed, dataset=nyctaxi_2010-01, file_type=feather, language=R, output_type=dataframe](https://conbench.ursa.dev/compare/benchmarks/065f3de9d6f873c48000642c34eeea86...065f468d3dd9749e8000f83ff5c80c24)
The [full Conbench report](https://github.com/apache/arrow/runs/22740614579) has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40398: [C++] Expose protobuf dependency if opentelemetry or ORC are enabled [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #40399:
URL: https://github.com/apache/arrow/pull/40399#discussion_r1517431886
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1812,12 +1812,12 @@ if(ARROW_WITH_PROTOBUF)
else()
set(ARROW_PROTOBUF_REQUIRED_VERSION "2.6.1")
endif()
- if(ARROW_FLIGHT)
- set(ARROW_PROTOBUF_ARROW_CMAKE_PACKAGE_NAME "ArrowFlight")
- set(ARROW_PROTOBUF_ARROW_PC_PACKAGE_NAME "arrow-flight")
- else()
+ if(ARROW_ORC OR ARROW_WITH_OPENTELEMETRY)
set(ARROW_PROTOBUF_ARROW_CMAKE_PACKAGE_NAME "Arrow")
set(ARROW_PROTOBUF_ARROW_PC_PACKAGE_NAME "arrow")
+ elseif(ARROW_FLIGHT)
Review Comment:
Thanks. Could you fix lint error?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40398: [C++] Expose protobuf dependency if HDFS or ORC are enabled [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #40399:
URL: https://github.com/apache/arrow/pull/40399#discussion_r1515832209
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1812,12 +1812,14 @@ if(ARROW_WITH_PROTOBUF)
else()
set(ARROW_PROTOBUF_REQUIRED_VERSION "2.6.1")
endif()
- if(ARROW_FLIGHT)
- set(ARROW_PROTOBUF_ARROW_CMAKE_PACKAGE_NAME "ArrowFlight")
- set(ARROW_PROTOBUF_ARROW_PC_PACKAGE_NAME "arrow-flight")
- else()
+ if(ARROW_ORC
+ OR ARROW_HDFS
Review Comment:
Hmm, do we need `ARROW_HDFS`? It seems that HDFS related codes don't use ProtoBuf.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40398: [C++] Expose protobuf dependency if opentelemetry or ORC are enabled [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #40399:
URL: https://github.com/apache/arrow/pull/40399#discussion_r1517088501
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -1812,12 +1812,12 @@ if(ARROW_WITH_PROTOBUF)
else()
set(ARROW_PROTOBUF_REQUIRED_VERSION "2.6.1")
endif()
- if(ARROW_FLIGHT)
- set(ARROW_PROTOBUF_ARROW_CMAKE_PACKAGE_NAME "ArrowFlight")
- set(ARROW_PROTOBUF_ARROW_PC_PACKAGE_NAME "arrow-flight")
- else()
+ if(ARROW_ORC OR ARROW_WITH_OPENTELEMETRY)
set(ARROW_PROTOBUF_ARROW_CMAKE_PACKAGE_NAME "Arrow")
set(ARROW_PROTOBUF_ARROW_PC_PACKAGE_NAME "arrow")
+ elseif(ARROW_FLIGHT)
Review Comment:
Can we simplify this?
```suggestion
else()
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40398: [C++] Expose protobuf dependency if opentelemetry or ORC are enabled [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #40399:
URL: https://github.com/apache/arrow/pull/40399
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org