ORC-1055: [C++] Add the timezone option for the csv-import tool #975

coderex2522 · 2021-12-23T02:35:00Z

What changes were proposed in this pull request?

The pull request provides the csv-import tool with support for timezone settings

Why are the changes needed?

This is a new option to mitigate ORC-1055 situation.

How was this patch tested?

The unit case is TestCSVFileImport.testTimezoneOption in TestCSVFileImport.cc

…Options

coderex2522 · 2021-12-24T03:18:47Z

@wgtmac Hi, please take a look!

coderex2522 · 2021-12-24T06:27:48Z

These timezones alias information is a reference to https://github.com/frohoff/jdk8u-dev-jdk/blob/master/src/share/classes/sun/util/calendar/ZoneInfoFile.java line:220-244

dongjoon-hyun

Thank you for making a PR, @coderex2522 .

In general, we need a test case which failed before your PR. Do you think you can add a test coverage for that?
This PR proposes to introduce alias which is borrowed from Java oldMappings. Does this PR handle both two JVM cases sun.timezone.ids.oldmapping=true/false?

wgtmac · 2021-12-27T04:21:03Z

ed before your PR. Do

+1 for adding a test case. We can directly use the csv file from the JIRA.

After some investigation, I think the Java odlMappings may introduce new issues. For example, CST can either be Central Standard Time (America/Chicago) or China Standard Time (Asia/Shanghai).

dongjoon-hyun · 2021-12-28T02:17:19Z

Thank you for updating, @coderex2522 . Could you revise the PR title and description according to the new code?

coderex2522 · 2021-12-28T02:59:25Z

The PR title and descrition has been modified, @dongjoon-hyun please take a look again!

wgtmac · 2021-12-28T14:32:25Z

tools/src/CSVFileImport.cc

            << "                  <schema> <input> <output>\n"
            << "Import CSV file into an Orc file using the specified schema.\n"
+            << "The timezone can be viewed in the directory /usr/share/zoneinfo\n"


It should be stated clearly that the timezone is writer timezone of timestamp types.

You have specified it as a required_argument which conflicts with the comment here.

tools/test/TestCSVFileImport.cc

dongjoon-hyun · 2021-12-29T08:34:24Z

tools/test/TestCSVFileImport.cc

+  std::string output;
+  std::string error;
+
+  std::string option = "--timezone=America/Los_Angeles";


Maybe, this test case always succeeds at America/Los_Angeles timezone? If then, can we choose less popular timezone?

If we don't set timezone for the WriterOptions, the writer will write the timestamp with the GMT timezone. And the orc-contents tool will scan the content in the GMT timezone.This case should not be successful?

We can add another test to use timezone as Europe/Paris or Asia/Shanghai.

@coderex2522 . Yes, the test case should fail without your patch and should pass with your patch. That's the purpose of verification of your code's contribution.

And, +1 for @wgtmac 's suggestion.

We can add another test to use timezone as Europe/Paris or Asia/Shanghai.

Add the timezone 'Europe/Paris' test case done.

coderex2522 · 2022-01-04T09:41:42Z

@williamhyun Hi, can you help investigate this problem in continuous-integration/appveyor/pr?
The error message shows "The build phase is set to "MSBuild" mode (default), but no Visual Studio project or solution files were found in the root directory. If you are not building Visual Studio project switch build mode to "Script" and provide your custom build command."

guiyanakuang · 2022-01-04T09:51:39Z

@coderex2522 Don't worry, it has nothing to do with your pr
Appveyor's webhook is being removed.
https://issues.apache.org/jira/browse/INFRA-22692

wgtmac

LGTM +1

dongjoon-hyun

+1, LGTM. Thank you, @coderex2522 , @wgtmac , @guiyanakuang .

dongjoon-hyun · 2022-01-04T19:59:00Z

Merged to main/branch-1.7 for Apache ORC 1.7.3.

cc @williamhyun

### What changes were proposed in this pull request? The pull request provides the csv-import tool with support for timezone settings ### Why are the changes needed? This is a new option to mitigate ORC-1055 situation. ### How was this patch tested? The unit case is TestCSVFileImport.testTimezoneOption in TestCSVFileImport.cc (cherry picked from commit 9a66348) Signed-off-by: Dongjoon Hyun <[email protected]>

…he#975) ### What changes were proposed in this pull request? The pull request provides the csv-import tool with support for timezone settings ### Why are the changes needed? This is a new option to mitigate ORC-1055 situation. ### How was this patch tested? The unit case is TestCSVFileImport.testTimezoneOption in TestCSVFileImport.cc

ORC-1055: [C++] Add timezone options for csv-import tool

