executor: fix hash join between datetime and timestamp #25915

sylzd · 2021-07-04T09:37:27Z

What problem does this PR solve?

Issue Number: close #25902

Problem Summary:

What is changed and how it works?

Proposal: xxx

What's Changed:

How it Works:

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

Check List

Tests

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

Side effects

Performance regression
- Consumes more CPU
- Consumes more MEM
Breaking backward compatibility

Release note

No release note

ti-srebot · 2021-07-04T09:37:58Z

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

sylzd · 2021-07-04T10:28:15Z

Timestamp hash table cannot be encoded by utc zone, because it's not a persistent table, and used by hash join. Is that used by any other places?

sylzd · 2021-07-05T01:53:07Z

/cc @XuHuaiyu @morgo

XuHuaiyu · 2021-07-05T02:50:54Z

@wshwsh12 @tiancaiamao
May you help to review this bug-fix PR?

mmyj · 2021-07-05T03:49:31Z

@wshwsh12 @tiancaiamao
May you help to review this bug-fix PR?

I can help to review it.

tiancaiamao · 2021-07-05T03:50:13Z

@wshwsh12 @tiancaiamao
May you help to review this bug-fix PR?

I'll take a look.

tiancaiamao · 2021-07-05T03:54:00Z

util/codec/codec.go

