[GH-15810] Allow the user to adjust parquet import timezone #16304
Conversation
| @@ -303,25 +305,42 @@ public void addBinary(Binary value) { | |||
| } | |||
|
|
|||
| private static class TimestampConverter extends PrimitiveConverter { | |||
| public final static String TIMESTAMP_ADJUSTMENT = SYSTEM_PROP_PREFIX + "parquet.import.timestamp.adjustment"; | |||
There was a problem hiding this comment.
Can we add comment what value we expecting here? I saw it from the tests that it a number representing the hours. What happen if user put something outside of +-24?
There was a problem hiding this comment.
there's no limit basically, someone can adjust for example +- a month
| with tempfile.TemporaryDirectory() as dir: | ||
| timestamp_df = H2OFrame({"timestamp": '2024-06-06 06:06:06'}) | ||
| h2o.export_file(timestamp_df, path=dir, format="parquet", write_checksum=False) | ||
| imported_df = h2o.import_file(dir) |
There was a problem hiding this comment.
I wonder if it would be better to have this option as an export_file parameter..
I see you implement the workaround
manipulate the data before saving
What if user want to manipulate data into different timezones? In this case, they need to restart the cloud with the new jvm arguments.
Am I right or I just overthinking it and its not a probable usecase?
There was a problem hiding this comment.
And what if user want to adjust the timezone just for the one file, and leave it "as is" for the rest of them? It will also require a restart
There was a problem hiding this comment.
the story was that the users were importing a file and were expecting to see their timezone - the same behavior as Apache Spark if I'm not mistaken - spark is assuming the timestamp is in GMT and then showing it to the user in their own timezone - we do not do that, we just show the "real" value
so the purpose is for the system administrator to just set it so the users do not have to care about it
of course all those points you're mentioning are valid and it's possible that questions will come
There was a problem hiding this comment.
more technical user can adjust on their own though, but very valid points!
There was a problem hiding this comment.
The main user experience issue is that you need to think about the future import of parquet file when you start the h2o. I think its worth to have it as an import (not export) option in the function. WDYT @wendycwong?
Come to think of it, what happen if you import your file with sys.ai.h2o.parquet.import.timestamp.adjustment=2
and export it. IMHO it will export it with +2hours.
| @@ -795,6 +797,7 @@ def parse_setup(raw_frames, destination_frame=None, header=0, separator=None, co | |||
| :param partition_by: A list of columns the dataset has been partitioned by. None by default. | |||
| :param quotechar: A hint for the parser which character to expect as quoting character. Only single quote, double quote or None (default) are allowed. None means automatic detection. | |||
| :param escapechar: (Optional) One ASCII character used to escape other characters. | |||
| :param tz_adjustment: (Optional) Adjust the imported data timezone from GMT to cluster timezone. | |||
|
|
|||
There was a problem hiding this comment.
tz_adjustment is only for parsing parquet files, right? Please add the clarification.
| h2o.rapids(expr='(setTimeZone "America/Los_Angeles")') | ||
| expected_df = H2OFrame({"timestamp": '2024-08-02 12:00:00'}) | ||
| imported_df = h2o.import_file(dir, tz_adjustment=True) | ||
| assert imported_df[0, 0] == expected_df[0, 0] |
There was a problem hiding this comment.
Since the tz_adjustment is only for parquet, what happens if the user set tz_adjustment for say csv file? It should throw an error and telling user that tz_adjustment is only for parquet. Please add a test to check this.
|
Okay, I am feeling confused here. Assume that when we read in a file through import_file, the time zone used is UTC. Now, we can to load a parquet file and want to make sure the time column reflects the local time zone. @krasinski has used rapids to change the time zone to local time and then when doing import_file, a parameter is set to denote that correction to local time zone is requested. I suggest that this parameter to be part of the h2o frame. If the user wants to export the frame again, we can check what the parser time zone is set to and if the frame has been adjusted to local time. If the answer to both is yes, then we need to offset the time column again so that the final parquet file is saved with UTC time zone again. If the user set parser time zone to local time zone, however, when they import a parquet file but do not want to use local time zone, the parameter is set to false. Next, if the user wants to export the dataset again, we will check the parser time zone and the parameter to adjust to local time. In this case, even though the parser time zone is local time but the parameter to adjust time zone is false, we know the dataset time information is not offset and is still in UTC, we will not change the timezone during the export. If the user keep changing the parser time zone multiple times, things can get complicated. I was thinking perhaps we can have two parameters with each H2O frame: parser time zone when file is parsed, adjust_to_local time zone. In this case, if a user set parser time zone to Los_angeles, import_file a file with adjust_to_local_time_zone = True. Let's call this frame1, the parser time zone when file is parsed = Los_angeles, adjust_to_local_time_zone = true. Next, user change parser time zone to New_York and decide to export frame1 to a file. In this case, we see that the parser time zone is Los_Angeles and the time zone is adjusted to Los_angeles. In this case, during the export process, we need to add offset to Los_angeles time zone to UTC such that the export file time information is UTC. However, the major draw back is we are adding two parameters to the parser?????? |
|
|
||
| # import the file and see tz_adjust_to_local works | ||
| imported_df = h2o.import_file(dir + "/import", tz_adjust_to_local=True) | ||
| expected_df = H2OFrame({"timestamp": '2024-08-02 07:00:00'}) |
There was a problem hiding this comment.
This test is failing for me. I am running it on PST. So you need to adjust this answer to the local cluster time zone and expected_df should contain '2024-08-02 12:00:00' adjusted dynamically to the local cluster timezone.
|
I think what you have here will work. There is no ambiguity. If you import a parquet file with tz_adjust_to_loca = True, the time column will be presented with local cluster timezone. You just need to make sure you export_file with tz_adjust_to_loca = True again to get the correct time column. Minor issue with your test failing while running at my PST time zone. |
fix #15810