73e5714

github-actions bot added the CPP label Dec 23, 2021

mingshen.zx and others added 3 commits December 23, 2021 14:21

ORC-1055: [C++] The csv-import tool use local timezone for the Writer…

cfa1cbe

…Options

Merge remote-tracking branch 'origin/ORC-1055'

04e0a75

ORC-1055: [C++] add the timezone alias transform function

645983e

coderex2522 changed the title ~~ORC-1055: [C++] Add timezone options for csv-import tool~~ ORC-1055: [C++] Set timezone writer options for csv-import tool Dec 24, 2021

dongjoon-hyun requested changes Dec 25, 2021

View reviewed changes

ORC-1055: [C++] add the timezone option for csv-import tool

4503e2c

github-actions bot added the EXAMPLES label Dec 27, 2021

coderex2522 requested a review from dongjoon-hyun December 28, 2021 01:49

coderex2522 changed the title ~~ORC-1055: [C++] Set timezone writer options for csv-import tool~~ ORC-1055: [C++] Add the timezone option for the csv-import tool Dec 28, 2021

wgtmac reviewed Dec 28, 2021

View reviewed changes

ORC-1055: [C++] modify the option description of the csv-import tool

f065e0f

dongjoon-hyun reviewed Dec 29, 2021

View reviewed changes

tools/test/TestCSVFileImport.cc Show resolved Hide resolved

dongjoon-hyun reviewed Dec 29, 2021

View reviewed changes

coderex2522 added 2 commits January 4, 2022 10:53

ORC-1055: [C++] modify the test case for the csv-import tool

04b6c3c

Merge branch 'apache:main' into ORC-1055

23fe58c

wgtmac approved these changes Jan 4, 2022

View reviewed changes

dongjoon-hyun added this to the 1.7.3 milestone Jan 4, 2022

dongjoon-hyun approved these changes Jan 4, 2022

View reviewed changes

dongjoon-hyun merged commit 9a66348 into apache:main Jan 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ORC-1055: [C++] Add the timezone option for the csv-import tool #975

ORC-1055: [C++] Add the timezone option for the csv-import tool #975

coderex2522 commented Dec 23, 2021 •

edited by dongjoon-hyun

Loading

coderex2522 commented Dec 24, 2021

coderex2522 commented Dec 24, 2021

dongjoon-hyun left a comment

wgtmac commented Dec 27, 2021

dongjoon-hyun commented Dec 28, 2021 •

edited

Loading

coderex2522 commented Dec 28, 2021

wgtmac Dec 28, 2021

dongjoon-hyun Dec 29, 2021

coderex2522 Dec 30, 2021

wgtmac Dec 30, 2021

dongjoon-hyun Dec 30, 2021 •

edited

Loading

coderex2522 Jan 4, 2022

coderex2522 commented Jan 4, 2022

guiyanakuang commented Jan 4, 2022 •

edited

Loading

wgtmac left a comment

dongjoon-hyun left a comment

dongjoon-hyun commented Jan 4, 2022

ORC-1055: [C++] Add the timezone option for the csv-import tool #975

ORC-1055: [C++] Add the timezone option for the csv-import tool #975

Conversation

coderex2522 commented Dec 23, 2021 • edited by dongjoon-hyun Loading

What changes were proposed in this pull request?

Why are the changes needed?

How was this patch tested?

coderex2522 commented Dec 24, 2021

coderex2522 commented Dec 24, 2021

dongjoon-hyun left a comment

Choose a reason for hiding this comment

wgtmac commented Dec 27, 2021

dongjoon-hyun commented Dec 28, 2021 • edited Loading

coderex2522 commented Dec 28, 2021

wgtmac Dec 28, 2021

Choose a reason for hiding this comment

dongjoon-hyun Dec 29, 2021

Choose a reason for hiding this comment

coderex2522 Dec 30, 2021

Choose a reason for hiding this comment

wgtmac Dec 30, 2021

Choose a reason for hiding this comment

dongjoon-hyun Dec 30, 2021 • edited Loading

Choose a reason for hiding this comment

coderex2522 Jan 4, 2022

Choose a reason for hiding this comment

coderex2522 commented Jan 4, 2022

guiyanakuang commented Jan 4, 2022 • edited Loading

wgtmac left a comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Jan 4, 2022

coderex2522 commented Dec 23, 2021 •

edited by dongjoon-hyun

Loading

dongjoon-hyun commented Dec 28, 2021 •

edited

Loading

dongjoon-hyun Dec 30, 2021 •

edited

Loading

guiyanakuang commented Jan 4, 2022 •

edited

Loading