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

[h-vetinari]

What changes were proposed in this pull request?

Find tzdb without having to set TZDIR when in a conda-environment (where tzdata has 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

[h-vetinari]

If desired, I can test this patch on conda-forge infrastructure (build a patched orc that's published in a separate channel, then run conda-forge/arrow-cpp-feedstock#1086 against that).

[wgtmac]

Thanks for the fix! Do you have a JIRA account? I can assign this to you.

[h-vetinari]

Thanks for the quick review!

Thanks for the fix! Do you have a JIRA account? I can assign this to you.

https://issues.apache.org/jira/secure/ViewProfile.jspa?name=h-vetinari

[h-vetinari]

Would it be possible/acceptable to create a symlink in CI? I believe we could write a test that exercises the branch for CONDA_PREFIX here similar to

orc/c++/test/TestTimezone.cc

Line 423 in b75dcaa

TEST(TestTimezone, testMissingTZDB) {

but it would need a working tzdb at the end of $CONDA_PREFIX/share/zoneinfo.

[h-vetinari]

Ah, just found

orc/cmake_modules/ThirdpartyToolchain.cmake

Line 340 in b75dcaa

set(TZDATA_DIR ${SOURCE_DIR}/share/zoneinfo)

So I guess it should be there?

[h-vetinari]

Interestingly, I cannot inspect any CI logs here. Unfolding any section just gives a red Error:.

Nevermind, it worked now after 5min...

[wgtmac]

Ah, just found

orc/cmake_modules/ThirdpartyToolchain.cmake

Line 340 in b75dcaa

set(TZDATA_DIR ${SOURCE_DIR}/share/zoneinfo)

So I guess it should be there?

That is only used in WIN32 to make test pass on Windows...

[h-vetinari]

That is only used in WIN32 to make test pass on Windows...

Well, then we could still test that the CONDA_PREFIX mechanics work on windows, no? See my last commit, which tries to do this.

[h-vetinari]

OK, so the test I added passed now:

[ RUN      ] TestTimezone.testTzdbFromCondaEnv
[       OK ] TestTimezone.testTzdbFromCondaEnv (0 ms)

but restoring TZDIR still fails:

 unknown file: error: C++ exception with description "Time zone file ������������������������������������������������������������������������������/UTC does not exist. 

Since this is copied from testMissingTZDB, I'm surprised the same error doesn't happen there, because that test is saving tzDirBackup in exactly the same way - i.e. just a pointer to an environment variable which then gets deleted. It seems to me that the restoration of that environment variable cannot succeed (without a deepcopy before deleting TZDIR), and so far that test may have only passed due to checking if (tzDirBackup != nullptr), but - potentially - never actually restored the environment variable...? 🤔

[h-vetinari]

Tests are passing on windows now! 🥳

[h-vetinari]

This is all green now and should be ready for closer review. :)

[h-vetinari]

I have a last question - I see that there's some sort of caching for the tzdb going on (or at least, there's a test for it):

orc/c++/test/TestTimezone.cc

Lines 346 to 347 in 695e0f3

	TEST(TestTimezone, testZoneCache) {
	const Timezone* la1 = &getTimezoneByName("America/Los_Angeles");

Can we delete that cache somehow to rule out that this is what's causing testTzdbFromCondaEnv to pass? Or is it enough to choose some timezones that haven't been queried yet to bypass the cache?

[wgtmac]

I have a last question - I see that there's some sort of caching for the tzdb going on (or at least, there's a test for it):

orc/c++/test/TestTimezone.cc

Lines 346 to 347 in 695e0f3

	TEST(TestTimezone, testZoneCache) {
	const Timezone* la1 = &getTimezoneByName("America/Los_Angeles");

Can we delete that cache somehow to rule out that this is what's causing testTzdbFromCondaEnv to pass?

We can either use a different timezone to avoid caching or add a function to invalidate the cache.

[h-vetinari]

Sorry for the force-pushes; I had to figure out the right dates/times (which is a bit harder because we apparently need to specify them in UTC rather than local time; in the latter it would always be around 2/3am).

[h-vetinari]

OK, I think I'm done now from my side. This tests all the things I could think of as relevant.

[wgtmac]

Thanks @h-vetinari for all your effort!

[h-vetinari]

Thanks a lot for the review and help here @wgtmac! 🙏

Would you consider including this in the 2.0.1 release? I'm probably going to backport this in conda-forge if not, but it would be nicer if it could be included in the release here directly; seeing that it's just about checking a single environment variable, I think the risk here is very low.

[wgtmac]

Yes, I have already ported it to branch-2.0 and will be released in 2.0.1.

[h-vetinari]

Amazing, thanks a lot! 🤩