Ignore time zones that are not recognizable by OS when building time zone database #10654

zhztheplayer · 2024-08-05T03:16:45Z

The patch fixes fatal error <zone id> not found in timezone database when at least one of the timezone IDs in file don't exist in OS's supported timezone list.

netlify · 2024-08-05T03:17:04Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`37201b7`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/66e22f24309cde0008a098f3

zhztheplayer · 2024-08-05T03:21:09Z

velox/type/tz/TimeZoneMap.cpp

+      } catch (std::runtime_error& err) {
+        // Timezone not found in OS, skip it.
+        LOG(WARNING) << "Timezone [" << entry.second << "] not found due to: '"
+                     << err.what() << "', ignoring it. ";
+        continue;
+      }


@pedroerp Would you think it's OK to modify tz.h / tz.cpp? Since perhaps we could add a new function, like bool has_zone(string tz_name) to avoid catching a runtime_error here.

We made some other changes to that library, so it should be ok. Just make sure you capture the changes in a .patch file

Also, what happens when API clients try to convert a timezone not found?

Also, what happens when API clients try to convert a timezone not found?

Do you mean when e.g., calling APIs in TimeZoneMap.h ?

In that case an Exception: VeloxUserError like the following one will be raised

unknown file: Failure C++ exception with description "Exception: VeloxUserError Error Source: USER Error Code: INVALID_ARGUMENT Reason: Unknown time zone: 'America/Non_Exist' Retriable: False Function: locateZone File: /opt/gluten/ep/build-velox/build/velox_ep/velox/type/tz/TimeZoneMap.cpp Line: 276

@zhztheplayer there are two different things in this API:

The pointer to date::time_zone*, which does the actual time zone conversion.

The TimeZone object, which carries the date::time_zone* pointer above.

These two used to be coupled, so you could only have 2 with 1, and the TimeZone class has an internal assumption that if 1 is nullptr, it goes ahead and makes the to_sys() and to_local() based on offsets.

Now, with this PR, we are making a change where we allow 2 to be created without 1. We need to have additional logic to prevent one from making time zone conversion if the internal date::time_zone pointer doesn't exist. We still need to allow 1 to exist without 2 if we want to support missing timezone in the local tzdata database, but we need to add code to prevent time zone conversion to happen using these "empty" TimeZone objects.

Thank you for the explanation. Which helps me understand the code's history a lot.

So, IIUC, one of the intentions of #10577 was to allow Velox convert time zones that don't exist in OS's timezone list, so we'd rather make the convertion continue than throw an error, am I understanding correctly?

This is the worst behaviour in Gluten before #10577:

During native library is loaded (process startup): No error

Converting an non-existing time zone: Error

The behaviour in Gluten after #10577:

