You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@orc.apache.org by "wgtmac (via GitHub)" <gi...@apache.org> on 2023/08/16 05:33:28 UTC
[GitHub] [orc] wgtmac opened a new pull request, #1587: ORC-1481: [C++] Better error message when TZDB is unavailable
wgtmac opened a new pull request, #1587:
URL: https://github.com/apache/orc/pull/1587
### What changes were proposed in this pull request?
Check the availability of TZDB and throw helpful error message before it crashes.
### Why are the changes needed?
When local IANA TZDB is unavailable or TZDIR env is not properly set, getTimezoneByName() simply fails without much helpful guidance on what to do.
### How was this patch tested?
Added a test case TestTimezone.testMissingTZDB to make sure error message is as expected.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] coderex2522 commented on a diff in pull request #1587: ORC-1481: [C++] Better error message when TZDB is unavailable
Posted by "coderex2522 (via GitHub)" <gi...@apache.org>.
coderex2522 commented on code in PR #1587:
URL: https://github.com/apache/orc/pull/1587#discussion_r1300848515
##########
c++/src/Timezone.cc:
##########
@@ -675,6 +675,13 @@ namespace orc {
if (itr != timezoneCache.end()) {
return *(itr->second).get();
}
+ if (!std::filesystem::exists(std::filesystem::path(filename))) {
Review Comment:
+1, LGTM! @wgtmac However, the logic of determining whether a file exists can be integrated into the readLocalFile function by throwing a FileNotFound exception. This exception can be caught in the getTimezoneByFilename function and further thrown as a Timezone exception.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] mystic-lama commented on a diff in pull request #1587: ORC-1481: [C++] Better error message when TZDB is unavailable
Posted by "mystic-lama (via GitHub)" <gi...@apache.org>.
mystic-lama commented on code in PR #1587:
URL: https://github.com/apache/orc/pull/1587#discussion_r1295983141
##########
c++/src/Timezone.cc:
##########
@@ -675,6 +675,13 @@ namespace orc {
if (itr != timezoneCache.end()) {
return *(itr->second).get();
}
+ if (!std::filesystem::exists(std::filesystem::path(filename))) {
+ std::stringstream ss;
+ ss << "Time zone file " << filename << " does not exist."
+ << " Please install IANA time zone database and set TZDIR env properly"
Review Comment:
Just a suggestion: Can rephrase as
"Please install IANA time zone database and set TZDIR env property to point to /usr/share/zoneinfo" or something like that
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] coderex2522 commented on a diff in pull request #1587: ORC-1481: [C++] Better error message when TZDB is unavailable
Posted by "coderex2522 (via GitHub)" <gi...@apache.org>.
coderex2522 commented on code in PR #1587:
URL: https://github.com/apache/orc/pull/1587#discussion_r1300848515
##########
c++/src/Timezone.cc:
##########
@@ -675,6 +675,13 @@ namespace orc {
if (itr != timezoneCache.end()) {
return *(itr->second).get();
}
+ if (!std::filesystem::exists(std::filesystem::path(filename))) {
Review Comment:
+1, LGTM! @wgtmac However, the logic of determining whether a file exists can be integrated into the readLocalFile function by throwing a file not found exception. This exception can be caught in the getTimezoneByFilename function and further thrown as a Timezone 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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a diff in pull request #1587: ORC-1481: [C++] Better error message when TZDB is unavailable
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #1587:
URL: https://github.com/apache/orc/pull/1587#discussion_r1297877017
##########
c++/src/Timezone.cc:
##########
@@ -675,6 +675,13 @@ namespace orc {
if (itr != timezoneCache.end()) {
return *(itr->second).get();
}
+ if (!std::filesystem::exists(std::filesystem::path(filename))) {
+ std::stringstream ss;
+ ss << "Time zone file " << filename << " does not exist."
+ << " Please install IANA time zone database and set TZDIR env"
+ << " if it is not installed at /usr/share/zoneinfo";
Review Comment:
Thank you for investigating and making a PR, @wgtmac .
Just a question: The original issue report (#1577) also mentioned Windows, what happens in Windows? Is this an actionable message in Windows?
> Specifically on Windows, the DEFAULT_TZDIR location is an invalid path,
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on a diff in pull request #1587: ORC-1481: [C++] Better error message when TZDB is unavailable
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1587:
URL: https://github.com/apache/orc/pull/1587#discussion_r1300870195
##########
c++/src/Timezone.cc:
##########
@@ -675,6 +675,13 @@ namespace orc {
if (itr != timezoneCache.end()) {
return *(itr->second).get();
}
+ if (!std::filesystem::exists(std::filesystem::path(filename))) {
Review Comment:
Yes, I just want to check proactively before reading it so the error message is clear. It may fail reading due to other reasons.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ORC-1481: [C++] Better error message when TZDB is unavailable [orc]
Posted by "scw (via GitHub)" <gi...@apache.org>.
scw commented on PR #1587:
URL: https://github.com/apache/orc/pull/1587#issuecomment-1804879932
> Sorry for writing in a closed thread but I think the question fits here. I downloaded the database from https://data.iana.org/time-zones/releases/tzdata2023c.tar.gz unpacked it, set the env variable TZDIR to the unpacked directory and still can't get ORC to work as it looks for a file called "UTC" but files in the database are mostly continent names like "africa", "asia", etc. What am I missing?
You still need to build the package, while that has data is is still a source distribution. For example, see how conda-forge does it here: https://github.com/conda-forge/tzdata-feedstock/blob/main/recipe/build.sh
Once you've built it, you should see the expected files that can be used for `TZDIR`
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on a diff in pull request #1587: ORC-1481: [C++] Better error message when TZDB is unavailable
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1587:
URL: https://github.com/apache/orc/pull/1587#discussion_r1296024801
##########
c++/src/Timezone.cc:
##########
@@ -675,6 +675,13 @@ namespace orc {
if (itr != timezoneCache.end()) {
return *(itr->second).get();
}
+ if (!std::filesystem::exists(std::filesystem::path(filename))) {
+ std::stringstream ss;
+ ss << "Time zone file " << filename << " does not exist."
+ << " Please install IANA time zone database and set TZDIR env properly"
Review Comment:
What about this: `Please install IANA time zone database and set TZDIR env if it is not installed at /usr/share/zoneinfo`
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on a diff in pull request #1587: ORC-1481: [C++] Better error message when TZDB is unavailable
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1587:
URL: https://github.com/apache/orc/pull/1587#discussion_r1298288586
##########
c++/src/Timezone.cc:
##########
@@ -675,6 +675,13 @@ namespace orc {
if (itr != timezoneCache.end()) {
return *(itr->second).get();
}
+ if (!std::filesystem::exists(std::filesystem::path(filename))) {
+ std::stringstream ss;
+ ss << "Time zone file " << filename << " does not exist."
+ << " Please install IANA time zone database and set TZDIR env"
+ << " if it is not installed at /usr/share/zoneinfo";
Review Comment:
The default location `/usr/share/zoneinfo` does not apply to Windows. I think the error message works for Windows, too.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a diff in pull request #1587: ORC-1481: [C++] Better error message when TZDB is unavailable
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #1587:
URL: https://github.com/apache/orc/pull/1587#discussion_r1298940081
##########
c++/src/Timezone.cc:
##########
@@ -675,6 +675,13 @@ namespace orc {
if (itr != timezoneCache.end()) {
return *(itr->second).get();
}
+ if (!std::filesystem::exists(std::filesystem::path(filename))) {
+ std::stringstream ss;
+ ss << "Time zone file " << filename << " does not exist."
+ << " Please install IANA time zone database and set TZDIR env"
+ << " if it is not installed at /usr/share/zoneinfo";
Review Comment:
Yes, it's not applied to `Windows OS` because the path style is invalid.
This PR might lead a user to try to check the location of invalid path information by mapping from `/usr/share/zoneinfo` to `C:\\usr\share\zoneinfo`. Although the user ends up to conclude that the condition statement (`if it is not installed at /usr/share/zoneinfo`) is `false`, this message might confuse him by suggesting him to install it into `C:\\usr\share\zoneinfo`.
When the error message happens, I'm wondering if we don't need the message (`if it is not installed at /usr/share/zoneinfo`) at all.
The first two lines look sufficient to me. WDYT?
```c++
ss << "Time zone file " << filename << " does not exist."
<< " Please install IANA time zone database and set TZDIR env.";
```
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on pull request #1587: ORC-1481: [C++] Better error message when TZDB is unavailable
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1587:
URL: https://github.com/apache/orc/pull/1587#issuecomment-1689663835
I've merged it. Thanks @mystic-lama @dongjoon-hyun @coderex2522!
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] mystic-lama commented on a diff in pull request #1587: ORC-1481: [C++] Better error message when TZDB is unavailable
Posted by "mystic-lama (via GitHub)" <gi...@apache.org>.
mystic-lama commented on code in PR #1587:
URL: https://github.com/apache/orc/pull/1587#discussion_r1296105988
##########
c++/src/Timezone.cc:
##########
@@ -675,6 +675,13 @@ namespace orc {
if (itr != timezoneCache.end()) {
return *(itr->second).get();
}
+ if (!std::filesystem::exists(std::filesystem::path(filename))) {
+ std::stringstream ss;
+ ss << "Time zone file " << filename << " does not exist."
+ << " Please install IANA time zone database and set TZDIR env properly"
Review Comment:
minor nit:
... set TZDIR env to point to correct location, if not installed....
I am just wearing a user's hat and trying to see if the message helps me resolve things. Please note that English is not my 1st language
@wgtmac Thanks :)
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a diff in pull request #1587: ORC-1481: [C++] Better error message when TZDB is unavailable
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #1587:
URL: https://github.com/apache/orc/pull/1587#discussion_r1297877017
##########
c++/src/Timezone.cc:
##########
@@ -675,6 +675,13 @@ namespace orc {
if (itr != timezoneCache.end()) {
return *(itr->second).get();
}
+ if (!std::filesystem::exists(std::filesystem::path(filename))) {
+ std::stringstream ss;
+ ss << "Time zone file " << filename << " does not exist."
+ << " Please install IANA time zone database and set TZDIR env"
+ << " if it is not installed at /usr/share/zoneinfo";
Review Comment:
Thank you for investigating and making a PR, @wgtmac .
Just a question: The original issue report (#1577) also mentioned Windows, what happens in Windows?
> Specifically on Windows, the DEFAULT_TZDIR location is an invalid path,
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on a diff in pull request #1587: ORC-1481: [C++] Better error message when TZDB is unavailable
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1587:
URL: https://github.com/apache/orc/pull/1587#discussion_r1300868189
##########
c++/src/Timezone.cc:
##########
@@ -675,6 +675,13 @@ namespace orc {
if (itr != timezoneCache.end()) {
return *(itr->second).get();
}
+ if (!std::filesystem::exists(std::filesystem::path(filename))) {
+ std::stringstream ss;
+ ss << "Time zone file " << filename << " does not exist."
+ << " Please install IANA time zone database and set TZDIR env"
+ << " if it is not installed at /usr/share/zoneinfo";
Review Comment:
I agree with you, let me change it.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac closed pull request #1587: ORC-1481: [C++] Better error message when TZDB is unavailable
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac closed pull request #1587: ORC-1481: [C++] Better error message when TZDB is unavailable
URL: https://github.com/apache/orc/pull/1587
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a diff in pull request #1587: ORC-1481: [C++] Better error message when TZDB is unavailable
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #1587:
URL: https://github.com/apache/orc/pull/1587#discussion_r1298940081
##########
c++/src/Timezone.cc:
##########
@@ -675,6 +675,13 @@ namespace orc {
if (itr != timezoneCache.end()) {
return *(itr->second).get();
}
+ if (!std::filesystem::exists(std::filesystem::path(filename))) {
+ std::stringstream ss;
+ ss << "Time zone file " << filename << " does not exist."
+ << " Please install IANA time zone database and set TZDIR env"
+ << " if it is not installed at /usr/share/zoneinfo";
Review Comment:
Yes, it's not applied to `Windows OS` because the path style is invalid.
This PR might lead a user to try to check the location of invalid path information by mapping from `/usr/share/zoneinfo` to `C:\\usr\share\zoneinfo`. Although the user ends up to conclude that the condition statement (`if it is not installed at /usr/share/zoneinfo`) is `false`, this message might confuse him by suggesting him to install it into `C:\\usr\share\zoneinfo`.
When the error message happens, I'm wondering if we don't need the message (`if it is not installed at /usr/share/zoneinfo`) at all.
The first two lines look sufficient to me.
```c++
ss << "Time zone file " << filename << " does not exist."
<< " Please install IANA time zone database and set TZDIR env.";
```
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #1587: ORC-1481: [C++] Better error message when TZDB is unavailable
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #1587:
URL: https://github.com/apache/orc/pull/1587#issuecomment-1690207770
Thank you so much, @wgtmac and all.
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ORC-1481: [C++] Better error message when TZDB is unavailable [orc]
Posted by "rafal-c (via GitHub)" <gi...@apache.org>.
rafal-c commented on PR #1587:
URL: https://github.com/apache/orc/pull/1587#issuecomment-1804054286
Sorry for writing in a closed thread but I think the question fits here. I downloaded the database from https://data.iana.org/time-zones/releases/tzdata2023c.tar.gz unpacked it, set the env variable TZDIR to the unpacked directory and still can't get ORC to work as it looks for a file called "UTC" but files in the database are mostly continent names like "africa", "asia", etc.
What am I missing?
--
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: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org