GH-34590: [C++][ORC] Fix timestamp type mapping between orc and arrow #34591

wgtmac · 2023-03-16T15:24:25Z

Rationale for this change

Background: There was an effort to fix inconsistent timestamp types across different SQL-on-Hadoop engines: https://docs.google.com/document/d/1gNRww9mZJcHvUDCXklzjFEQGpefsuR_akCDfWsdE35Q

In the Apache Orc, two timestamp types are provided:

TIMESTAMP: timestamp type without timezone, timestamp value is stored in the writer timezone .
TIMESTAMP_INSTANT: timestamp type with local timezone, timestamp value is stored in the UTC timezone.

arrow::TimestampType has an optional timezone field:

If timezone is provided, values are normalized in UTC.
If timezone is missing, values can be in any timezone.

What changes are included in this PR?

The type mapping is fixed as below:

orc::TIMESTAMP <=> arrow::TimestampType w/o timezone
orc::TIMESTAMP_INSTANT <=> arrow::TimestampType w/ timezone

Are these changes tested?

Make sure all tests pass.

Are there any user-facing changes?

No.

Closes: [C++][ORC] Fix timestamp type mapping between orc and arrow #34590

github-actions · 2023-03-16T15:24:51Z

Closes: [C++][ORC] Fix timestamp type mapping between orc and arrow #34590

github-actions · 2023-03-16T15:24:54Z

⚠️ GitHub issue #34590 has been automatically assigned in GitHub to PR creator.

… arrow

wgtmac · 2023-03-17T14:48:32Z

@westonpace @wjones127 @lidavidm Could you please take a look? Thanks!

wjones127 · 2023-03-17T18:22:21Z

cpp/src/arrow/adapters/orc/util.cc

+      // The timestamp values stored in ORC are in the writer timezone.
      return timestamp(TimeUnit::NANO);


Is this true? I would have expected the values in the "UTC" timezone and the expectations is readers should interpret it in the local time zone.

Yes, you're right.

For orc::TIMESTAMP type:

The Orc writer expects input data (i.e. in the orc::TimestampVectorBatch) to be in the "UTC" timezone and serializes it into the writer timezone: https://github.com/apache/orc/blob/main/c%2B%2B/src/ColumnWriter.cc#L1717

The Orc reader deserializes the data from writer timezone and restores it into reader timezone: https://github.com/apache/orc/blob/main/c%2B%2B/src/ColumnReader.cc#L336

For orc::TIMESTAMP_INSTANT type:

The Orc writer expects input data to be in the "UTC" timezone and serializes it into the "UTC" timezone: https://github.com/apache/orc/blob/main/c%2B%2B/src/ColumnWriter.cc#L1644

The Orc reader deserializes the data from "UTC" timezone and no more conversion is needed because writerTimezone and readerTimezone are both "UTC": https://github.com/apache/orc/blob/main/c%2B%2B/src/ColumnReader.cc#L282

We have seen many issues around orc::TIMESTAMP type because of the writer-reader timezone conversion, especially with different day-light saving rules. So that's why orc::TIMESTAMP_INSTANT type is added and is always preferred over orc::TIMESTAMP type if user can take care of the timezone.

Okay that makes sense. It does make me wonder if we should then be applying the local time zone to these types, instead of using naive timestamps:

Suggested change

// The timestamp values stored in ORC are in the writer timezone.

return timestamp(TimeUnit::NANO);

// The timestamp values stored in ORC are in the writer timezone.

return timestamp(TimeUnit::NANO, GetLocalTimeZone());

This would fail the equality check of timestamp types in the roundtrip tests. Additionally, it seems that there is a lack of utility like GetLocalTimeZone()?

IMHO, if writer uses naive timestamp types, reader should follow the same pattern.

wgtmac · 2023-03-21T03:23:17Z

cpp/src/arrow/adapters/orc/util.cc