During native library is loaded (process startup): Error (some zones don't exist in local tz list)

Converting an non-existing time zone: N/A

This PR so far:

During native library is loaded (process startup): No error (warning instead)

Converting an non-existing time zone: Error

So I think we should make both 1 and 2 pass without throwing, which was the intention of #10577 and relevant issues, is that correct?

So, IIUC, one of the intentions of #10577 was to allow Velox convert time zones that don't exist in OS's timezone list, so we'd rather make the convertion continue than throw an error, am I understanding correctly?

No, these are different. The PR above is to support conversions using fixed timezone offsets (things like "+09:00" and "-03:00"). These also don't exist in the time zone database, but they are different from a real time zone (say, "America/Los_Angeles") which may happen not to be available in the time zone database. For the latter case, we need to fail the conversion since we don't know the time zone potential daylight savings schedule.

zhztheplayer · 2024-08-06T05:27:49Z

I've updated the PR, CI error is unrelated I guess.

@pedroerp Thanks

zhztheplayer · 2024-08-08T03:21:59Z

kindly ping @pedroerp in case you missed

pedroerp

What happens when you try to use a timezone that could not be loaded? Could you double check if we have an assertion that makes it fail with a descriptive message, and prevent the binary from just crashing?

pedroerp · 2024-08-12T15:11:06Z

velox/external/date/tz.h

+    : public std::runtime_error
+{
+public:
+    invalid_timezone(const std::string& tz_name);


nit: why not just defining the body here inline?

Was just following the file's code style. tz.h doesn't likely adopt inline definitions.

pedroerp · 2024-08-12T15:14:44Z

velox/type/tz/TimeZoneMap.cpp

+      } catch (date::invalid_timezone& err) {
+        // Timezone not found in OS, skip it.
+        LOG(WARNING) << "Timezone [" << entry.second << "] not found due to: '"
+                     << err.what() << "', ignoring it. ";


concatenating these two messages will read weird. What about something like:

Unable to load "[timezone]" from local timezone database: error_msg

pedroerp · 2024-08-12T15:20:25Z

Do you mean when e.g., calling APIs in TimeZoneMap.h ?

There are two cases we need to make sure we cover:

An invalid time zone name. This is likely already covered today and returns a nice message.
A timezone that exists in the list, but where the internal pointer to the time zone database in external/date is null.

2 wouldn't happen before your PR, so it's likely that we don't have check for it. We will need to check both locateZone() functions, the one that searches based on a time zone name, and the one based on a time zone ID.

zhztheplayer · 2024-08-13T08:40:21Z

@pedroerp Did you mean to add a test to emulate the case that I described in PR description? If yes I've added one in the latest commits.

zhztheplayer · 2024-09-06T03:22:11Z

@pedroerp I would like to revisit this and appreciate for your help in advance.

A timezone that exists in the list, but where the internal pointer to the time zone database in external/date is null.

2 wouldn't happen before your PR, so it's likely that we don't have check for it.

If by timezone you meant Velox's TimeZone object, I think in the PR we already prevent a TimeZone object from being created if the internal date::time_zone cannot be found. See PR's code:

velox/velox/type/tz/TimeZoneMap.cpp

Lines 74 to 85 in 1b64d39

    
             const date::time_zone* zone; 
        
             try { 
        
               zone = locateZoneExt(entry.second); 
        
             } catch (date::invalid_timezone& err) { 
        
               // Timezone not found in OS, skip it. 
        
               LOG(WARNING) << "Unable to load [" << entry.second 
        
                            << "] from local timezone database: " << err.what(); 
        
               continue; 
        
             } 
        
             timeZonePtr = std::make_unique<TimeZone>(entry.second, entry.first, zone); 
        
           } 
        
           tzDatabase[entry.first] = std::move(timeZonePtr);

There is a continue; statement at line 81 to avoid creating a TimeZone object with nullptr date::time_zone in it.

Are we aligned here so far?

pedroerp

@zhztheplayer I'm just coming back from PTO, sorry about the delay on this.

Thank you for the iterations. Your comment is right, thanks for clarifying. Once we address the two small comments we can tag it as ready to merge.

pedroerp · 2024-09-09T16:33:01Z

velox/type/tz/TimeZoneMap.cpp

+        // Timezone not found in OS, skip it.
+        LOG(WARNING) << "Unable to load [" << entry.second
+                     << "] from local timezone database: " << err.what();
+        continue;


@zhztheplayer You're right, in this case we leave the entire TimeZone pointers as null, so it will do the right thing. Thanks for clarifying.

It would probably be worth it to add a small comment here explaining what in this case we continue the iteration and leave the pointer unchanged (nullptr), for the next unadvised reader like myself :)

pedroerp · 2024-09-09T16:33:58Z

velox/type/tz/TimeZoneMap.cpp

@@ -39,6 +42,12 @@ inline std::chrono::minutes getTimeZoneOffset(int16_t tzID) {
  return std::chrono::minutes{(tzID <= 840) ? (tzID - 841) : (tzID - 840)};
 }

