fix: align sec_to_time and time assignment boundaries - #27780
fix: align sec_to_time and time assignment boundaries#27780iamlinjunhong wants to merge 8 commits into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
aptend
left a comment
There was a problem hiding this comment.
Blocking issue found: the function-output boundary and the TIME-column assignment boundary are different in MySQL. The current shared constant makes strict DML accept out-of-range column values and makes non-strict DML preserve them instead of clamping. I reproduced this against MySQL 9.6 and with an exact-head counterexample test.
| // MySQLTimeMax is the largest value accepted by a MySQL TIME column. | ||
| // MatrixOne's Time representation deliberately has a wider internal range | ||
| // because intermediate duration expressions can exceed 838 hours. | ||
| MySQLTimeMax = Time((838*SecsPerHour+59*SecsPerMinute+59)*MicroSecsPerSec + MicroSecsPerSec - 1) |
There was a problem hiding this comment.
This is not the largest value accepted by a MySQL TIME column. MySQL allows SEC_TO_TIME(3020399.999999) to produce 838:59:59.999999, but column assignment has a 838:59:59.000000 endpoint. Repro: SET sql_mode="STRICT_TRANS_TABLES"; CREATE TABLE t(v TIME(6)); INSERT INTO t VALUES ("838:59:59.000001"); returns error 1292 in MySQL 9.6; with non-strict mode it warns and stores 838:59:59.000000. At this head, NewAssignCast accepts the strict insert because mysqlTimeForCast uses this constant, and non-strict assignment retains the extra microseconds. Please separate the SEC_TO_TIME result range from the column-assignment range and update the assignment/BVT expectations accordingly.
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed exact head 8cb8494e1f4a3741560ac8a6c65d51e0773bb560 against merge base 9c9223ba7feec745bd23efba656bcfdfaca947fd.
One blocking MySQL-compatibility issue remains. types.MySQLTimeMax is defined as 838:59:59.999999 and is used both by SEC_TO_TIME result clamping and by mysqlTimeForCast at the TIME-column assignment boundary. Those boundaries differ: SEC_TO_TIME may return the fractional endpoint, while a TIME column accepts only 838:59:59.000000. Consequently strict DML accepts values such as 838:59:59.000001, and non-strict/IGNORE assignment preserves excess microseconds instead of warning and clamping to .000000.
Separate the function-result maximum from the assignment maximum, route strict/non-strict/IGNORE assignment through the latter, and add positive/negative plus scale controls at both boundaries. Exact-head CI is green, but its current expectations encode the shared-boundary regression.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed exact head 2157b36 against base 02af906.
Requesting changes for two assignment-boundary correctness gaps:
[P1] Unsigned TIME assignment can silently corrupt the value. integerToTime converts the generic input to int64 before validating its range. For a uint64 source, math.MaxUint64 therefore becomes -1, passes the existing bounds and parser, and strict assignment stores -00:00:01 instead of rejecting the out-of-range value. I reproduced this with an exact-head counterexample through NewAssignCast under STRICT_TRANS_TABLES. Validate unsigned values before narrowing, and cover strict, non-strict, and INSERT IGNORE behavior around MaxInt64 and MaxUint64.
[P2] Non-strict and IGNORE clamping is silent. mysqlTimeForCast returns ClampMySQLTimeForScale without appending a warning. MySQL 8.4 emits warning 1264 for both 839:00:00 and 838:59:59.000001 while storing 838:59:59.000000; the exact-head counterexample clamps but leaves the session warning list empty. Preserve the warning so SHOW WARNINGS and clients can detect lossy assignment, and assert it in tests.
The previous shared function-result versus column-assignment boundary blocker is fixed, and the added PR tests plus planner tests pass locally. However they miss both counterexamples above. The BVT golden for SEC_TO_TIME(-2378) also currently records positive 00:39:38 despite the SQL comment and MySQL oracle expecting -00:39:38; please correct or isolate that harness/serialization gap so the negative sub-hour case is actually protected.
Performance and unhappy-path audit: the new work is bounded per row, introduces no wait or ownership graph, and I found no hang, leak, OOM, or log-storm blocker. The unconditional same-type TIME assignment cast adds a small linear validation pass, which is justified by the wider internal representation.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review completed at exact head 7cdaf3c1beaf71a8deb97b997db7cee93f8ef19f.
The two prior mechanisms are materially improved: malformed/partial varchar inputs now retain a separate DECIMAL warning alongside TIME-range warnings, warning values are bounded to 128 characters before reaching either sink, and the remote path preserves the complete count while retaining a fixed diagnostic payload. The previous assignment-boundary, unsigned narrowing, warning-code/row-index, and remote-collector fixes remain patch-equivalent.
One correctness blocker remains:
[P1] Do not use the bounded TIME calculation threshold as the DECIMAL underflow diagnostic threshold. At func_binary.go:8844-8862, exponentLimit := totalDigits + 14 exists to avoid constructing an exponent larger than SEC_TO_TIME needs. When a negative exponent exceeds that small computational bound, the code returns conversionTruncated=true. For '1e-16', totalDigits=1, so exponent 16 already crosses limit 15 and this head emits warning 1292 even though the numeric input is valid and merely rounds to zero microseconds.
I verified the public oracle with MySQL 8.4.10: SEC_TO_TIME('1e-16'), '1e-30', and '1e-81' all return 00:00:00.000000 with zero warnings; '1e-82' is the first adjacent underflow case and emits one 1292 DECIMAL warning. The current test jumps directly to 1e-999999999, so it locks in the extreme case while missing the false-positive range. The symmetric conversion-overflow side is also hidden: MySQL 8.4.10 emits conversion diagnostics in addition to the TIME warning for 1e81, whereas this branch returns only truncated=true with conversionTruncated=false.
Please separate the bounded TIME arithmetic state from the actual DECIMAL conversion status, and add no-warning controls around representable tiny values plus underflow/overflow boundary controls (at least 1e-81 / 1e-82 and the positive boundary). The long-value/many-row remote test should remain; its count and payload bounds are sound.
Exact-head CI is terminal green, git diff --check passes, and I found no additional allocation, retention, lifecycle, hang, or normal-path blocker.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep-reviewed exact head 131f95a426101621274dc2820707547ee0687091 against base 41f5649904db84dc1fcbe0c654c61b68d70f9e4d. The previous 1e-81 / 1e-82 boundary is fixed, but two public SQL compatibility blockers remain.
-
[P1] Preserve pre-exponent DECIMAL mantissa overflow. At
pkg/sql/plan/function/func_binary.go:8880-8899, trailing zeroes and the exponent are normalized before conversion overflow is decided. MySQL parses the mantissa into its bounded decimal buffer before applying the exponent, so a compensating exponent cannot erase an already overflowing significand. On MySQL 8.4.10, the adjacent controlCONCAT('1', REPEAT('0',80), 'e-80')returns00:00:01.000000with no warning, whileCONCAT('1', REPEAT('0',81), 'e-81')returns838:59:59.000000with both 1292 DECIMAL and TIME warnings. Exact head returns00:00:01with zero warnings for the latter. A focused exact-head counterexample fails withexpected 838:59:59, but get 00:00:01. Track the original mantissa capacity separately from the normalized TIME arithmetic, preserve MySQL overflow clamping, and add the adjacent 81/82-digit controls. -
[P1] Non-strict/IGNORE string TIME assignment still hard-fails above MatrixOne internal range. At
pkg/sql/plan/function/func_cast.go:8029-8034,types.ParseTimereturns before assignment mode can classify a syntactically valid range overflow. MySQL 8.4.10 stores838:59:59.000000with warning 1264 for2562047788:00:00under both emptysql_modeandINSERT IGNORE; strict assignment rejects it. Exact head instead returnsinvalid input: invalid time value 2562047788:00:00even in non-strictNewAssignCast. Distinguish range overflow from malformed input before returning, then route positive/negative non-strict and IGNORE cases through the existing clamp+warning path; keep strict behavior as an error.
Exact-head CI is terminal green and git diff --check passes. The focused counterexample tests fail deterministically on both paths above. I found no additional concurrency, lifecycle, allocation-retention, or normal-path performance blocker in the 21-file change map.
What type of PR is this?
Which issue(s) this PR fixes:
issue #25304
What this PR does / why we need it:
fix: align sec_to_time and time assignment boundaries