ORC-1053: Fix time zone offset precision when convert tool converts `LocalDateTime` to `Timestamp` is not consistent with the internal default precision of ORC #967

guiyanakuang · 2021-12-01T10:50:56Z

What changes were proposed in this pull request?

// use tool compute offset : 17762
int toolOffset = ((LocalDateTime) temporalAccessor).atZone(TimeZone.getTimeZone("America/New_York").toZoneId()).getOffset().getTotalSeconds();

// in orc internal compute offset: 18000
int orcInternalOffset = TimeZone.getTimeZone("America/New_York").getRawOffset() / 1000

This pr is designed to modify the implementation of the LocalDateTime to Timestamp conversion so that the time zone accuracy is consistent with the ORC internal accuracy during the conversion

Why are the changes needed?

Avoid inconsistencies between converted data and expected data from convert tools.

How was this patch tested?

Add issue-specific unit test

…onverts LocalDateTime to timestamp is not consistent with the internal default precision of ORC

guiyanakuang · 2021-12-01T10:55:57Z

cc @dongjoon-hyun @williamhyun : )

dongjoon-hyun · 2021-12-01T17:03:17Z

Thank you so much, @guiyanakuang !

dongjoon-hyun · 2021-12-01T17:05:12Z

java/tools/src/java/org/apache/orc/tools/convert/CsvReader.java

@@ -372,4 +370,15 @@ Converter buildConverter(IntWritable startOffset, TypeDescription schema) {
        throw new IllegalArgumentException("Unhandled type " + schema);
    }
  }
+
+  public static void main(String[] args) {


Could you remove this please?

Very sorry for committing some validation code

dongjoon-hyun · 2021-12-01T17:36:37Z

java/tools/src/test/org/apache/orc/tools/convert/TestConvert.java

+  }
+
+  @Test
+  public void testConvertCustomTimestampFromCsv() throws IOException, ParseException {


Thank you for adding this.

dongjoon-hyun · 2021-12-01T17:45:48Z

java/tools/src/test/org/apache/orc/tools/convert/TestConvert.java

+      for (int row = 0; row < batch.size; ++row) {
+        Timestamp timestamp = Timestamp.valueOf(timeValues[row]);
+        assertEquals(timestamp.getTime(), tcv.time[row]);
+        assertEquals(timestamp.getNanos(), tcv.nanos[row]);


This seems to pass without your patch. Could you verify this again?

I verified before submitting that my local default timezone, and the America/New_York timezone on which jira uploaded the file, would not pass the test based on past code.

The reason for this is the inconsistent interpretation of the timestamp offfset accuracy when writing and reading.

Timestamp timestamp1 = Timestamp.from(localDateTime.toInstant()); Timestamp timestamp2 = Timestamp.valueOf(localDateTime);

Both methods use the local time zone to calculate the timestamp, but the offset precision of the time zone of the previous method is seconds (for New York is 17762 s), and the precision of the latter method is hours (for New York is 18000 s, which is 5 hours)
And the readings are interpreted by the latter precision (hour)

I ran this yesterday and today. It passed on my laptop without your patch.

$ git diff main --stat java/tools/src/test/org/apache/orc/tools/convert/TestConvert.java | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) $ mvn package -pl tools -Dtest=org.apache.orc.tools.convert.TestConvert ... [INFO] ------------------------------------------------------- [INFO] T E S T S [INFO] ------------------------------------------------------- [INFO] Running org.apache.orc.tools.convert.TestConvert Processing test.csv [INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.337 s - in org.apache.orc.tools.convert.TestConvert [INFO] [INFO] Results: [INFO] [INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0 ...

The difference between you and me seems to be the timezones. You are in Asia timezone and I'm in America/Los Angeles timezone.

Can we make the test case to be independent from the tester's environment?

Maybe I should specify the time zone to test, but there are many methods inside ORC to get the local default time zone, so it's not good to cover it uniformly at the moment, I need to think.

@dongjoon-hyun I added two hooks using annotations to ensure that the default time zone during testing this class is New York, which feels good. Also I removed a redundant statement 😅.
You can try again when you have time, I'm sure the old code will not pass the test.

dongjoon-hyun · 2021-12-01T17:46:47Z

Also, cc @wgtmac since C++ tool seems to have the same issue according to the JIRA.

guiyanakuang · 2021-12-02T09:56:43Z

Also, cc @wgtmac since C++ tool seems to have the same issue according to the JIRA.

I have also debugged some of the C++ code. It looks like strptime can't convert values before 1970 on mac os.

orc/tools/src/CSVFileImport.cc

Line 257 in 334bf1f

char *left=strptime(col.c_str(), "%Y-%m-%d %H:%M:%S", &timeStruct);

dongjoon-hyun

+1, LGTM. Thank you, @guiyanakuang .
For C++ case, could you file a new JIRA for that?
ORC-1053 is enough for Java-only patch

…LocalDateTime` to `Timestamp` is not consistent with the internal default precision of ORC (#967) ### What changes were proposed in this pull request? ```java // use tool compute offset : 17762 int toolOffset = ((LocalDateTime) temporalAccessor).atZone(TimeZone.getTimeZone("America/New_York").toZoneId()).getOffset().getTotalSeconds(); // in orc internal compute offset: 18000 int orcInternalOffset = TimeZone.getTimeZone("America/New_York").getRawOffset() / 1000 ``` This pr is designed to modify the implementation of the LocalDateTime to Timestamp conversion so that the time zone accuracy is consistent with the ORC internal accuracy during the conversion ### Why are the changes needed? Avoid inconsistencies between converted data and expected data from convert tools. ### How was this patch tested? Add issue-specific unit test (cherry picked from commit e787b8b) Signed-off-by: Dongjoon Hyun <[email protected]>

dongjoon-hyun · 2021-12-03T07:33:14Z

Merged to main/1.7.

guiyanakuang · 2021-12-03T07:53:20Z

+1, LGTM. Thank you, @guiyanakuang . For C++ case, could you file a new JIRA for that? ORC-1053 is enough for Java-only patch

Okay, I created the ORC-1055

dongjoon-hyun · 2021-12-03T08:08:56Z

Thank you!

wgtmac · 2021-12-03T09:41:56Z

Also, cc @wgtmac since C++ tool seems to have the same issue according to the JIRA.

I have also debugged some of the C++ code. It looks like strptime can't convert values before 1970 on mac os.

orc/tools/src/CSVFileImport.cc

Line 257 in 334bf1f

char *left=strptime(col.c_str(), "%Y-%m-%d %H:%M:%S", &timeStruct);

Pre-1970 timestamp is a notorious issue. Not sure if these test cases will help:
https://github.com/apache/orc/blob/main/c%2B%2B/test/TestWriter.cc#L621
https://github.com/apache/orc/blob/main/c%2B%2B/test/TestWriter.cc#L741

guiyanakuang · 2021-12-03T10:08:13Z

@wgtmac , Unfortunately I think this is a new issue.

int main(int,char* argv[]){
  struct tm timeStruct;
  std::string name = "0001-01-01 00:00:00.000";
  char *left=strptime(name.c_str(), "%Y-%m-%d %H:%M:%S", &timeStruct);
  time_t _time = timegm(&timeStruct);
  char *tail;
  double d = strtod(left, &tail);
  long na;
  if (tail != left) {
    na = static_cast<long>(d * 1000000000.0);
  } else {
    na = 0;
  }
  std::cout << _time << " " << na << std::endl;
}

On my mac machine the output is "-1 0"
Using the online C++ runtime environment claimed on Linux, the output is "-62135596800 0"
Time format strings that appear to be older than 1970 get -1 on the mac.

https://github.com/apache/orc/blob/main/c%2B%2B/test/TestWriter.cc#L621
https://github.com/apache/orc/blob/main/c%2B%2B/test/TestWriter.cc#L741

It looks like the first test verifies that we can write time as a negative number.
The second one holds our custom time zone. So I think this is a new issue.

Of course I verified the converted_by_cpp.orc uploaded by the user in ORC-1055, and it is clear that it is not converted on the mac, so there should be another issue that is not yet clear

wgtmac · 2021-12-06T02:37:13Z

@wgtmac , Unfortunately I think this is a new issue.
int main(int,char* argv[]){
  struct tm timeStruct;
  std::string name = "0001-01-01 00:00:00.000";
  char *left=strptime(name.c_str(), "%Y-%m-%d %H:%M:%S", &timeStruct);
  time_t _time = timegm(&timeStruct);
  char *tail;
  double d = strtod(left, &tail);
  long na;
  if (tail != left) {
    na = static_cast<long>(d * 1000000000.0);
  } else {
    na = 0;
  }
  std::cout << _time << " " << na << std::endl;
}
On my mac machine the output is "-1 0" Using the online C++ runtime environment claimed on Linux, the output is "-62135596800 0" Time format strings that appear to be older than 1970 get -1 on the mac.

https://github.com/apache/orc/blob/main/c%2B%2B/test/TestWriter.cc#L621
https://github.com/apache/orc/blob/main/c%2B%2B/test/TestWriter.cc#L741

It looks like the first test verifies that we can write time as a negative number. The second one holds our custom time zone. So I think this is a new issue.

Of course I verified the converted_by_cpp.orc uploaded by the user in ORC-1055, and it is clear that it is not converted on the mac, so there should be another issue that is not yet clear

Thanks for the input. I will take a look later this week.

dongjoon-hyun · 2021-12-06T03:00:35Z

Thank you so much, @guiyanakuang and @wgtmac .

Fix the issue that the time zone offset precision when convert tool c…

7d97cfa

…onverts LocalDateTime to timestamp is not consistent with the internal default precision of ORC

github-actions bot added the JAVA label Dec 1, 2021

Remove UnusedImport

bb62d55

dongjoon-hyun reviewed Dec 1, 2021

View reviewed changes

Remove redundant addition

b3cd24c

dongjoon-hyun reviewed Dec 1, 2021

View reviewed changes

guiyanakuang added 3 commits December 2, 2021 17:32

Make the test time zone, independent of the host environment

047396f

Merge remote-tracking branch 'origin/ORC-1053' into ORC-1053

4ee56d1

Remove redundant call

2e7ce98

dongjoon-hyun approved these changes Dec 3, 2021

View reviewed changes

dongjoon-hyun merged commit e787b8b into apache:main Dec 3, 2021

dongjoon-hyun added this to the 1.7.2 milestone Dec 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ORC-1053: Fix time zone offset precision when convert tool converts `LocalDateTime` to `Timestamp` is not consistent with the internal default precision of ORC #967

ORC-1053: Fix time zone offset precision when convert tool converts `LocalDateTime` to `Timestamp` is not consistent with the internal default precision of ORC #967

guiyanakuang commented Dec 1, 2021 •

edited

Loading

guiyanakuang commented Dec 1, 2021

dongjoon-hyun commented Dec 1, 2021

dongjoon-hyun Dec 1, 2021

guiyanakuang Dec 1, 2021

dongjoon-hyun Dec 1, 2021

dongjoon-hyun Dec 1, 2021

guiyanakuang Dec 1, 2021

guiyanakuang Dec 2, 2021

dongjoon-hyun Dec 2, 2021

dongjoon-hyun Dec 2, 2021

dongjoon-hyun Dec 2, 2021

guiyanakuang Dec 2, 2021

guiyanakuang Dec 2, 2021

dongjoon-hyun commented Dec 1, 2021

guiyanakuang commented Dec 2, 2021

dongjoon-hyun left a comment

dongjoon-hyun commented Dec 3, 2021

guiyanakuang commented Dec 3, 2021

dongjoon-hyun commented Dec 3, 2021

wgtmac commented Dec 3, 2021

guiyanakuang commented Dec 3, 2021

wgtmac commented Dec 6, 2021

dongjoon-hyun commented Dec 6, 2021

ORC-1053: Fix time zone offset precision when convert tool converts LocalDateTime to Timestamp is not consistent with the internal default precision of ORC #967

ORC-1053: Fix time zone offset precision when convert tool converts LocalDateTime to Timestamp is not consistent with the internal default precision of ORC #967

Conversation

guiyanakuang commented Dec 1, 2021 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

How was this patch tested?

guiyanakuang commented Dec 1, 2021

dongjoon-hyun commented Dec 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Dec 1, 2021

guiyanakuang commented Dec 2, 2021

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Dec 3, 2021

guiyanakuang commented Dec 3, 2021

dongjoon-hyun commented Dec 3, 2021

wgtmac commented Dec 3, 2021

guiyanakuang commented Dec 3, 2021

wgtmac commented Dec 6, 2021

dongjoon-hyun commented Dec 6, 2021

ORC-1053: Fix time zone offset precision when convert tool converts `LocalDateTime` to `Timestamp` is not consistent with the internal default precision of ORC #967

ORC-1053: Fix time zone offset precision when convert tool converts `LocalDateTime` to `Timestamp` is not consistent with the internal default precision of ORC #967

guiyanakuang commented Dec 1, 2021 •

edited

Loading