add time extract - #26257
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? |
|
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep-reviewed updated exact head 9382044e1cdb43cae9472448463aee3c0f20b25d, including the author reply and the regressions added for the previous review. Those specific cases are fixed, but the replacement TIME-prefix state machine still has reachable MySQL 8.4 compatibility gaps.
P1 — Partial TIME prefix parsing still drops or rejects valid coercible fields
The parser returns immediately after a leading-colon minute and requires digits after a second colon, while also explicitly rejects a third colon and lets punctuation before the fractional dot poison compact fallback. Against MySQL 8.4.10:
:34:56must yield0/34/56; this head yields0/34/0.12:34:must yield12/34/0; this head returns NULL.12::56must yield0/0/12; this head returns NULL.12:34:56:must yield12/34/56; this head returns NULL.12-.abcmust yield0/0/12; this head returns NULL.
These are the same public VARCHAR/CHAR/TEXT overloads and are reachable from ordinary SQL. Please model MySQL prefix consumption as one consistent scanner instead of special-casing only the previously reported examples, and add the exact boundary matrix above to UT/BVT.
Validation: official MySQL 8.4.10 container under default SQL mode; a direct probe test against this exact head reproduced all five mismatches. The PR's focused string-extract tests pass with the deterministic CGo wrapper, so existing green tests do not cover this remaining state space.
|
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of exact head 208371b44ab1e64fddeff5dcba1eb0b1425da148, including the author comment and the added VARCHAR/CHAR/TEXT UT/BVT matrix. The five previously reported inputs are fixed, but the new prefix scanner still has reachable MySQL 8.4 compatibility gaps.
P1 — Prefix semantics still change across the compact, day-TIME, and DATETIME branches
The new mysqlCompactTimePrefixBoundary rejects a compact prefix whenever any later punctuation-delimited text contains a digit. MySQL instead keeps the already consumed compact prefix: 12-34 yields HOUR/MINUTE/SECOND 0/0/12, while this head returns NULL (func_unary.go:5050-5055,5131-5140). This is the numeric counterpart of the author-comment example 12-.abc.
The day-TIME path then applies compact-field semantics where MySQL treats the post-day field as hours. For 1 02, MySQL returns 26/0/0; this head returns 24/0/2 because 02 is parsed as compact seconds before adding the day (:5011-5032).
The separated-DATETIME path has the same state-fragmentation problem: 2024-12-20 12 and 2024-12-20 12: both return 12/0/0 in MySQL, but this head marks the grammar as matched-invalid and returns NULL because a minute is mandatory (:4909-4920). 2024-12-20 12::56 is another reachable case: MySQL returns 12/56/0, this head returns NULL.
Please make prefix consumption context-aware without reinterpreting already parsed fields, and add these boundary families to the oracle-derived UT/BVT matrix rather than only the individual examples.
Validation used an official MySQL 8.4.10 container with default SQL mode plus a direct probe of this exact parser. The author regressions pass under race (-count=5), the full function package passes, and build/vet/diff checks plus remote CI are green; the green suite does not cover these remaining branches.
同时为 VARCHAR、CHAR、TEXT 增加了完整的 UT/BVT 边界矩阵,并更新 HOUR、MINUTE、SECOND 的预期结果文件。 |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Blocking MySQL compatibility issue remains at eff97c01acb8f0c89b17910884827dca1dd76ff8.
Against MySQL 8.4.10, the following string arguments produce HOUR/MINUTE/SECOND as shown, but the current parser returns different results:
2024-12-20foo: MySQL0/20/24; current parser returns NULL (ok=false).2024-12-20 12:34::56: MySQL12/34/56; current parser returns12/34/0.2024-12-20 12-34: MySQL12/34/0; current parser returns12/0/0.
The date-time scanner still has isolated colon-specific branches and only recognizes an exact date-only form, so valid consumed prefix components are discarded or treated as invalid. Please make prefix consumption consistent after a valid date/clock prefix, and cover these cases for VARCHAR/CHAR/TEXT in UT and BVT.
|
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep-reviewed updated exact head 6b614bb55028082f68bd6616d4ccbccaff5d03c2. The three cases from the previous review are fixed, but the replacement parser still has a blocking same-shape compatibility gap.
P1 — Grammar ownership and prefix-consumption precedence remain inconsistent
I ran an 82-case differential matrix against an official MySQL 8.4.10 server under its default SQL mode and a MatrixOne server built from this exact head. Nine cases still disagree. Representative HOUR/MINUTE/SECOND triples are:
input MySQL this head
1 2 0/0/1 26/0/0
12:34:56 78 12/34/56 NULL
1-2-3 4:5:6 0/0/1 4/5/6
12345-1-1 1:2:3 NULL 1/23/45
--12:34 NULL 12/34/0
2024-12-20 ++12:34 12/34/0 NULL
2024-12-20 12:34:56+abc 0/20/24 12/34/56
2024-12-20 12:34:56-abc 0/20/24 12/34/56
These are not independent one-off omissions. mysqlSeparatedDatetimeClockForExtract, the day-TIME branch, compact fallback, and sign/suffix handling can each claim or reject the same prefix using different rules. For example, the separated-DATETIME classifier claims 12:34:56 78 as a date-shaped input and returns matched-invalid before the TIME scanner can preserve the valid clock, while any digit-only field before whitespace is treated as a day in 1 2 even though MySQL consumes a different compact prefix.
Please define one ordered grammar/consumption contract for ambiguous DATE/DATETIME/day-TIME/compact-TIME prefixes, with a single owner for sign, whitespace, and suffix termination, rather than adding branches for these examples. Add an oracle-derived regression matrix around the ambiguous boundaries above (nearest valid/invalid controls and VARCHAR/CHAR/TEXT), so the grammar classes—not only reported spellings—are closed.
Fresh exact-head service build, go list, build, vet, all TestStringTimeExtract* tests, and the full pkg/sql/plan/function suite pass. The existing green matrix does not cover these remaining states.
|
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep-reviewed updated exact head eafe2e7009eb7aa7d4c91a9e813a9390f0b0716d. The eight cases from the previous review are fixed, but three reachable grammar families still disagree with MySQL 8.4.10.
P1 — day-TIME ownership cannot be decided by post-space digit count alone
mysqlDayTimeClockCandidateForExtract requires at least two leading digits after the day separator (func_unary.go:5129-5134). MySQL distinguishes a lone compact field from a one-digit clock field by the following separator, so this rule loses valid day-TIME values:
input MySQL H/M/S this head
1 2:3 26/3/0 0/0/1
12 3:4 291/4/0 0/0/12
The existing 1 2 versus 1 02 regression is not sufficient; cover one-digit post-day hour with and without minute/second separators.
P1 — valid three-digit-year DATETIME is routed to compact TIME
The separated-DATETIME gate accepts only exactly 2 or 4 year digits (func_unary.go:4889-4891). MySQL accepts 123-2-3 4:5:6 as a separated DATETIME and returns 4/5/6; this head skips the DATETIME owner and returns compact-TIME 0/1/23. The boundary should cover 1/2/3/4/5-digit year prefixes rather than only the previously reported 1- and 5-digit controls.
P1 — a bare trailing sign terminates DATETIME ownership
mysqlDatetimeClockSuffixForExtract rejects +/- only when another byte follows (func_unary.go:4985-4995), explicitly treating a bare sign as a valid clock suffix. MySQL instead falls back to the DATE prefix:
input MySQL H/M/S this head
2024-12-20 12:34:56+ 0/20/24 12/34/56
2024-12-20 12:34:56- 0/20/24 12/34/56
All five mismatches were independently reproduced against the official MySQL 8.4.10 image and with a direct probe of this exact production parser. Existing TestStringTimeExtract* tests pass under race (-count=3), the full function package, vet, and diff checks pass, and CI is green; the current oracle matrix simply does not close these adjacent states.
|
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep-reviewed updated exact head 21c67bd0c85c8569a4788cf9de35022b04e2e83a. The five cases from the previous review are fixed, but two adjacent grammar families still disagree with MySQL 8.4.10.
P1 — Separated-DATETIME ownership is not determined by year width alone
mysqlSeparatedDatetimeClockForExtract now accepts only 2–4 year digits (pkg/sql/plan/function/func_unary.go:4887-4891). The new one-digit control covers 1-2-3 4:5:6, but MySQL's ownership also depends on the following clock-field widths:
input MySQL H/M/S this head
1-1-1 1:2:3 0/0/1 0/0/1
1-1-1 01:02:03 1/2/3 0/0/1
1-2-3 12:34:56 12/34/56 0/0/1
0-1-1 12:34:56 12/34/56 0/0/0
So treating every one-digit year as compact TIME is still an overfit boundary: changing only the width of the clock fields transfers ownership to DATETIME in MySQL. Please derive the owner from the complete prefix grammar and cover one-digit year × one-/two-digit clock fields (including year zero) for VARCHAR/CHAR/TEXT.
P1 — Pre-parse TrimSpace changes grammar ownership
timeStringToFixedWithNullOnError rewrites every nonempty input with strings.TrimSpace before classification (func_unary.go:4776-4785). That is not semantics-preserving for ambiguous prefixes:
input MySQL H/M/S this head
'123:34:56 78' NULL NULL
'123:34:56 78 ' 123/34/56 NULL
'838:59:59 78' NULL NULL
'838:59:59 78 ' 838/59/59 NULL
The same result holds with surrounding tabs. Detecting an all-whitespace value as NULL must not discard leading/trailing whitespace from a nonempty value before the ordered grammar sees it. Please keep whitespace ownership inside the parser and add padded/unpadded controls around the same ambiguous prefix.
Validation used direct SQL string literals against the official MySQL 8.4.10 image and the exact-head production parser. The existing TestStringTimeExtract* suite passes under -race -count=3, and remote CI is green; those tests do not cover either adjacent family.
What type of PR is this?
Which issue(s) this PR fixes:
issue #24504
What this PR does / why we need it:
此分支为 HOUR()、MINUTE()、SECOND() 增加 VARCHAR/CHAR/TEXT 字符串参数支持:
pkg/sql/plan/function 全包测试通过,未发现阻塞问题。