Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

executor: fix hash join between datetime and timestamp #25915

Merged
merged 7 commits into from
Jul 6, 2021
Merged

executor: fix hash join between datetime and timestamp #25915

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
30b99e9
fix 25902
sylzd Jul 4, 2021
0fd8cfd
fix annotation
sylzd Jul 4, 2021
1c48aef
add test case of datetime type
sylzd Jul 4, 2021
154565f
add test case of other timezone
sylzd Jul 5, 2021
ecfbd3f
fix UT
sylzd Jul 5, 2021
66fa1bd
fix UT session variable
sylzd Jul 5, 2021
dc4fc62
Merge branch 'master' into fix_25902
ti-chi-bot Jul 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions executor/join_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2592,3 +2592,18 @@ func (s *testSuiteJoinSerial) TestIssue20219(c *C) {
tk.MustQuery("select /*+ inl_join(s)*/ t.a from t left join s on t.a = s.a;").Check(testkit.Rows("i", "j"))
tk.MustQuery("show warnings").Check(testkit.Rows())
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing tt3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

tk.MustExec("create table tt1 (ts timestamp);")
tk.MustExec("create table tt2 (ts varchar(32));")
tk.MustExec("create table tt3 (ts datetime);")
tk.MustExec("insert into tt1 values (\"2001-01-01 00:00:00\");")
tk.MustExec("insert into tt2 values (\"2001-01-01 00:00:00\");")
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';")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we reset to time_zone to +08?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx, it fixed~

tk.MustQuery("select * from tt1 where ts in (select ts from tt2);").Check(testkit.Rows())
}
18 changes: 2 additions & 16 deletions util/codec/codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sylzd sylzd Jul 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly

// 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
}
}

var v uint64
v, err = t.ToPackedUint()
if err != nil {
Expand Down Expand Up @@ -507,14 +500,7 @@ func HashChunkSelected(sc *stmtctx.StatementContext, h []hash.Hash64, chk *chunk
isNull[i] = !ignoreNull
} else {
buf[0] = uintFlag
// 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
}
}

var v uint64
v, err = t.ToPackedUint()
if err != nil {
Expand Down