@@ -330,14 +330,7 @@ func encodeHashChunkRowIdx(sc *stmtctx.StatementContext, row chunk.Row, tp *type
 	case mysql.TypeDate, mysql.TypeDatetime, mysql.TypeTimestamp:
 		flag = uintFlag
 		t := row.GetTime(idx)
-		// Encoding timestamp need to consider timezone.


I think here is right, the decoding process should take the timezone into consideration.

Since we do not store the timezone information in the TiKV, all timestamp data is stored as UTC.
They are converted to UTC first, and then into a uint64

The read back process should convert the uint64 to the correct timestamp considering the timezone

Timestamp hash table cannot be encoded by utc zone, because it's not a persistent table, and used by hash join. Is that used by any other places?

As I see, the hash table is not a persistent table, It's used to do join operator after being getted from kv and converted to the right timezone, other than store in kv.

For instance, this sql can produce the right result, because we do not convert timezone when do join. Timezone convertion should be considered when it was getted from kv.

select /*+ MERGE_JOIN(tt1, tt2) */ * from tt1 inner join tt2 on tt1.ts=tt2.ts;

tiancaiamao · 2021-07-05T09:27:08Z

Here is what we need to change:

tidb/util/codec/codec.go

Lines 333 to 340 in cb06e11

    
           // Encoding timestamp need to consider timezone. 
        
           // If it's not in UTC, transform to UTC first. 
        
           if t.Type() == mysql.TypeTimestamp && sc.TimeZone != time.UTC { 
        
           	err = t.ConvertTimeZone(sc.TimeZone, time.UTC) 
        
           	if err != nil { 
        
           		return 
        
           	} 
        
           }

tidb/util/codec/codec.go

Lines 510 to 517 in cb06e11

    
           // Encoding timestamp need to consider timezone. 
        
           // If it's not in UTC, transform to UTC first. 
        
           if t.Type() == mysql.TypeTimestamp && sc.TimeZone != time.UTC { 
        
           	err = t.ConvertTimeZone(sc.TimeZone, time.UTC) 
        
           	if err != nil { 
        
           		return 
        
           	} 
        
           }

Could you help us remove it in this pull request, and also add a test like that:

mysql> select * from tt1 where ts in (select ts from tt2);
+---------------------+
| ts                  |
+---------------------+
| 2001-01-01 00:00:00 |
+---------------------+
1 row in set (0.00 sec)

mysql> set @@session.time_zone = '+10:00';
Query OK, 0 rows affected (0.00 sec)

mysql> select * from tt1 where ts in (select ts from tt2);
Empty set (0.01 sec)

Bug fix should comes with a unit test to prevent regression. @sylzd

tiancaiamao · 2021-07-05T09:38:41Z

Explain: tikv doesn't store the time_zone information, so the timestamp is converted to UTC first before stored.
When the timestamp data is loaded into chunk, they are convert back considering the session time_zone.

mysql> insert into tt1 values ("2001-01-01 00:00:00");
Query OK, 1 row affected (0.00 sec)
# stored as `2000-12-31 16:00:00` in UTC

mysql> select * from tt1;
+---------------------+
| ts                  |
+---------------------+
| 2001-01-01 00:00:00 |
+---------------------+
1 row in set (0.01 sec)
# read back `2000-12-31 16:00:00` considering session time_zone=+8:00 gets `2001-01-01 00:00:00` UTC+8

mysql> set @@session.time_zone = '+10:00';
Query OK, 0 rows affected (0.00 sec)

mysql> select * from tt1;
+---------------------+
| ts                  |
+---------------------+
| 2001-01-01 02:00:00 |
+---------------------+
1 row in set (0.00 sec)
# read back `2000-12-31 16:00:00` considering session time_zone=+8:00 gets `2001-01-01 02:00:00` UTC+10

The bug of that issue is, the loading to chunk process had already handle the time_zone converting, so when hash join key comparing, the converting should not be done again.

tidb/executor/hash_table.go

Line 127 in cb06e11

    
           func (c *hashRowContainer) matchJoinKey(buildRow, probeRow chunk.Row, probeHCtx *hashContext) (ok bool, err error) {

sylzd · 2021-07-05T10:00:18Z

Explain: tikv doesn't store the time_zone information, so the timestamp is converted to UTC first before stored.
When the timestamp data is loaded into chunk, they are convert back considering the session time_zone.
mysql> insert into tt1 values ("2001-01-01 00:00:00");
Query OK, 1 row affected (0.00 sec)
# stored as `2000-12-31 16:00:00` in UTC

mysql> select * from tt1;
+---------------------+
| ts                  |
+---------------------+
| 2001-01-01 00:00:00 |
+---------------------+
1 row in set (0.01 sec)
# read back `2000-12-31 16:00:00` considering session time_zone=+8:00 gets `2001-01-01 00:00:00` UTC+8

mysql> set @@session.time_zone = '+10:00';
Query OK, 0 rows affected (0.00 sec)

mysql> select * from tt1;
+---------------------+
| ts                  |
+---------------------+
| 2001-01-01 02:00:00 |
+---------------------+
1 row in set (0.00 sec)
# read back `2000-12-31 16:00:00` considering session time_zone=+8:00 gets `2001-01-01 02:00:00` UTC+10
The bug of that issue is, the loading to chunk process had already handle the time_zone converting, so when hash join key comparing, the converting should not be done again.

tidb/executor/hash_table.go

Line 127 in cb06e11

func (c *hashRowContainer) matchJoinKey(buildRow, probeRow chunk.Row, probeHCtx *hashContext) (ok bool, err error) {

yes, that's what I mean~

sylzd · 2021-07-05T10:14:33Z

@mmyj PTAL~

mmyj · 2021-07-05T10:59:35Z

executor/join_test.go

+
+func (s *testSuiteJoinSerial) TestIssue25902(c *C) {
+	tk := testkit.NewTestKitWithInit(c, s.store)
+	tk.MustExec("drop table if exists tt1,tt2; ")


Missing tt3

mmyj · 2021-07-05T11:00:25Z

executor/join_test.go

+	tk.MustExec("insert into tt3 values (\"2001-01-01 00:00:00\");")
+	tk.MustQuery("select * from tt1 where ts in (select ts from tt2);").Check(testkit.Rows("2001-01-01 00:00:00"))
+	tk.MustQuery("select * from tt1 where ts in (select ts from tt3);").Check(testkit.Rows("2001-01-01 00:00:00"))
+	tk.MustExec("set @@session.time_zone = '+10:00';")


Should we reset to time_zone to +08?

thx, it fixed~

mmyj

lgtm

ti-chi-bot · 2021-07-05T11:38:12Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

mmyj
tiancaiamao

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.

tiancaiamao · 2021-07-06T03:11:25Z

/merge

ti-chi-bot · 2021-07-06T03:11:28Z

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

Commit hash: 66fa1bd

guo-shaoge · 2021-07-06T06:43:27Z

/label needs-cherry-pick-4.0
/label needs-cherry-pick-5.0
/label needs-cherry-pick-5.1

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2021-07-06T06:44:28Z

cherry pick to release-4.0 in PR #25988

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2021-07-06T06:45:52Z

cherry pick to release-5.0 in PR #25990

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2021-07-06T06:46:47Z

cherry pick to release-5.1 in PR #25991

fix 25902

30b99e9

ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 4, 2021

github-actions bot added the sig/execution SIG execution label Jul 4, 2021

fix annotation

0fd8cfd

ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 4, 2021

add test case of datetime type

1c48aef

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 4, 2021

ti-chi-bot requested review from morgo and XuHuaiyu July 5, 2021 01:53

XuHuaiyu requested review from wshwsh12 and tiancaiamao and removed request for morgo and XuHuaiyu July 5, 2021 02:49

tiancaiamao reviewed Jul 5, 2021

View reviewed changes

add test case of other timezone

154565f

tiancaiamao approved these changes Jul 5, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 5, 2021

mmyj reviewed Jul 5, 2021

View reviewed changes

sylzd added 2 commits July 5, 2021 19:24

fix UT

ecfbd3f

fix UT session variable

66fa1bd

mmyj approved these changes Jul 5, 2021

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 Jul 5, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 6, 2021

Merge branch 'master' into fix_25902

dc4fc62

ti-chi-bot merged commit dc53b52 into pingcap:master Jul 6, 2021

ti-chi-bot added needs-cherry-pick-4.0 needs-cherry-pick-release-5.0 needs-cherry-pick-release-5.1 labels Jul 6, 2021

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jul 6, 2021

cherry pick pingcap#25915 to release-4.0

8ac0c8b

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Jul 6, 2021

executor: fix hash join between datetime and timestamp (#25915) #25988

Merged

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jul 6, 2021

cherry pick pingcap#25915 to release-5.0

8e49d3b

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Jul 6, 2021

executor: fix hash join between datetime and timestamp (#25915) #25990

Merged

ti-srebot mentioned this pull request Jul 6, 2021

executor: fix hash join between datetime and timestamp (#25915) #25991

Merged

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jul 6, 2021

cherry pick pingcap#25915 to release-5.1

e3fd0af

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

XuHuaiyu added the type/bugfix This PR fixes a bug. label Jul 7, 2021

ti-chi-bot pushed a commit that referenced this pull request Jul 16, 2021

executor: fix hash join between datetime and timestamp (#25915) (#25988)

e4a7573

ti-chi-bot pushed a commit that referenced this pull request Jul 21, 2021

executor: fix hash join between datetime and timestamp (#25915) (#25991)

2a93597

ti-chi-bot pushed a commit that referenced this pull request Aug 13, 2021

executor: fix hash join between datetime and timestamp (#25915) (#25990)

9355681

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: fix hash join between datetime and timestamp #25915

executor: fix hash join between datetime and timestamp #25915

sylzd commented Jul 4, 2021 •

edited

Loading

ti-srebot commented Jul 4, 2021

sylzd commented Jul 4, 2021 •

edited

Loading

sylzd commented Jul 5, 2021

XuHuaiyu commented Jul 5, 2021

mmyj commented Jul 5, 2021

tiancaiamao commented Jul 5, 2021

tiancaiamao Jul 5, 2021

tiancaiamao Jul 5, 2021

sylzd Jul 5, 2021

sylzd Jul 5, 2021 •

edited

Loading

tiancaiamao Jul 5, 2021

tiancaiamao commented Jul 5, 2021

tiancaiamao commented Jul 5, 2021

sylzd commented Jul 5, 2021

sylzd commented Jul 5, 2021

mmyj Jul 5, 2021

sylzd Jul 5, 2021

mmyj Jul 5, 2021

sylzd Jul 5, 2021

mmyj left a comment

ti-chi-bot commented Jul 5, 2021

tiancaiamao commented Jul 6, 2021

ti-chi-bot commented Jul 6, 2021

guo-shaoge commented Jul 6, 2021

ti-srebot commented Jul 6, 2021

ti-srebot commented Jul 6, 2021

ti-srebot commented Jul 6, 2021

executor: fix hash join between datetime and timestamp #25915

executor: fix hash join between datetime and timestamp #25915

Conversation

sylzd commented Jul 4, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

ti-srebot commented Jul 4, 2021

sylzd commented Jul 4, 2021 • edited Loading

sylzd commented Jul 5, 2021

XuHuaiyu commented Jul 5, 2021

mmyj commented Jul 5, 2021

tiancaiamao commented Jul 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylzd Jul 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao commented Jul 5, 2021

tiancaiamao commented Jul 5, 2021

sylzd commented Jul 5, 2021

sylzd commented Jul 5, 2021

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

mmyj left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Jul 5, 2021

tiancaiamao commented Jul 6, 2021

ti-chi-bot commented Jul 6, 2021

guo-shaoge commented Jul 6, 2021

ti-srebot commented Jul 6, 2021

ti-srebot commented Jul 6, 2021

ti-srebot commented Jul 6, 2021

sylzd commented Jul 4, 2021 •

edited

Loading

sylzd commented Jul 4, 2021 •

edited

Loading

sylzd Jul 5, 2021 •

edited

Loading