@@ -1111,7 +1121,20 @@ Result<std::shared_ptr<DataType>> GetArrowType(const liborc::Type* type) {
    case liborc::CHAR:
      return fixed_size_binary(static_cast<int>(type->getMaximumLength()));
    case liborc::TIMESTAMP:
+      // Values of TIMESTAMP type are stored in the writer timezone in the Orc file.


I have added extensive comment to explain the issue and suggested usage. Please check again. Thanks @wjones127

wjones127

Thank you for adding those comments!

ursabot · 2023-03-21T20:55:46Z

Benchmark runs are scheduled for baseline = ba1f992 and contender = 1f8a335. 1f8a335 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.82% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.51% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.25% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 1f8a335d ec2-t3-xlarge-us-east-2
[Finished] 1f8a335d test-mac-arm
[Finished] 1f8a335d ursa-i9-9960x
[Finished] 1f8a335d ursa-thinkcentre-m75q
[Finished] ba1f9924 ec2-t3-xlarge-us-east-2
[Failed] ba1f9924 test-mac-arm
[Finished] ba1f9924 ursa-i9-9960x
[Finished] ba1f9924 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

… arrow (apache#34591) ### Rationale for this change Background: There was an effort to fix inconsistent timestamp types across different SQL-on-Hadoop engines: https://docs.google.com/document/d/1gNRww9mZJcHvUDCXklzjFEQGpefsuR_akCDfWsdE35Q In the Apache Orc, two timestamp types are provided: - TIMESTAMP: timestamp type without timezone, timestamp value is stored in the writer timezone . - TIMESTAMP_INSTANT: timestamp type with local timezone, timestamp value is stored in the UTC timezone. arrow::TimestampType has an optional timezone field: - If timezone is provided, values are normalized in UTC. - If timezone is missing, values can be in any timezone. ### What changes are included in this PR? The type mapping is fixed as below: - orc::TIMESTAMP <=> arrow::TimestampType w/o timezone - orc::TIMESTAMP_INSTANT <=> arrow::TimestampType w/ timezone ### Are these changes tested? Make sure all tests pass. ### Are there any user-facing changes? No. * Closes: apache#34590 Authored-by: Gang Wu <[email protected]> Signed-off-by: Will Jones <[email protected]>

github-actions bot added Component: C++ awaiting review Awaiting review labels Mar 16, 2023

apacheGH-34590: [C++][ORC] Fix timestamp type mapping between orc and…

ecfc43e

… arrow

wgtmac force-pushed the orc_timestamp branch from 65f988a to ecfc43e Compare March 16, 2023 15:33

wjones127 self-requested a review March 17, 2023 18:10

wjones127 reviewed Mar 17, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Mar 17, 2023

explictly set UTC to orc writer/reader

ee0960e

wgtmac force-pushed the orc_timestamp branch from d1c7849 to ee0960e Compare March 21, 2023 03:22

wgtmac commented Mar 21, 2023

View reviewed changes

wgtmac requested a review from wjones127 March 21, 2023 03:43

wjones127 approved these changes Mar 21, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Mar 21, 2023

wjones127 merged commit 1f8a335 into apache:main Mar 21, 2023

Jefffrey mentioned this pull request Mar 25, 2024

Timestamp instant support datafusion-contrib/datafusion-orc#13

Closed

Jefffrey mentioned this pull request Apr 2, 2024

ORC-1671: update timestamp doc in specfication apache/orc#1867

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-34590: [C++][ORC] Fix timestamp type mapping between orc and arrow #34591

GH-34590: [C++][ORC] Fix timestamp type mapping between orc and arrow #34591

wgtmac commented Mar 16, 2023 •

edited

Loading

github-actions bot commented Mar 16, 2023

github-actions bot commented Mar 16, 2023

wgtmac commented Mar 17, 2023

wjones127 Mar 17, 2023

wgtmac Mar 20, 2023

wjones127 Mar 20, 2023

wgtmac Mar 21, 2023

wgtmac Mar 21, 2023

wjones127 left a comment

ursabot commented Mar 21, 2023

		// The timestamp values stored in ORC are in the writer timezone.
		return timestamp(TimeUnit::NANO);

GH-34590: [C++][ORC] Fix timestamp type mapping between orc and arrow #34591

GH-34590: [C++][ORC] Fix timestamp type mapping between orc and arrow #34591

Conversation

wgtmac commented Mar 16, 2023 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Mar 16, 2023

github-actions bot commented Mar 16, 2023

wgtmac commented Mar 17, 2023

wjones127 Mar 17, 2023

Choose a reason for hiding this comment

wgtmac Mar 20, 2023

Choose a reason for hiding this comment

wjones127 Mar 20, 2023

Choose a reason for hiding this comment

wgtmac Mar 21, 2023

Choose a reason for hiding this comment

wgtmac Mar 21, 2023

Choose a reason for hiding this comment

wjones127 left a comment

Choose a reason for hiding this comment

ursabot commented Mar 21, 2023

wgtmac commented Mar 16, 2023 •

edited

Loading