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

[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! 🤩