Skip to content

fix: align sec_to_time and time assignment boundaries - #27780

Open
iamlinjunhong wants to merge 8 commits into
matrixorigin:mainfrom
iamlinjunhong:m-25304
Open

fix: align sec_to_time and time assignment boundaries#27780
iamlinjunhong wants to merge 8 commits into
matrixorigin:mainfrom
iamlinjunhong:m-25304

Conversation

@iamlinjunhong

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

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

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@aptend aptend left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread pkg/container/types/time.go Outdated
// 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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 aunjgr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Deep-reviewed exact head 131f95a426101621274dc2820707547ee0687091 against base 41f5649904db84dc1fcbe0c654c61b68d70f9e4d. The previous 1e-81 / 1e-82 boundary is fixed, but two public SQL compatibility blockers remain.

  1. [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 control CONCAT('1', REPEAT('0',80), 'e-80') returns 00:00:01.000000 with no warning, while CONCAT('1', REPEAT('0',81), 'e-81') returns 838:59:59.000000 with both 1292 DECIMAL and TIME warnings. Exact head returns 00:00:01 with zero warnings for the latter. A focused exact-head counterexample fails with expected 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.

  2. [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.ParseTime returns before assignment mode can classify a syntactically valid range overflow. MySQL 8.4.10 stores 838:59:59.000000 with warning 1264 for 2562047788:00:00 under both empty sql_mode and INSERT IGNORE; strict assignment rejects it. Exact head instead returns invalid input: invalid time value 2562047788:00:00 even in non-strict NewAssignCast. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants