wgtmac commented on code in PR #1088:
URL: https://github.com/apache/orc/pull/1088#discussion_r851858410
##########
c++/src/ColumnWriter.cc:
##########
@@ -1837,7 +1837,7 @@ namespace orc {
// TimestampVectorBatch already stores data in UTC
int64_t millsUTC = secs[i] * 1000 + nanos[i] / 1000000;
if (!isUTC) {
- millsUTC = timezone.convertToUTC(millsUTC);
+ millsUTC = timezone.convertToUTC(secs[i]) * 1000 + nanos[i] /
1000000;
Review Comment:
If we have set nanosecond in the test case
_**WriterTest.writeTimestampWithTimezone**_, this issue may be fixed earlier.
##########
c++/src/ColumnWriter.cc:
##########
@@ -1837,7 +1837,7 @@ namespace orc {
// TimestampVectorBatch already stores data in UTC
int64_t millsUTC = secs[i] * 1000 + nanos[i] / 1000000;
if (!isUTC) {
- millsUTC = timezone.convertToUTC(millsUTC);
+ millsUTC = timezone.convertToUTC(secs[i]) * 1000 + nanos[i] /
1000000;
Review Comment:
> Yes, it's a little counter-intuitive because we only convert `secs[i]`
only. So, `Timezone.convertToUTC` doesn't care `nanos` part properly and we
should not put it here?
>
>
https://github.com/apache/orc/blob/f4c7cc14b31876e3dd4934b4bfc40c05d4b8ebca/c%2B%2B/src/Timezone.cc#L604-L606
>
> If then, can we fix it in `Timezone.convertToUTC` instead? Is there a
side-effect?
I think the current fix is good enough. As time_t uses second internally,
we'd better keep the contract of Timezone.convertToUTC. We may add some comment
above the fix to help understanding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
