[GitHub] [orc] wgtmac commented on a change in pull request #683: ORC-784:[C++] Read/Write timestamp column with the specified timezone option

[GitBox <gi...@apache.org>]

wgtmac commented on a change in pull request #683:
URL: https://github.com/apache/orc/pull/683#discussion_r614517380



##########
File path: c++/src/Writer.cc
##########
@@ -56,6 +57,7 @@ namespace orc {
       enableIndex = true;
       bloomFilterFalsePositiveProb = 0.05;
       bloomFilterVersion = UTF8;
+      timezone = "GMT";

Review comment:
       The Java ORC writer uses local time zone by default. However, the C++ ORC writer uses GMT by default in this [fix](https://github.com/apache/orc/commit/8ea3592f6aeddb5e6930798e4ff95065c6177e00). I hard-coded it to GMT at that time to work around the insufficient support of timezone conversion on the C++ side. I'd suggest to leave some comments to explain this history.

##########
File path: c++/test/TestWriter.cc
##########
@@ -699,6 +704,98 @@ namespace orc {
     }
   }
 
+#ifndef _MSC_VER

Review comment:
       leave some comments to explain why you skip windows here.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org