Fix cast int as time by leiysky · Pull Request #1788 · pingcap/tiflash

leiysky · 2021-04-16T08:33:52Z

Signed-off-by: Yu Lei leiysky@outlook.com

What problem does this PR solve?

Issue Number: close #1764

Problem Summary:

What is changed and how it works?

Proposal: xxx

What's Changed:

Related changes

Need to cherry-pick to the release branch:

Check List

Tests

Unit test
Integration test

Release note

Fix the issue that cast int as time function in TiFlash's coprocessor may produce wrong result

leiysky · 2021-04-16T08:52:23Z

/run-all-tests

leiysky · 2021-04-16T12:35:38Z

/run-all-tests

leiysky · 2021-04-18T05:32:06Z

/run-all-tests

dbms/src/Common/MyTime.cpp

leiysky · 2021-04-19T11:37:59Z

/run-all-tests

Signed-off-by: Yu Lei <leiysky@outlook.com>

Signed-off-by: leiysky <leiysky@outlook.com>

leiysky · 2021-04-26T12:48:03Z

@windtalker PTAL, again.

leiysky · 2021-04-27T03:37:58Z

/run-all-tests

windtalker · 2021-04-27T08:18:36Z

dbms/src/Common/MyTime.cpp

    {
-        number = (number + 200000000) * 1000000;
-        return get_datetime(number);
+        if (!allow_zero_in_date && (month == 0 || day == 0))


why year == 0 is allowed?

It looks weird but MySQL does allow this.

Reference from https://dev.mysql.com/doc/refman/5.7/en/sql-mode.html#sqlmode_no_zero_in_date:

The NO_ZERO_IN_DATE mode affects whether the server permits dates in which the year part is nonzero but the month or day part is 0.

windtalker · 2021-04-27T08:24:47Z

dbms/src/Common/MyTime.cpp

-    // check YYYYMMDD
-    if (number <= 10000101)
+    UInt8 max_day = 31;
+    if (!allow_invalid_date)


So day should never be bigger than 31 even if allow_invalid_date is true?

windtalker · 2021-04-27T08:27:55Z

dbms/src/Common/MyTime.cpp


-    // check MMDDhhmmss
-    if (number < 101000000)
+    if (hour < 0 || hour >= 24)


Theoretically speaking, for a MyDate value, hour,minute,second is meaningless, they can be any value, we should not throw Incorrect datetime value for a MyDate if its hour is invalid.

Yes, but MySQL doesn't really distinguish them in this case.

windtalker · 2021-04-27T08:30:10Z

dbms/src/Common/MyTime.h

 bool isValidSeperator(char c, int previous_parts);

+// Build MyDateTime value with checking overflow of internal fields, return true if input is invalid.
+bool checkedDateTime(const UInt64 & year, const UInt64 & month, const UInt64 & day, const UInt64 & hour, const UInt64 & minute,


The function name is misleading, better to use a more appropriate name.

windtalker · 2021-04-27T08:32:56Z

Looks like the update pr still does not answer my previous question:
what about zero date with non-zero hour/minute/second/microsecond, should it be treated as a valid datetime?

Signed-off-by: leiysky <leiysky@outlook.com>

leiysky · 2021-04-28T06:52:39Z

Looks like the update pr still does not answer my previous question:
what about zero date with non-zero hour/minute/second/microsecond, should it be treated as a valid datetime?

Yes, there's no extra rule to constraint time part in datetime.

windtalker · 2021-04-30T01:17:53Z

dbms/src/Functions/FunctionsTiDBConversion.h

+    UInt64 minute = hms / 100;
+    UInt64 second = hms % 100;
+
+    if (fromDateTimeChecked(year, month, day, hour, minute, second, 0, result))


This part is still hard to understand: first after calling fromDateTimeChecked, I will expected to get a valid datetime, and may be confused why result.check need to be called again?

This function only checks whether the year, month, day, hour, minute, second value can fit CoreTime fields or not.

leiysky · 2021-05-10T12:15:22Z

@windtalker PTAL

windtalker

LGTM except that fromDateTimeChecked is a little confusing because even if this function returns true, the datetime could still be invalid

leiysky · 2021-05-11T04:18:10Z

/merge

ti-srebot · 2021-05-11T04:18:22Z

/run-all-tests

ti-srebot · 2021-05-11T05:14:53Z

@leiysky merge failed.

leiysky · 2021-05-11T06:35:50Z

/merge

ti-srebot · 2021-05-11T06:36:22Z

/run-all-tests

ti-srebot · 2021-05-11T07:16:53Z

@leiysky merge failed.

leiysky · 2021-05-11T08:34:32Z

/merge

ti-srebot · 2021-05-11T08:34:52Z

/run-all-tests

ti-srebot · 2021-05-11T09:25:53Z

@leiysky merge failed.

leiysky · 2021-05-12T05:53:09Z

/merge

ti-srebot · 2021-05-12T05:58:35Z

/run-all-tests

ti-srebot · 2021-05-12T07:04:19Z

cherry pick to release-4.0 in PR #1892

ti-srebot · 2021-05-12T07:06:13Z

cherry pick to release-5.0 in PR #1893

Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Co-authored-by: Flowyi <flowbehappy@gmail.com> cherry pick pingcap#1821 to release-5.0 (pingcap#1827) Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Co-authored-by: xufei <xufei@pingcap.com> Remove ReplicatedMergeTree family (pingcap#1805) (pingcap#1826) cherry pick pingcap#1835 to release-5.0 (pingcap#1840) Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Co-authored-by: xufei <xufei@pingcap.com> Use file path as cache key (pingcap#1852) (pingcap#1862) Lazily initializing DeltaMergeStore (pingcap#1423) (pingcap#1751) (pingcap#1868) Add background gc check thread for DeltaTree storage (pingcap#1742) (pingcap#1828) * cherry pick pingcap#1742 to release-5.0 Signed-off-by: ti-srebot <ti-srebot@pingcap.com> * fix conflict Co-authored-by: lidezhu <47731263+lidezhu@users.noreply.github.com> Fix calculate stable property (pingcap#1839) (pingcap#1878) * cherry pick pingcap#1839 to release-5.0 Signed-off-by: ti-srebot <ti-srebot@pingcap.com> * fix compile and cherry pick a small fix Co-authored-by: lidezhu <47731263+lidezhu@users.noreply.github.com> Fix client-c mistake setting current_ts to 0 when resolving lock for async commit (pingcap#1856) (pingcap#1870) support constant folding in dbg coprocessor (pingcap#1881) (pingcap#1884) Optimize & Reduce time cost about ci test (pingcap#1849) (pingcap#1894) * reduce time cost about ci test by parallel * add `-DNO_WERROR=ON` to cmake config for release-darwin build * Fix tidb_ghpr_tics_test fail (pingcap#1895) Signed-off-by: Zhigao Tong <tongzhigao@pingcap.com> Fix panic with feature `compaction filter` in release-5.0 (pingcap#1891) Init store in background task. (pingcap#1843,pingcap#1896) (pingcap#1874) Push down filter on timestamp type column to storage layer (pingcap#1875) (pingcap#1906) * cherry pick pingcap#1875 to release-5.0 Signed-off-by: ti-srebot <ti-srebot@pingcap.com> * remove irrevalant code Co-authored-by: lidezhu <47731263+lidezhu@users.noreply.github.com> Fix ExchangeSender: remove duplicated write stream operation (pingcap#1379) (pingcap#1883) cherry pick pingcap#1917 to release-5.0 (pingcap#1923) Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Co-authored-by: Fu Zhe <fuzhe1989@gmail.com> Pre-handle SSTFiles to DTFiles when applying snapshots (pingcap#1439) (pingcap#1867) * Add `raft.snapshot.method` in configuration file to control the method of applying snapshots - "block" decode snapshot data as a block - "file1" decode snapshot data as DTFiles (directory mode) [**By default**] * Add a stream `SSTFilesToBlockInputStream` to decode snapshot data into blocks * Add a stream `BoundedSSTFilesToBlockInputStream` to bound the blocks read from SSTFilesToBlockInputStream by column `_tidb_rowid` * Add a stream `SSTFilesToDTFilesOutputStream` that accept `BoundedSSTFilesToBlockInputStream` as input stream to write blocks into DTFiles * Make `STORAGE_FORMAT_CURRENT` to be `STORAGE_FORMAT_V2` by default to support ingest DTFile into DT engine * Fix the bug that the block decoded from SSTFile is not sorted by primary key and version (pingcap#1888) * Fix panic with feature compaction filter with DTFile * Fix ingest write amplification after snapshot apply optimization (pingcap#1913) Co-authored-by: Zhigao Tong <tongzhigao@pingcap.com> Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com> Cast int/real as real (pingcap#1911) (pingcap#1928) Fix the problem that old dmfile is not removed atomically (pingcap#1918) (pingcap#1925) Eliminate the table lock between reading,writing and DDL operators for TiFlash (pingcap#1736) (pingcap#1858) * Eliminate the table lock between reading,writing and DDL operators for TiFlash (pingcap#1736) 1. Port the latest `RWLock` for `IStorage` from Clickhouse. "phase-fair rwlocks have been shown to provide more predictable timings compared to task-fair rwlocks for certain workloads (for relatively low share of writes, <25%). Brandenburg B, Anderson J, (2010)" 2. Refactor the lock model for `IStorage` to eliminate the lock between reading, writing, and DDL operations. 3. Acquire table lock by QueryID/threadName instead of function name 4. Tombstone the database after receiving a "drop" DDL from TiDB. Physically drop the databases / tables after gc safe point. 5. Remove some outdated tests under `tests/mutable-test/txn_schema` (all those tests are ported into `tests/delta-merge-test/raft/schema`) * Mark FIXME for conflict codes Conflicts: dbms/src/Flash/Coprocessor/DAGQueryBlockInterpreter.cpp Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com> cached tics code incase of clone same code repeatedly (pingcap#1994) (pingcap#2001) Add row & bytes threshold config for CompactLog (pingcap#1997) (pingcap#2005) * cherry pick pingcap#1997 to release-5.0 Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Co-authored-by: Zhigao Tong <tongzhigao@pingcap.com> Fix the bug that incomplete write batches are not truncated (pingcap#1934) (pingcap#2003) Revert "Push down filter on timestamp type column to storage layer (pingcap#1875) (pingcap#1906)" (pingcap#2010) This reverts commit 630b612. Revert Lazily Init Store (pingcap#2011) * Revert "Init store in background task. (pingcap#1843,pingcap#1896) (pingcap#1874)" This reverts commit 0882461. Conflicts: dbms/src/Storages/StorageDeltaMerge.cpp * format code * Revert "Lazily initializing DeltaMergeStore (pingcap#1423) (pingcap#1751) (pingcap#1868)" This reverts commit bbce050. Conflicts: dbms/src/Storages/DeltaMerge/tests/gtest_dm_storage_delta_merge.cpp dbms/src/Storages/StorageDeltaMerge.cpp dbms/src/Storages/StorageDeltaMerge.h Co-authored-by: JinheLin <linjinhe@pingcap.com> Co-authored-by: Flowyi <flowbehappy@gmail.com> Revert "background gc thread" (pingcap#2015) * Revert "Fix calculate stable property (pingcap#1839) (pingcap#1878)" This reverts commit 6a9eb58. * Revert "Add background gc check thread for DeltaTree storage (pingcap#1742) (pingcap#1828)" This reverts commit cbbbd09. * remove code * format code * format code * fix test compile * fix comment Fix the potential concurrency problem when clone the shared delta index (pingcap#2030) (pingcap#2033) * cherry pick pingcap#2030 to release-5.0 Signed-off-by: ti-srebot <ti-srebot@pingcap.com> * Update dbms/src/Storages/DeltaMerge/DeltaIndex.h Co-authored-by: JaySon <jayson.hjs@gmail.com> Co-authored-by: lidezhu <47731263+lidezhu@users.noreply.github.com> Co-authored-by: JaySon <jayson.hjs@gmail.com> Fix bug for unexpected deleted ingest file (pingcap#2047) (pingcap#2048) * Only enable GC for DTFiles after they get applied to all segments. * Refine some loggings Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com> Fix cast int as time (pingcap#1788) (pingcap#1893) cherry pick pingcap#1903 to release-5.0 (pingcap#1915) Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Fix ingesting file may add ref page to deleted page (pingcap#2054) (pingcap#2055) 1. Put the ingest file id into `storage_pool.data` before ingesting them into segments 2. Add ref pages to the ingest files for each segment 3. Delete the original page id after all Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com> Fix compile error on Mac Clang 12.0.5 (pingcap#2058) (pingcap#2070) cherry pick pingcap#2017 pingcap#2020 pingcap#2051 to release-5.0 (pingcap#2074) Revert "cherry pick pingcap#2017 pingcap#2020 pingcap#2051 to release-5.0 (pingcap#2074)" This reverts commit 8e0712c. Revert "Fix compile error on Mac Clang 12.0.5 (pingcap#2058) (pingcap#2070)" This reverts commit 3c31b92. Revert "Fix ingesting file may add ref page to deleted page (pingcap#2054) (pingcap#2055)" This reverts commit c773d61. Revert "Fix bug for unexpected deleted ingest file (pingcap#2047) (pingcap#2048)" This reverts commit b329618. Revert "Pre-handle SSTFiles to DTFiles when applying snapshots (pingcap#1439) (pingcap#1867)" This reverts commit c947fd6. Conflicts: dbms/src/Common/FailPoint.cpp dbms/src/Storages/DeltaMerge/File/DMFileBlockOutputStream.h dbms/src/Storages/DeltaMerge/File/DMFileWriter.h dbms/src/Storages/DeltaMerge/ReorganizeBlockInputStream.h dbms/src/Storages/DeltaMerge/SSTFilesToBlockInputStream.cpp dbms/src/Storages/DeltaMerge/SSTFilesToBlockInputStream.h dbms/src/Storages/DeltaMerge/SSTFilesToDTFilesOutputStream.cpp dbms/src/Storages/DeltaMerge/Segment.cpp dbms/src/Storages/DeltaMerge/StableValueSpace.cpp dbms/src/Storages/DeltaMerge/tests/gtest_dm_file.cpp dbms/src/Storages/DeltaMerge/tests/gtest_dm_storage_delta_merge.cpp dbms/src/Storages/StorageDeltaMerge.cpp dbms/src/Storages/StorageDeltaMerge.h dbms/src/Storages/Transaction/ApplySnapshot.cpp dbms/src/Storages/Transaction/PartitionStreams.cpp tests/delta-merge-test/raft/schema/drop_on_restart.test Fix comment Signed-off-by: JaySon-Huang <jayson.hjs@gmail.com>

leiysky added needs-cherry-pick-release-4.0 PR which needs to be cherry-picked to release-4.0 type/bugfix This PR fixes a bug. needs-cherry-pick-release-5.0 PR which needs to be cherry-picked to release-5.0 labels Apr 16, 2021

leiysky requested a review from windtalker April 16, 2021 08:33

windtalker reviewed Apr 19, 2021

View reviewed changes

dbms/src/Common/MyTime.cpp Outdated Show resolved Hide resolved

leiysky added 6 commits April 26, 2021 16:35

fix numberToDateTime

ce251ca

Signed-off-by: Yu Lei <leiysky@outlook.com>

save

c888585

fix

f43e0e1

Signed-off-by: Yu Lei <leiysky@outlook.com>

refine

8364c32

Signed-off-by: leiysky <leiysky@outlook.com>

refine

02aab1a

Signed-off-by: leiysky <leiysky@outlook.com>

refine

8528e37

Signed-off-by: leiysky <leiysky@outlook.com>

windtalker reviewed Apr 27, 2021

View reviewed changes

rename

9da3d32

Signed-off-by: leiysky <leiysky@outlook.com>

windtalker reviewed Apr 30, 2021

View reviewed changes

leiysky added 2 commits May 10, 2021 14:38

Merge branch 'master' into fix/cast-int-as-time

44eeec8

Merge branch 'master' into fix/cast-int-as-time

a4db0eb

windtalker approved these changes May 11, 2021

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label May 11, 2021

leiysky added 2 commits May 11, 2021 12:11

fix name

0e88aa1

Merge branch 'master' into fix/cast-int-as-time

6ff0fe5

ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label May 11, 2021

Merge branch 'master' into fix/cast-int-as-time

361d1b5

ti-srebot merged commit c1e7782 into pingcap:master May 12, 2021

ti-srebot mentioned this pull request May 12, 2021

Fix cast int as time (#1788) #1892

Merged

leiysky deleted the fix/cast-int-as-time branch May 12, 2021 07:05

ti-srebot mentioned this pull request May 12, 2021

Fix cast int as time (#1788) #1893

Merged

ti-srebot added a commit that referenced this pull request Jun 1, 2021

Fix cast int as time (#1788) (#1893)

c72ea70

JaySon-Huang mentioned this pull request Jun 8, 2021

Check the diff between revert_snapshot and pick need commit JaySon-Huang/tiflash#8

Merged

ti-srebot added a commit that referenced this pull request Jul 7, 2021

Fix cast int as time (#1788) (#1892)

3e1dd3c

Conversation

leiysky commented Apr 16, 2021

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

Uh oh!

leiysky commented Apr 16, 2021

Uh oh!

leiysky commented Apr 16, 2021

Uh oh!

leiysky commented Apr 18, 2021

Uh oh!

Uh oh!

leiysky commented Apr 19, 2021

Uh oh!

leiysky commented Apr 26, 2021

Uh oh!

leiysky commented Apr 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

windtalker commented Apr 27, 2021

Uh oh!

leiysky commented Apr 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leiysky commented May 10, 2021

Uh oh!

windtalker left a comment

Choose a reason for hiding this comment

Uh oh!

leiysky commented May 11, 2021

Uh oh!

ti-srebot commented May 11, 2021

Uh oh!

ti-srebot commented May 11, 2021

Uh oh!

leiysky commented May 11, 2021

Uh oh!

ti-srebot commented May 11, 2021

Uh oh!

ti-srebot commented May 11, 2021

Uh oh!

leiysky commented May 11, 2021

Uh oh!

ti-srebot commented May 11, 2021

Uh oh!

ti-srebot commented May 11, 2021

Uh oh!

leiysky commented May 12, 2021

Uh oh!

ti-srebot commented May 12, 2021

Uh oh!

ti-srebot commented May 12, 2021

Uh oh!

ti-srebot commented May 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects