You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "h-vetinari (via GitHub)" <gi...@apache.org> on 2024/05/21 21:36:17 UTC
[PR] teach CheckTimeZoneDatabaseAvailability how to inspect conda environm… [arrow]
h-vetinari opened a new pull request, #41767:
URL: https://github.com/apache/arrow/pull/41767
Fixes https://github.com/apache/arrow/issues/41755
Deployed in conda-forge through https://github.com/conda-forge/arrow-cpp-feedstock/pull/1424 and confirmed as working in https://github.com/conda-forge/pyarrow-feedstock/pull/122
--
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-41755: [C++][ORC] Teach CheckTimeZoneDatabaseAvailability how to inspect conda environments [arrow]
Posted by "h-vetinari (via GitHub)" <gi...@apache.org>.
h-vetinari commented on code in PR #41767:
URL: https://github.com/apache/arrow/pull/41767#discussion_r1609205580
##########
cpp/src/arrow/adapters/orc/adapter.cc:
##########
@@ -189,15 +189,21 @@ liborc::RowReaderOptions DefaultRowReaderOptions() {
#ifdef ARROW_ORC_NEED_TIME_ZONE_DATABASE_CHECK
// Proactively check timezone database availability for ORC versions older than 2.0.0
Status CheckTimeZoneDatabaseAvailability() {
- auto tz_dir = std::getenv("TZDIR");
- bool is_tzdb_avaiable = tz_dir != nullptr
- ? std::filesystem::exists(tz_dir)
- : std::filesystem::exists("/usr/share/zoneinfo");
- if (!is_tzdb_avaiable) {
+ // orc >=2.0.1 will look for tzdb in $CONDA_PREFIX/share/zoneinfo,
+ // which is provided by the package `tzdata` (if installed)
+ auto conda_prefix = std::getenv("CONDA_PREFIX");
+ auto tz_dir_raw = std::getenv("TZDIR");
+ std::string tz_dir = conda_prefix != nullptr
+ ? std::string(conda_prefix) + "/share/zoneinfo"
+ : std::string(tz_dir_raw != nullptr ? tz_dir_raw : "");
+ bool is_tzdb_available = (!tz_dir.empty())
+ ? std::filesystem::exists(tz_dir)
+ : std::filesystem::exists("/usr/share/zoneinfo");
+ if (!is_tzdb_available) {
return Status::Invalid(
"IANA time zone database is unavailable but required by ORC."
" Please install it to /usr/share/zoneinfo or set TZDIR env to the installed"
- " directory");
+ " directory. If you are using conda, simply install the package `tzdata`.");
Review Comment:
I don't understand how that would be annoying TBH. It just adds one more way to fix the problem, and the user can choose the appropriate solution. I mean, I can do what you ask, but I feel it's implementation complexity for very little (or even negative) gain.
--
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-41755: teach CheckTimeZoneDatabaseAvailability how to inspect conda environments [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41767:
URL: https://github.com/apache/arrow/pull/41767#issuecomment-2123483373
:warning: GitHub issue #41755 **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-41755: [C++][ORC] Teach CheckTimeZoneDatabaseAvailability how to inspect conda environments [arrow]
Posted by "h-vetinari (via GitHub)" <gi...@apache.org>.
h-vetinari commented on code in PR #41767:
URL: https://github.com/apache/arrow/pull/41767#discussion_r1609284384
##########
cpp/src/arrow/adapters/orc/adapter.cc:
##########
@@ -189,15 +189,21 @@ liborc::RowReaderOptions DefaultRowReaderOptions() {
#ifdef ARROW_ORC_NEED_TIME_ZONE_DATABASE_CHECK
// Proactively check timezone database availability for ORC versions older than 2.0.0
Status CheckTimeZoneDatabaseAvailability() {
- auto tz_dir = std::getenv("TZDIR");
- bool is_tzdb_avaiable = tz_dir != nullptr
- ? std::filesystem::exists(tz_dir)
- : std::filesystem::exists("/usr/share/zoneinfo");
- if (!is_tzdb_avaiable) {
+ // orc >=2.0.1 will look for tzdb in $CONDA_PREFIX/share/zoneinfo,
+ // which is provided by the package `tzdata` (if installed)
+ auto conda_prefix = std::getenv("CONDA_PREFIX");
+ auto tz_dir_raw = std::getenv("TZDIR");
+ std::string tz_dir = conda_prefix != nullptr
+ ? std::string(conda_prefix) + "/share/zoneinfo"
+ : std::string(tz_dir_raw != nullptr ? tz_dir_raw : "");
+ bool is_tzdb_available = (!tz_dir.empty())
+ ? std::filesystem::exists(tz_dir)
+ : std::filesystem::exists("/usr/share/zoneinfo");
+ if (!is_tzdb_available) {
return Status::Invalid(
"IANA time zone database is unavailable but required by ORC."
" Please install it to /usr/share/zoneinfo or set TZDIR env to the installed"
- " directory");
+ " directory. If you are using conda, simply install the package `tzdata`.");
Review Comment:
> I don't know why this is happen with conda.
I described it in the issue (#41755) and in #36026: it's due to https://github.com/apache/orc/pull/1882, which was the only way to avoid injecting `TZDIR` as an environment variable into every conda environment using arrow on windows, which would have been very intrusive.
> Can we add `tzdata` to dependencies of `arrow-cpp` or `orc` conda packages?
That already happened in https://github.com/conda-forge/pyarrow-feedstock/pull/122 (and backports to 15.x, 14.x, 13.x). In general, someone needs to sync the feedstocks back to the repo here, but that's unrelated to this PR.
--
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-41755: [C++][ORC] Teach CheckTimeZoneDatabaseAvailability how to inspect conda environments [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #41767:
URL: https://github.com/apache/arrow/pull/41767#discussion_r1609170136
##########
cpp/src/arrow/adapters/orc/adapter.cc:
##########
@@ -189,15 +189,21 @@ liborc::RowReaderOptions DefaultRowReaderOptions() {
#ifdef ARROW_ORC_NEED_TIME_ZONE_DATABASE_CHECK
// Proactively check timezone database availability for ORC versions older than 2.0.0
Status CheckTimeZoneDatabaseAvailability() {
- auto tz_dir = std::getenv("TZDIR");
- bool is_tzdb_avaiable = tz_dir != nullptr
- ? std::filesystem::exists(tz_dir)
- : std::filesystem::exists("/usr/share/zoneinfo");
- if (!is_tzdb_avaiable) {
+ // orc >=2.0.1 will look for tzdb in $CONDA_PREFIX/share/zoneinfo,
+ // which is provided by the package `tzdata` (if installed)
+ auto conda_prefix = std::getenv("CONDA_PREFIX");
+ auto tz_dir_raw = std::getenv("TZDIR");
+ std::string tz_dir = conda_prefix != nullptr
+ ? std::string(conda_prefix) + "/share/zoneinfo"
+ : std::string(tz_dir_raw != nullptr ? tz_dir_raw : "");
+ bool is_tzdb_available = (!tz_dir.empty())
+ ? std::filesystem::exists(tz_dir)
+ : std::filesystem::exists("/usr/share/zoneinfo");
+ if (!is_tzdb_available) {
return Status::Invalid(
"IANA time zone database is unavailable but required by ORC."
" Please install it to /usr/share/zoneinfo or set TZDIR env to the installed"
- " directory");
+ " directory. If you are using conda, simply install the package `tzdata`.");
Review Comment:
IMHO, it would be annoying to non-conda users to see this irrelevant error message. Perhaps we can add this only when `CONDA_PREFIX` is set but TZDB is not installed?
--
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-41755: [C++][ORC] Teach CheckTimeZoneDatabaseAvailability how to inspect conda environments [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #41767:
URL: https://github.com/apache/arrow/pull/41767#discussion_r1609228646
##########
cpp/src/arrow/adapters/orc/adapter.cc:
##########
@@ -189,15 +189,21 @@ liborc::RowReaderOptions DefaultRowReaderOptions() {
#ifdef ARROW_ORC_NEED_TIME_ZONE_DATABASE_CHECK
// Proactively check timezone database availability for ORC versions older than 2.0.0
Status CheckTimeZoneDatabaseAvailability() {
- auto tz_dir = std::getenv("TZDIR");
- bool is_tzdb_avaiable = tz_dir != nullptr
- ? std::filesystem::exists(tz_dir)
- : std::filesystem::exists("/usr/share/zoneinfo");
- if (!is_tzdb_avaiable) {
+ // orc >=2.0.1 will look for tzdb in $CONDA_PREFIX/share/zoneinfo,
+ // which is provided by the package `tzdata` (if installed)
+ auto conda_prefix = std::getenv("CONDA_PREFIX");
+ auto tz_dir_raw = std::getenv("TZDIR");
+ std::string tz_dir = conda_prefix != nullptr
+ ? std::string(conda_prefix) + "/share/zoneinfo"
+ : std::string(tz_dir_raw != nullptr ? tz_dir_raw : "");
+ bool is_tzdb_available = (!tz_dir.empty())
+ ? std::filesystem::exists(tz_dir)
+ : std::filesystem::exists("/usr/share/zoneinfo");
+ if (!is_tzdb_available) {
return Status::Invalid(
"IANA time zone database is unavailable but required by ORC."
" Please install it to /usr/share/zoneinfo or set TZDIR env to the installed"
- " directory");
+ " directory. If you are using conda, simply install the package `tzdata`.");
Review Comment:
@kou WDYT?
--
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-41755: [C++][ORC] Teach CheckTimeZoneDatabaseAvailability how to inspect conda environments [arrow]
Posted by "h-vetinari (via GitHub)" <gi...@apache.org>.
h-vetinari commented on code in PR #41767:
URL: https://github.com/apache/arrow/pull/41767#discussion_r1615551852
##########
cpp/src/arrow/adapters/orc/adapter.cc:
##########
@@ -189,15 +189,21 @@ liborc::RowReaderOptions DefaultRowReaderOptions() {
#ifdef ARROW_ORC_NEED_TIME_ZONE_DATABASE_CHECK
// Proactively check timezone database availability for ORC versions older than 2.0.0
Status CheckTimeZoneDatabaseAvailability() {
- auto tz_dir = std::getenv("TZDIR");
- bool is_tzdb_avaiable = tz_dir != nullptr
- ? std::filesystem::exists(tz_dir)
- : std::filesystem::exists("/usr/share/zoneinfo");
- if (!is_tzdb_avaiable) {
+ // orc >=2.0.1 will look for tzdb in $CONDA_PREFIX/share/zoneinfo,
+ // which is provided by the package `tzdata` (if installed)
+ auto conda_prefix = std::getenv("CONDA_PREFIX");
+ auto tz_dir_raw = std::getenv("TZDIR");
+ std::string tz_dir = conda_prefix != nullptr
+ ? std::string(conda_prefix) + "/share/zoneinfo"
+ : std::string(tz_dir_raw != nullptr ? tz_dir_raw : "");
+ bool is_tzdb_available = (!tz_dir.empty())
+ ? std::filesystem::exists(tz_dir)
+ : std::filesystem::exists("/usr/share/zoneinfo");
+ if (!is_tzdb_available) {
return Status::Invalid(
"IANA time zone database is unavailable but required by ORC."
" Please install it to /usr/share/zoneinfo or set TZDIR env to the installed"
- " directory");
+ " directory. If you are using conda, simply install the package `tzdata`.");
Review Comment:
> Could you try this?
Confirmed in https://github.com/conda-forge/arrow-cpp-feedstock/pull/1432 that this works, thank you! Do you want to open a separate PR, or should I change this PR to your one-line suggestion?
--
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] teach CheckTimeZoneDatabaseAvailability how to inspect conda environments [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41767:
URL: https://github.com/apache/arrow/pull/41767#issuecomment-2123481782
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). 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](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) 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](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - Contributing Overview](https://arrow.apache.org/docs/developers/overview.html)
--
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-41755: [C++][ORC] Teach CheckTimeZoneDatabaseAvailability how to inspect conda environments [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #41767:
URL: https://github.com/apache/arrow/pull/41767#discussion_r1609271042
##########
cpp/src/arrow/adapters/orc/adapter.cc:
##########
@@ -189,15 +189,21 @@ liborc::RowReaderOptions DefaultRowReaderOptions() {
#ifdef ARROW_ORC_NEED_TIME_ZONE_DATABASE_CHECK
// Proactively check timezone database availability for ORC versions older than 2.0.0
Status CheckTimeZoneDatabaseAvailability() {
- auto tz_dir = std::getenv("TZDIR");
- bool is_tzdb_avaiable = tz_dir != nullptr
- ? std::filesystem::exists(tz_dir)
- : std::filesystem::exists("/usr/share/zoneinfo");
- if (!is_tzdb_avaiable) {
+ // orc >=2.0.1 will look for tzdb in $CONDA_PREFIX/share/zoneinfo,
+ // which is provided by the package `tzdata` (if installed)
+ auto conda_prefix = std::getenv("CONDA_PREFIX");
+ auto tz_dir_raw = std::getenv("TZDIR");
+ std::string tz_dir = conda_prefix != nullptr
+ ? std::string(conda_prefix) + "/share/zoneinfo"
+ : std::string(tz_dir_raw != nullptr ? tz_dir_raw : "");
+ bool is_tzdb_available = (!tz_dir.empty())
+ ? std::filesystem::exists(tz_dir)
+ : std::filesystem::exists("/usr/share/zoneinfo");
+ if (!is_tzdb_available) {
return Status::Invalid(
"IANA time zone database is unavailable but required by ORC."
" Please install it to /usr/share/zoneinfo or set TZDIR env to the installed"
- " directory");
+ " directory. If you are using conda, simply install the package `tzdata`.");
Review Comment:
Either is OK.
I don't know why this is happen with conda. Can we add `tzdata` to dependencies of `arrow-cpp` or `orc` conda packages?
--
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-41755: [C++][ORC] Teach CheckTimeZoneDatabaseAvailability how to inspect conda environments [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #41767:
URL: https://github.com/apache/arrow/pull/41767#discussion_r1609271042
##########
cpp/src/arrow/adapters/orc/adapter.cc:
##########
@@ -189,15 +189,21 @@ liborc::RowReaderOptions DefaultRowReaderOptions() {
#ifdef ARROW_ORC_NEED_TIME_ZONE_DATABASE_CHECK
// Proactively check timezone database availability for ORC versions older than 2.0.0
Status CheckTimeZoneDatabaseAvailability() {
- auto tz_dir = std::getenv("TZDIR");
- bool is_tzdb_avaiable = tz_dir != nullptr
- ? std::filesystem::exists(tz_dir)
- : std::filesystem::exists("/usr/share/zoneinfo");
- if (!is_tzdb_avaiable) {
+ // orc >=2.0.1 will look for tzdb in $CONDA_PREFIX/share/zoneinfo,
+ // which is provided by the package `tzdata` (if installed)
+ auto conda_prefix = std::getenv("CONDA_PREFIX");
+ auto tz_dir_raw = std::getenv("TZDIR");
+ std::string tz_dir = conda_prefix != nullptr
+ ? std::string(conda_prefix) + "/share/zoneinfo"
+ : std::string(tz_dir_raw != nullptr ? tz_dir_raw : "");
+ bool is_tzdb_available = (!tz_dir.empty())
+ ? std::filesystem::exists(tz_dir)
+ : std::filesystem::exists("/usr/share/zoneinfo");
+ if (!is_tzdb_available) {
return Status::Invalid(
"IANA time zone database is unavailable but required by ORC."
" Please install it to /usr/share/zoneinfo or set TZDIR env to the installed"
- " directory");
+ " directory. If you are using conda, simply install the package `tzdata`.");
Review Comment:
Both are OK.
I don't know why this is happen with conda. Can we add `tzdata` to dependencies of `arrow-cpp` or `orc` conda packages?
--
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-41755: [C++][ORC] Teach CheckTimeZoneDatabaseAvailability how to inspect conda environments [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #41767:
URL: https://github.com/apache/arrow/pull/41767#discussion_r1615318742
##########
cpp/src/arrow/adapters/orc/adapter.cc:
##########
@@ -189,15 +189,21 @@ liborc::RowReaderOptions DefaultRowReaderOptions() {
#ifdef ARROW_ORC_NEED_TIME_ZONE_DATABASE_CHECK
// Proactively check timezone database availability for ORC versions older than 2.0.0
Status CheckTimeZoneDatabaseAvailability() {
- auto tz_dir = std::getenv("TZDIR");
- bool is_tzdb_avaiable = tz_dir != nullptr
- ? std::filesystem::exists(tz_dir)
- : std::filesystem::exists("/usr/share/zoneinfo");
- if (!is_tzdb_avaiable) {
+ // orc >=2.0.1 will look for tzdb in $CONDA_PREFIX/share/zoneinfo,
+ // which is provided by the package `tzdata` (if installed)
+ auto conda_prefix = std::getenv("CONDA_PREFIX");
+ auto tz_dir_raw = std::getenv("TZDIR");
+ std::string tz_dir = conda_prefix != nullptr
+ ? std::string(conda_prefix) + "/share/zoneinfo"
+ : std::string(tz_dir_raw != nullptr ? tz_dir_raw : "");
+ bool is_tzdb_available = (!tz_dir.empty())
+ ? std::filesystem::exists(tz_dir)
+ : std::filesystem::exists("/usr/share/zoneinfo");
+ if (!is_tzdb_available) {
return Status::Invalid(
"IANA time zone database is unavailable but required by ORC."
" Please install it to /usr/share/zoneinfo or set TZDIR env to the installed"
- " directory");
+ " directory. If you are using conda, simply install the package `tzdata`.");
Review Comment:
> Indeed something might be going wrong there
Could you try this?
```diff
diff --git a/cpp/cmake_modules/FindorcAlt.cmake b/cpp/cmake_modules/FindorcAlt.cmake
index 289416678a..ce8cd11b4c 100644
--- a/cpp/cmake_modules/FindorcAlt.cmake
+++ b/cpp/cmake_modules/FindorcAlt.cmake
@@ -71,4 +71,5 @@ if(orcAlt_FOUND)
PROPERTIES IMPORTED_LOCATION "${ORC_STATIC_LIB}"
INTERFACE_INCLUDE_DIRECTORIES "${ORC_INCLUDE_DIR}")
endif()
+ set(orcAlt_VERSION ${ORC_VERSION})
endif()
```
> We can add tzdata as a dependency on `orc`, but I want to make this unconditional. It's better to always rely on conda-forge's tzdata, rather than potentially out-of-date stuff in `/usr/share/zoneinfo` (aside from not introducing unnecessary divergence between platforms).
It makes sense.
> We either need to ensure that `ARROW_ORC_NEED_TIME_ZONE_DATABASE_CHECK` correctly gets set to false on orc >=2.0.1, or do something like this PR. I think this PR is better
We don't want to maintain this code as much as possible ORC 2.0.0 or later have the same check. So we want to choose the former.
--
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-41755: [C++][ORC] ensure orc version gets set correctly [arrow]
Posted by "h-vetinari (via GitHub)" <gi...@apache.org>.
h-vetinari commented on code in PR #41767:
URL: https://github.com/apache/arrow/pull/41767#discussion_r1615620860
##########
cpp/src/arrow/adapters/orc/adapter.cc:
##########
@@ -189,15 +189,21 @@ liborc::RowReaderOptions DefaultRowReaderOptions() {
#ifdef ARROW_ORC_NEED_TIME_ZONE_DATABASE_CHECK
// Proactively check timezone database availability for ORC versions older than 2.0.0
Status CheckTimeZoneDatabaseAvailability() {
- auto tz_dir = std::getenv("TZDIR");
- bool is_tzdb_avaiable = tz_dir != nullptr
- ? std::filesystem::exists(tz_dir)
- : std::filesystem::exists("/usr/share/zoneinfo");
- if (!is_tzdb_avaiable) {
+ // orc >=2.0.1 will look for tzdb in $CONDA_PREFIX/share/zoneinfo,
+ // which is provided by the package `tzdata` (if installed)
+ auto conda_prefix = std::getenv("CONDA_PREFIX");
+ auto tz_dir_raw = std::getenv("TZDIR");
+ std::string tz_dir = conda_prefix != nullptr
+ ? std::string(conda_prefix) + "/share/zoneinfo"
+ : std::string(tz_dir_raw != nullptr ? tz_dir_raw : "");
+ bool is_tzdb_available = (!tz_dir.empty())
+ ? std::filesystem::exists(tz_dir)
+ : std::filesystem::exists("/usr/share/zoneinfo");
+ if (!is_tzdb_available) {
return Status::Invalid(
"IANA time zone database is unavailable but required by ORC."
" Please install it to /usr/share/zoneinfo or set TZDIR env to the installed"
- " directory");
+ " directory. If you are using conda, simply install the package `tzdata`.");
Review Comment:
Done
--
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-41755: [C++][ORC] Teach CheckTimeZoneDatabaseAvailability how to inspect conda environments [arrow]
Posted by "h-vetinari (via GitHub)" <gi...@apache.org>.
h-vetinari commented on code in PR #41767:
URL: https://github.com/apache/arrow/pull/41767#discussion_r1614977296
##########
cpp/src/arrow/adapters/orc/adapter.cc:
##########
@@ -189,15 +189,21 @@ liborc::RowReaderOptions DefaultRowReaderOptions() {
#ifdef ARROW_ORC_NEED_TIME_ZONE_DATABASE_CHECK
// Proactively check timezone database availability for ORC versions older than 2.0.0
Status CheckTimeZoneDatabaseAvailability() {
- auto tz_dir = std::getenv("TZDIR");
- bool is_tzdb_avaiable = tz_dir != nullptr
- ? std::filesystem::exists(tz_dir)
- : std::filesystem::exists("/usr/share/zoneinfo");
- if (!is_tzdb_avaiable) {
+ // orc >=2.0.1 will look for tzdb in $CONDA_PREFIX/share/zoneinfo,
+ // which is provided by the package `tzdata` (if installed)
+ auto conda_prefix = std::getenv("CONDA_PREFIX");
+ auto tz_dir_raw = std::getenv("TZDIR");
+ std::string tz_dir = conda_prefix != nullptr
+ ? std::string(conda_prefix) + "/share/zoneinfo"
+ : std::string(tz_dir_raw != nullptr ? tz_dir_raw : "");
+ bool is_tzdb_available = (!tz_dir.empty())
+ ? std::filesystem::exists(tz_dir)
+ : std::filesystem::exists("/usr/share/zoneinfo");
+ if (!is_tzdb_available) {
return Status::Invalid(
"IANA time zone database is unavailable but required by ORC."
" Please install it to /usr/share/zoneinfo or set TZDIR env to the installed"
- " directory");
+ " directory. If you are using conda, simply install the package `tzdata`.");
Review Comment:
> ORC 2.0.1 can find tzdb without setting `TZDIR` with conda (`CONDA_PREFIX`), right?
Yes. If the environment variable `CONDA_PREFIX` is set, the code will prefer looking in `$CONDA_PREFIX/share/zoneinfo`, which is what's shipped through `tzdata`.
> `libarrow` conda package uses ORC 2.0.1, right?
As of https://github.com/conda-forge/arrow-cpp-feedstock/pull/1421 (and the various backports), yes.
> So this message will not be shown for conda users. If this message is shown with ORC 2.0.1, ORC version detection may be failed.
Indeed something might be going wrong there, because https://github.com/conda-forge/pyarrow-feedstock/pull/122 failed before I backported this PR (in https://github.com/conda-forge/arrow-cpp-feedstock/pull/1424), despite building against orc 2.0.1 already
> Can we add `tzdata` to dependencies of `orc` only on Windows? If we can do it, it will solve this problem.
We can add tzdata as a dependency on `orc`, but I want to make this unconditional. It's better to always rely on conda-forge's tzdata, rather than potentially out-of-date stuff in `/usr/share/zoneinfo` (aside from not introducing unnecessary divergence between platforms).
In any case, the presence of `tzdata` itself is not enough. We either need to ensure that `ARROW_ORC_NEED_TIME_ZONE_DATABASE_CHECK` correctly gets set to false on orc >=2.0.1, or do something like this PR. I think this PR is better, because you cannot know at compile-time if the package will end up in a conda-environment, and IMO this check should match what `orc` does, i.e. check `CONDA_PREFIX`.
--
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-41755: [C++][ORC] Ensure setting detected ORC version [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #41767:
URL: https://github.com/apache/arrow/pull/41767
--
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-41755: [C++][ORC] Teach CheckTimeZoneDatabaseAvailability how to inspect conda environments [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #41767:
URL: https://github.com/apache/arrow/pull/41767#discussion_r1614918488
##########
cpp/src/arrow/adapters/orc/adapter.cc:
##########
@@ -189,15 +189,21 @@ liborc::RowReaderOptions DefaultRowReaderOptions() {
#ifdef ARROW_ORC_NEED_TIME_ZONE_DATABASE_CHECK
// Proactively check timezone database availability for ORC versions older than 2.0.0
Status CheckTimeZoneDatabaseAvailability() {
- auto tz_dir = std::getenv("TZDIR");
- bool is_tzdb_avaiable = tz_dir != nullptr
- ? std::filesystem::exists(tz_dir)
- : std::filesystem::exists("/usr/share/zoneinfo");
- if (!is_tzdb_avaiable) {
+ // orc >=2.0.1 will look for tzdb in $CONDA_PREFIX/share/zoneinfo,
+ // which is provided by the package `tzdata` (if installed)
+ auto conda_prefix = std::getenv("CONDA_PREFIX");
+ auto tz_dir_raw = std::getenv("TZDIR");
+ std::string tz_dir = conda_prefix != nullptr
+ ? std::string(conda_prefix) + "/share/zoneinfo"
+ : std::string(tz_dir_raw != nullptr ? tz_dir_raw : "");
+ bool is_tzdb_available = (!tz_dir.empty())
+ ? std::filesystem::exists(tz_dir)
+ : std::filesystem::exists("/usr/share/zoneinfo");
+ if (!is_tzdb_available) {
return Status::Invalid(
"IANA time zone database is unavailable but required by ORC."
" Please install it to /usr/share/zoneinfo or set TZDIR env to the installed"
- " directory");
+ " directory. If you are using conda, simply install the package `tzdata`.");
Review Comment:
Thanks for explaining more.
ORC 2.0.1 can find tzdb without setting `TZDIR` with conda (`CONDA_PREFIX`), right?
`libarrow` conda package uses ORC 2.0.1, right?
This code isn't used with ORC 2.0.0 or later: https://github.com/apache/arrow/blob/1c9e393b73195840960dfb9eca8c0dc390be751a/cpp/src/arrow/CMakeLists.txt#L911-L913
So this message will not be shown for conda users. If this message is shown with ORC 2.0.1, ORC version detection may be failed.
--
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-41755: [C++][ORC] Teach CheckTimeZoneDatabaseAvailability how to inspect conda environments [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #41767:
URL: https://github.com/apache/arrow/pull/41767#discussion_r1614918854
##########
cpp/src/arrow/adapters/orc/adapter.cc:
##########
@@ -189,15 +189,21 @@ liborc::RowReaderOptions DefaultRowReaderOptions() {
#ifdef ARROW_ORC_NEED_TIME_ZONE_DATABASE_CHECK
// Proactively check timezone database availability for ORC versions older than 2.0.0
Status CheckTimeZoneDatabaseAvailability() {
- auto tz_dir = std::getenv("TZDIR");
- bool is_tzdb_avaiable = tz_dir != nullptr
- ? std::filesystem::exists(tz_dir)
- : std::filesystem::exists("/usr/share/zoneinfo");
- if (!is_tzdb_avaiable) {
+ // orc >=2.0.1 will look for tzdb in $CONDA_PREFIX/share/zoneinfo,
+ // which is provided by the package `tzdata` (if installed)
+ auto conda_prefix = std::getenv("CONDA_PREFIX");
+ auto tz_dir_raw = std::getenv("TZDIR");
+ std::string tz_dir = conda_prefix != nullptr
+ ? std::string(conda_prefix) + "/share/zoneinfo"
+ : std::string(tz_dir_raw != nullptr ? tz_dir_raw : "");
+ bool is_tzdb_available = (!tz_dir.empty())
+ ? std::filesystem::exists(tz_dir)
+ : std::filesystem::exists("/usr/share/zoneinfo");
+ if (!is_tzdb_available) {
return Status::Invalid(
"IANA time zone database is unavailable but required by ORC."
" Please install it to /usr/share/zoneinfo or set TZDIR env to the installed"
- " directory");
+ " directory. If you are using conda, simply install the package `tzdata`.");
Review Comment:
> > Can we add `tzdata` to dependencies of `arrow-cpp` or `orc` conda packages?
>
> I also don't mind moving this to orc itself. Currently it's just used for pyarrow, because enabling the pyarrow-orc bindings exercised these code paths (which is what lead to #36026).
Can we add `tzdata` to dependencies of `orc` only on Windows? If we can do it, it will solve this problem.
--
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-41755: [C++][ORC] Teach CheckTimeZoneDatabaseAvailability how to inspect conda environments [arrow]
Posted by "h-vetinari (via GitHub)" <gi...@apache.org>.
h-vetinari commented on code in PR #41767:
URL: https://github.com/apache/arrow/pull/41767#discussion_r1609291378
##########
cpp/src/arrow/adapters/orc/adapter.cc:
##########
@@ -189,15 +189,21 @@ liborc::RowReaderOptions DefaultRowReaderOptions() {
#ifdef ARROW_ORC_NEED_TIME_ZONE_DATABASE_CHECK
// Proactively check timezone database availability for ORC versions older than 2.0.0
Status CheckTimeZoneDatabaseAvailability() {
- auto tz_dir = std::getenv("TZDIR");
- bool is_tzdb_avaiable = tz_dir != nullptr
- ? std::filesystem::exists(tz_dir)
- : std::filesystem::exists("/usr/share/zoneinfo");
- if (!is_tzdb_avaiable) {
+ // orc >=2.0.1 will look for tzdb in $CONDA_PREFIX/share/zoneinfo,
+ // which is provided by the package `tzdata` (if installed)
+ auto conda_prefix = std::getenv("CONDA_PREFIX");
+ auto tz_dir_raw = std::getenv("TZDIR");
+ std::string tz_dir = conda_prefix != nullptr
+ ? std::string(conda_prefix) + "/share/zoneinfo"
+ : std::string(tz_dir_raw != nullptr ? tz_dir_raw : "");
+ bool is_tzdb_available = (!tz_dir.empty())
+ ? std::filesystem::exists(tz_dir)
+ : std::filesystem::exists("/usr/share/zoneinfo");
+ if (!is_tzdb_available) {
return Status::Invalid(
"IANA time zone database is unavailable but required by ORC."
" Please install it to /usr/share/zoneinfo or set TZDIR env to the installed"
- " directory");
+ " directory. If you are using conda, simply install the package `tzdata`.");
Review Comment:
> Can we add `tzdata` to dependencies of `arrow-cpp` or `orc` conda packages?
I also don't mind moving this to orc itself. Currently it's just used for pyarrow, because enabling the pyarrow-orc bindings exercised these code paths (which is what lead to #36026).
--
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-41755: [C++][ORC] Teach CheckTimeZoneDatabaseAvailability how to inspect conda environments [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #41767:
URL: https://github.com/apache/arrow/pull/41767#discussion_r1615580897
##########
cpp/src/arrow/adapters/orc/adapter.cc:
##########
@@ -189,15 +189,21 @@ liborc::RowReaderOptions DefaultRowReaderOptions() {
#ifdef ARROW_ORC_NEED_TIME_ZONE_DATABASE_CHECK
// Proactively check timezone database availability for ORC versions older than 2.0.0
Status CheckTimeZoneDatabaseAvailability() {
- auto tz_dir = std::getenv("TZDIR");
- bool is_tzdb_avaiable = tz_dir != nullptr
- ? std::filesystem::exists(tz_dir)
- : std::filesystem::exists("/usr/share/zoneinfo");
- if (!is_tzdb_avaiable) {
+ // orc >=2.0.1 will look for tzdb in $CONDA_PREFIX/share/zoneinfo,
+ // which is provided by the package `tzdata` (if installed)
+ auto conda_prefix = std::getenv("CONDA_PREFIX");
+ auto tz_dir_raw = std::getenv("TZDIR");
+ std::string tz_dir = conda_prefix != nullptr
+ ? std::string(conda_prefix) + "/share/zoneinfo"
+ : std::string(tz_dir_raw != nullptr ? tz_dir_raw : "");
+ bool is_tzdb_available = (!tz_dir.empty())
+ ? std::filesystem::exists(tz_dir)
+ : std::filesystem::exists("/usr/share/zoneinfo");
+ if (!is_tzdb_available) {
return Status::Invalid(
"IANA time zone database is unavailable but required by ORC."
" Please install it to /usr/share/zoneinfo or set TZDIR env to the installed"
- " directory");
+ " directory. If you are using conda, simply install the package `tzdata`.");
Review Comment:
> should I change this PR to your one-line suggestion?
Could you do 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-41755: [C++][ORC] Ensure setting detected ORC version [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #41767:
URL: https://github.com/apache/arrow/pull/41767#issuecomment-2145636882
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit ff9921ffa89585be69ae85674bb365d03cb22ba4.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/25745129654) has more details. It also includes information about 114 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