+const date::time_zone* locateZoneExt(std::string_view tz_name) {


for consistency with other parts of the codebase, maybe call it locateZoneImpl()?

pedroerp

Thank you

zhztheplayer · 2024-09-10T23:56:29Z

Thank you

Thanks but UT had failures and I'll have another check on that.

fixup fixup fixup fixup fixup fixup fixup fixup fixup fixup retrigger ci fixup fixup [oap] Ignore timezone that is not available in OS

zhztheplayer · 2024-09-12T04:32:12Z

Thank you

Thanks but UT had failures and I'll have another check on that.

Fixed now, was a rebase issue.

facebook-github-bot · 2024-09-16T23:10:33Z

@pedroerp has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-09-18T03:09:37Z

@pedroerp merged this pull request in a8bd2de.

conbench-facebook · 2024-09-18T03:41:44Z

Conbench analyzed the 1 benchmark run on commit a8bd2dea.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…zone database (facebookincubator#10654) Summary: Related to discussion facebookincubator#10577 (comment) The patch fixes fatal error `<zone id> not found in timezone database` when at least one of the timezone IDs in [file](https://github.com/facebookincubator/velox/blob/main/velox/type/tz/TimeZoneDatabase.cpp) don't exist in OS's supported timezone list. Pull Request resolved: facebookincubator#10654 Reviewed By: kgpai Differential Revision: D62785133 Pulled By: pedroerp fbshipit-source-id: c8454454750040f8fdcbe053084f505d679f3142

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 5, 2024

zhztheplayer mentioned this pull request Aug 5, 2024

Add support for time zone offsets to TimeZone #10577

Closed

zhztheplayer commented Aug 5, 2024

View reviewed changes

zhztheplayer changed the title ~~Ignore time-zones that are not recognizable by OS when building time-zone database~~ Ignore time zones that are not recognizable by OS when building time zone database Aug 5, 2024

zhztheplayer force-pushed the wip-fix-tz branch 4 times, most recently from 970412c to 88538f2 Compare August 6, 2024 03:32

pedroerp reviewed Aug 12, 2024

View reviewed changes

pedroerp approved these changes Sep 9, 2024

View reviewed changes

pedroerp approved these changes Sep 10, 2024

View reviewed changes

pedroerp added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Sep 10, 2024

zhztheplayer marked this pull request as draft September 11, 2024 09:01

fixup

e9f3be1

fixup fixup fixup fixup fixup fixup fixup fixup fixup fixup retrigger ci fixup fixup [oap] Ignore timezone that is not available in OS

zhztheplayer force-pushed the wip-fix-tz branch from f1b814a to e9f3be1 Compare September 11, 2024 23:42

fixup

37201b7

zhztheplayer marked this pull request as ready for review September 12, 2024 00:54

facebook-github-bot closed this in a8bd2de Sep 18, 2024

facebook-github-bot added the Merged label Sep 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore time zones that are not recognizable by OS when building time zone database #10654

Ignore time zones that are not recognizable by OS when building time zone database #10654

zhztheplayer commented Aug 5, 2024 •

edited

Loading

netlify bot commented Aug 5, 2024 •

edited

Loading

zhztheplayer Aug 5, 2024

pedroerp Aug 5, 2024 •

edited

Loading

pedroerp Aug 5, 2024

zhztheplayer Aug 6, 2024 •

edited

Loading

pedroerp Aug 13, 2024

zhztheplayer Aug 13, 2024 •

edited

Loading

zhztheplayer Aug 13, 2024 •

edited

Loading

pedroerp Aug 14, 2024

zhztheplayer commented Aug 6, 2024

zhztheplayer commented Aug 8, 2024 •

edited

Loading

pedroerp left a comment

pedroerp Aug 12, 2024

zhztheplayer Aug 13, 2024 •

edited

Loading

pedroerp Aug 12, 2024

zhztheplayer Aug 13, 2024

pedroerp commented Aug 12, 2024

zhztheplayer commented Aug 13, 2024 •

edited

Loading

zhztheplayer commented Sep 6, 2024

pedroerp left a comment

pedroerp Sep 9, 2024

pedroerp Sep 9, 2024

pedroerp left a comment

zhztheplayer commented Sep 10, 2024

zhztheplayer commented Sep 12, 2024

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 18, 2024

conbench-facebook bot commented Sep 18, 2024

Ignore time zones that are not recognizable by OS when building time zone database #10654

Ignore time zones that are not recognizable by OS when building time zone database #10654

Conversation

zhztheplayer commented Aug 5, 2024 • edited Loading

netlify bot commented Aug 5, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

Choose a reason for hiding this comment

pedroerp Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhztheplayer Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhztheplayer Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

zhztheplayer Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhztheplayer commented Aug 6, 2024

zhztheplayer commented Aug 8, 2024 • edited Loading

pedroerp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhztheplayer Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedroerp commented Aug 12, 2024

zhztheplayer commented Aug 13, 2024 • edited Loading

zhztheplayer commented Sep 6, 2024

pedroerp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedroerp left a comment

Choose a reason for hiding this comment

zhztheplayer commented Sep 10, 2024

zhztheplayer commented Sep 12, 2024

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 18, 2024

conbench-facebook bot commented Sep 18, 2024

zhztheplayer commented Aug 5, 2024 •

edited

Loading

netlify bot commented Aug 5, 2024 •

edited

Loading

pedroerp Aug 5, 2024 •

edited

Loading

zhztheplayer Aug 6, 2024 •

edited

Loading

zhztheplayer Aug 13, 2024 •

edited

Loading

zhztheplayer Aug 13, 2024 •

edited

Loading

zhztheplayer commented Aug 8, 2024 •

edited

Loading

zhztheplayer Aug 13, 2024 •

edited

Loading

zhztheplayer commented Aug 13, 2024 •

edited

Loading