lightning: support importing timestamp from Hive parquet #37685

buchuitoudegou · 2022-09-07T09:38:13Z

What problem does this PR solve?

Issue Number: close #37536

Problem Summary:

What is changed and how it works?

The timestamp exported from Hive will be encoded as INT96 which is natively supported by neither parquet-go nor Golang itself. Therefore, this type of timestamp will be stored as strings in lightning. Because we cannot extract timestamps from this byte string, the import result is kind of fallacious.

This PR is about to identify this type of timestamp and decode true time from it.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

ti-chi-bot · 2022-09-07T09:38:16Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

D3Hunter
lance6716

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

buchuitoudegou · 2022-09-07T09:38:56Z

/component lightning
/cc @dsdashun @D3Hunter @lance6716

lance6716 · 2022-09-07T11:19:10Z

br/pkg/lightning/mydump/parquet_parser.go

@@ -446,6 +449,13 @@ func setDatumByString(d *types.Datum, v string, meta *parquet.SchemaElement) {
 	if meta.LogicalType != nil && meta.LogicalType.DECIMAL != nil {
 		v = binaryToDecimalStr([]byte(v), int(meta.LogicalType.DECIMAL.Scale))
 	}
+	if meta.Type != nil && *meta.Type == parquet.Type_INT96 && len([]byte(v)) == 12 {


Suggested change

if meta.Type != nil && *meta.Type == parquet.Type_INT96 && len([]byte(v)) == 12 {

if meta.Type != nil && *meta.Type == parquet.Type_INT96 && len(v) == 12 {

Or maybe use 96/8 to replace 12 😂

what's the value of LogicalType if enter this branch

No logical type is set (nil).

lance6716 · 2022-09-07T11:22:56Z

br/pkg/lightning/mydump/parquet_parser.go

+	if meta.Type != nil && *meta.Type == parquet.Type_INT96 && len([]byte(v)) == 12 {
+		ts := int96ToTime([]byte(v))
+		ts = ts.UTC()
+		tsStr := ts.Format("2006-01-02 15:04:05.999999Z")


why do we choose this layout?

copy from

tidb/br/pkg/lightning/mydump/parquet_parser.go

Lines 512 to 513 in ec9e43e

timeStr := formatTime(v, logicalType.TIMESTAMP.Unit, "2006-01-02 15:04:05.999999",

"2006-01-02 15:04:05.999999Z", logicalType.TIMESTAMP.IsAdjustedToUTC)

This layout is used when setting a UTC timestamp.

lance6716 · 2022-09-07T11:25:15Z

br/pkg/lightning/mydump/parquet_parser.go

+	return time.Unix(sec, int64(nsec))
+}
+
+func int96ToTime(parquetDate []byte) time.Time {


can you refer to a parquet encoding for timestamp as comment?

https://github.com/apache/spark/blob/d66a4e82eceb89a274edeb22c2fb4384bed5078b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetWriteSupport.scala#L171-L178

https://github.com/apache/spark/blob/9ee2c753b98b290fab9b2ec1f02d90c7c9441271/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala#L235-L240

maybe this is enough

lance6716 · 2022-09-07T11:29:01Z

br/pkg/lightning/mydump/parquet_parser.go

@@ -26,6 +27,8 @@ const (
 	// if a parquet if small than this threshold, parquet will load the whole file in a byte slice to
 	// optimize the read performance
 	smallParquetFileThreshold = 256 * 1024 * 1024
+	jan011970                 = 2440588


what's the meaning of 2440588

The decoding of the INT96 timestamp refers to a parquet library called parquet-go. Will paste some comments here tomorrow.

lance6716 · 2022-09-07T11:29:22Z

br/pkg/lightning/mydump/parquet_parser.go

+
+func jdToTime(jd int32, nsec int64) time.Time {
+	sec := int64(jd-jan011970) * secPerDay
+	return time.Unix(sec, int64(nsec))


Suggested change

return time.Unix(sec, int64(nsec))

return time.Unix(sec, nsec)

And can you add unit tests for nsec outside [0, 999,999,999]? (see the comment of time.Unix)

Will take a look that this later.

D3Hunter

rest lgtm

D3Hunter · 2022-09-08T02:55:48Z

br/pkg/lightning/mydump/parquet_parser.go

@@ -446,6 +449,13 @@ func setDatumByString(d *types.Datum, v string, meta *parquet.SchemaElement) {
 	if meta.LogicalType != nil && meta.LogicalType.DECIMAL != nil {
 		v = binaryToDecimalStr([]byte(v), int(meta.LogicalType.DECIMAL.Scale))
 	}
+	if meta.Type != nil && *meta.Type == parquet.Type_INT96 && len([]byte(v)) == 12 {


what's the value of LogicalType if enter this branch

br/pkg/lightning/mydump/parquet_parser.go

lance6716

rest lgtm

br/pkg/lightning/mydump/parquet_parser.go

lance6716 · 2022-09-08T03:55:16Z

br/pkg/lightning/mydump/parquet_parser.go

@@ -446,6 +451,11 @@ func setDatumByString(d *types.Datum, v string, meta *parquet.SchemaElement) {
 	if meta.LogicalType != nil && meta.LogicalType.DECIMAL != nil {
 		v = binaryToDecimalStr([]byte(v), int(meta.LogicalType.DECIMAL.Scale))
 	}
+	if meta.Type != nil && *meta.Type == parquet.Type_INT96 && len([]byte(v)) == 96/8 {


Suggested change

if meta.Type != nil && *meta.Type == parquet.Type_INT96 && len([]byte(v)) == 96/8 {

if meta.Type != nil && *meta.Type == parquet.Type_INT96 && len(v) == 96/8 {

lance6716 · 2022-09-08T03:55:52Z

br/pkg/lightning/mydump/parquet_parser.go

+	if meta.Type != nil && *meta.Type == parquet.Type_INT96 && len([]byte(v)) == 96/8 {
+		ts := int96ToTime([]byte(v))
+		ts = ts.UTC()
+		v = ts.Format("2006-01-02 15:04:05.999999Z")


this variable can be extracted as a constant

br/pkg/lightning/mydump/parquet_parser.go

Co-authored-by: lance6716 <lance6716@gmail.com>

buchuitoudegou · 2022-09-08T07:35:04Z

/run-integration-br-test

hawkingrei · 2022-09-09T05:11:09Z

/run-all-tests

hawkingrei · 2022-09-10T06:50:52Z

/run-mysql-test

lance6716 · 2022-09-13T02:02:06Z

/merge

ti-chi-bot · 2022-09-13T02:02:10Z

This pull request has been accepted and is ready to merge.

Commit hash: 63dfb26

sre-bot · 2022-09-13T02:49:47Z

TiDB MergeCI notify

✅ Well Done! New fixed [1] after this pr merged.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-common-test	🔴 failed 2, success 15, total 17	9 min 13 sec	Existing failure
idc-jenkins-ci-tidb/integration-ddl-test	✅ all 6 tests passed	22 min	Fixed
idc-jenkins-ci/integration-cdc-test	🟢 all 37 tests passed	29 min	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 11 tests passed	9 min 58 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	5 min 32 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	4 min 30 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	4 min 23 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 4 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	2 min 42 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

feat: support importing int96 ts

3b99213

ti-chi-bot added release-note-none do-not-merge/needs-triage-completed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 7, 2022

ti-chi-bot requested review from D3Hunter, dsdashun and lance6716 September 7, 2022 09:38

ti-chi-bot added the component/lightning This issue is related to Lightning of TiDB. label Sep 7, 2022

buchuitoudegou added 2 commits September 7, 2022 17:50

Merge branch 'master' of https://github.com/pingcap/tidb into parquet

babe0ce

fix: build failed

20d7ca2

lance6716 reviewed Sep 7, 2022

View reviewed changes

D3Hunter reviewed Sep 8, 2022

View reviewed changes

Merge branch 'master' into parquet

e41a858

ti-chi-bot removed the do-not-merge/needs-triage-completed label Sep 8, 2022

fix comment

2536ce5

lance6716 approved these changes Sep 8, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 8, 2022

buchuitoudegou and others added 4 commits September 8, 2022 12:01

Update br/pkg/lightning/mydump/parquet_parser.go

41aefd5

Co-authored-by: lance6716 <lance6716@gmail.com>

Update br/pkg/lightning/mydump/parquet_parser.go

d78b1ab

Co-authored-by: lance6716 <lance6716@gmail.com>

Update br/pkg/lightning/mydump/parquet_parser.go

cfefde7

Co-authored-by: lance6716 <lance6716@gmail.com>

extract constant layout

1f379a1

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 8, 2022

D3Hunter approved these changes Sep 8, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 8, 2022

Merge branch 'master' into parquet

63dfb26

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 13, 2022

ti-chi-bot merged commit 08cab41 into pingcap:master Sep 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lightning: support importing timestamp from Hive parquet #37685

lightning: support importing timestamp from Hive parquet #37685

buchuitoudegou commented Sep 7, 2022

ti-chi-bot commented Sep 7, 2022 •

edited

buchuitoudegou commented Sep 7, 2022

lance6716 Sep 7, 2022

D3Hunter Sep 8, 2022

buchuitoudegou Sep 8, 2022

buchuitoudegou Sep 8, 2022

lance6716 Sep 7, 2022

buchuitoudegou Sep 8, 2022

lance6716 Sep 7, 2022

lance6716 Sep 7, 2022 •

edited

buchuitoudegou Sep 8, 2022

lance6716 Sep 7, 2022

buchuitoudegou Sep 7, 2022

buchuitoudegou Sep 8, 2022

lance6716 Sep 7, 2022

buchuitoudegou Sep 7, 2022

buchuitoudegou Sep 8, 2022

D3Hunter left a comment

D3Hunter Sep 8, 2022

lance6716 left a comment

lance6716 Sep 8, 2022

buchuitoudegou Sep 8, 2022

lance6716 Sep 8, 2022

buchuitoudegou Sep 8, 2022

buchuitoudegou commented Sep 8, 2022

hawkingrei commented Sep 9, 2022

hawkingrei commented Sep 10, 2022

lance6716 commented Sep 13, 2022

ti-chi-bot commented Sep 13, 2022

sre-bot commented Sep 13, 2022

	if meta.Type != nil && *meta.Type == parquet.Type_INT96 && len([]byte(v)) == 12 {
	if meta.Type != nil && *meta.Type == parquet.Type_INT96 && len(v) == 12 {

	timeStr := formatTime(v, logicalType.TIMESTAMP.Unit, "2006-01-02 15:04:05.999999",
	"2006-01-02 15:04:05.999999Z", logicalType.TIMESTAMP.IsAdjustedToUTC)

	return time.Unix(sec, int64(nsec))
	return time.Unix(sec, nsec)

lightning: support importing timestamp from Hive parquet #37685

lightning: support importing timestamp from Hive parquet #37685

Conversation

buchuitoudegou commented Sep 7, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Sep 7, 2022 • edited

buchuitoudegou commented Sep 7, 2022

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

lance6716 Sep 7, 2022 • edited

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

D3Hunter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 left a 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

buchuitoudegou commented Sep 8, 2022

hawkingrei commented Sep 9, 2022

hawkingrei commented Sep 10, 2022

lance6716 commented Sep 13, 2022

ti-chi-bot commented Sep 13, 2022

sre-bot commented Sep 13, 2022

TiDB MergeCI notify

ti-chi-bot commented Sep 7, 2022 •

edited

lance6716 Sep 7, 2022 •

edited