ORC-1481: [C++] Better error message when TZDB is unavailable #1587

wgtmac · 2023-08-16T05:33:27Z

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.

paliwalashish · 2023-08-16T14:13:22Z

c++/src/Timezone.cc

+    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"


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

What about this: Please install IANA time zone database and set TZDIR env if it is not installed at /usr/share/zoneinfo

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 :)

dongjoon-hyun · 2023-08-18T00:38:44Z

c++/src/Timezone.cc

+      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";


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,

The default location /usr/share/zoneinfo does not apply to Windows. I think the error message works for Windows, too.

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?

ss << "Time zone file " << filename << " does not exist." << " Please install IANA time zone database and set TZDIR env.";

I agree with you, let me change it.

dongjoon-hyun

+1, LGTM (except one comment, https://github.com/apache/orc/pull/1587/files#r1298940081).

coderex2522 · 2023-08-22T02:53:38Z

c++/src/Timezone.cc

    if (itr != timezoneCache.end()) {
      return *(itr->second).get();
    }
+    if (!std::filesystem::exists(std::filesystem::path(filename))) {


+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.

Yes, I just want to check proactively before reading it so the error message is clear. It may fail reading due to other reasons.

wgtmac · 2023-08-23T09:59:28Z

I've merged it. Thanks @mystic-lama @dongjoon-hyun @coderex2522!

dongjoon-hyun · 2023-08-23T15:48:39Z

Thank you so much, @wgtmac and all.

rafal-c · 2023-11-09T15:31:40Z

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?

scw · 2023-11-10T00:20:08Z

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

### 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. Closes apache#1587 from wgtmac/ORC-1481. Authored-by: Gang Wu <ustcwg@gmail.com> Signed-off-by: Gang Wu <ustcwg@gmail.com>

### What changes were proposed in this pull request? Find tzdb without having to set `TZDIR` when in a conda-environment (where `tzdata` [has](https://conda-metadata-app.streamlit.app/?q=conda-forge%2Fnoarch%2Ftzdata-2024a-h0c530f3_0.conda) a uniform location of `$CONDA_PREFIX/share/zoneinfo` across all platforms). ### Why are the changes needed? This is due to issues in arrow (see apache/arrow#36026) that cannot really be fixed there, as it assumes that orc >=2.0 knows how to find the tzdb. Having to set `TZDIR` in all user environments is an intrusive change that should be avoided, and since the cost here is checking a single environment variable, it's hopefully not too onerous for consideration. ### How was this patch tested? CI here ### Was this patch authored or co-authored using generative AI tooling? No CC wgtmac See also: #1587 Closes #1882 from h-vetinari/tzdb. Authored-by: H. Vetinari <h.vetinari@gmx.com> Signed-off-by: Gang Wu <ustcwg@gmail.com>

### What changes were proposed in this pull request? Find tzdb without having to set `TZDIR` when in a conda-environment (where `tzdata` [has](https://conda-metadata-app.streamlit.app/?q=conda-forge%2Fnoarch%2Ftzdata-2024a-h0c530f3_0.conda) a uniform location of `$CONDA_PREFIX/share/zoneinfo` across all platforms). ### Why are the changes needed? This is due to issues in arrow (see apache/arrow#36026) that cannot really be fixed there, as it assumes that orc >=2.0 knows how to find the tzdb. Having to set `TZDIR` in all user environments is an intrusive change that should be avoided, and since the cost here is checking a single environment variable, it's hopefully not too onerous for consideration. ### How was this patch tested? CI here ### Was this patch authored or co-authored using generative AI tooling? No CC wgtmac See also: #1587 Closes #1882 from h-vetinari/tzdb. Authored-by: H. Vetinari <h.vetinari@gmx.com> Signed-off-by: Gang Wu <ustcwg@gmail.com> (cherry picked from commit e89ca33) Signed-off-by: Gang Wu <ustcwg@gmail.com>

github-actions bot added the CPP label Aug 16, 2023

wgtmac mentioned this pull request Aug 16, 2023

Unset TZDIR will cause crash without /usr/share/zoneinfo #1577

Closed

ORC-1481: [C++] Better error message when TZDB is unavailable

9e626e1

wgtmac force-pushed the ORC-1481 branch from 18d8dd3 to 9e626e1 Compare August 16, 2023 07:45

paliwalashish reviewed Aug 16, 2023

View reviewed changes

Refine error message

fd35805

dongjoon-hyun reviewed Aug 18, 2023

View reviewed changes

dongjoon-hyun added this to the 2.0.0 milestone Aug 19, 2023

dongjoon-hyun approved these changes Aug 19, 2023

View reviewed changes

coderex2522 approved these changes Aug 22, 2023

View reviewed changes

rephrase message

0acb597

wgtmac closed this in 873d00c Aug 23, 2023

wgtmac deleted the ORC-1481 branch February 27, 2024 02:28

wgtmac mentioned this pull request Mar 19, 2024

[Python] BUG: Reading ORC segfaults on windows (if TZDIR isn't set) apache/arrow#36026

Closed

kou mentioned this pull request Mar 19, 2024

[Python] ORC Reader aborts when timezone file is missing apache/arrow#40633

Closed

wgtmac mentioned this pull request Mar 21, 2024

GH-36026: [C++][ORC] Catch all ORC exceptions to avoid crash apache/arrow#40697

Merged

h-vetinari mentioned this pull request Apr 10, 2024

ORC-1684: [C++] Find tzdb without TZDIR when in conda-environments #1882

Closed

dongjoon-hyun mentioned this pull request Oct 23, 2024

Example files are using legacy timezone names (US/Pacific) #2049

Closed

ORC-1481: [C++] Better error message when TZDB is unavailable #1587

ORC-1481: [C++] Better error message when TZDB is unavailable #1587

Uh oh!

Conversation

wgtmac commented Aug 16, 2023

What changes were proposed in this pull request?

Why are the changes needed?

How was this patch tested?

Uh oh!

paliwalashish Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

wgtmac Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

paliwalashish Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Aug 18, 2023 • 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!

wgtmac Aug 18, 2023

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Aug 19, 2023 • 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!

wgtmac Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

coderex2522 Aug 22, 2023 • 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!

wgtmac Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Aug 23, 2023

Uh oh!

dongjoon-hyun commented Aug 23, 2023

Uh oh!

rafal-c commented Nov 9, 2023

Uh oh!

scw commented Nov 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dongjoon-hyun Aug 18, 2023 •

edited

Loading

dongjoon-hyun Aug 19, 2023 •

edited

Loading

coderex2522 Aug 22, 2023 •

edited

Loading