fix(function): round fractional SUBSTRING_INDEX counts

[ck89119]

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #28401

Related: #28496 (DECIMAL arguments and prepared execution for PERIOD_ADD/PERIOD_DIFF).

What this PR does / why we need it:

Add a shared integer-argument contract to function registration and overload resolution. This prevents exact DECIMAL arguments from choosing a cheaper DOUBLE conversion when the parameter requires integer evaluation.

The contract is used by:

• SUBSTRING_INDEX: count;
• PERIOD_ADD and PERIOD_DIFF: both arguments;
• HEX: numeric input, preserving exact integer digits above 2^53.

The shared resolver converts DECIMAL and signed integer operands to INT64, and BIT/unsigned integers to UINT64, before overload selection. It returns ordinary implicit CAST targets to the binder. FLOAT and string inputs retain their existing overload behavior. The global conversion-cost table is unchanged, and functions without this parameter contract are unaffected.

SQL EXECUTE source-type specialization reads the same registration metadata, fixing prepared DECIMAL parameters without hard-coding individual function names. Normal, speculative, and string-domain resolution entry points use the same conversion policy.

The previous substringIndexTypeMatch, SUBSTRING_INDEX-specific explicit CAST injection, and explicit DECIMAL CAST kernel modifications have been removed. No new runtime overload IDs or MORPC capabilities are introduced, and existing serialized overloads keep their executors.

Intentional compatibility boundary

DECIMAL integer-argument overflow is an error in MatrixOne. Unlike MySQL's saturation plus warning, an out-of-range argument aborts the query. This is an explicitly chosen policy, covered by SQL/BVT error assertions, including the existing oversized negative SUBSTRING_INDEX count case.

This PR does not change existing explicit CAST behavior or ordinary assignment conversion implementations. It does not claim complete MySQL compatibility for all FLOAT input forms: existing DOUBLE constant-versus-column discrepancies remain outside this DECIMAL-focused change.

Validation

Using Go 1.26.4:

• Full pkg/sql/plan/function and pkg/sql/plan tests
• TestIssue27088PreparedDecimalCommonType
• TestIssue28401SubstringIndexDecimalCount, expanded with all three additional functions, all DECIMAL widths, prepared SQL, exact HEX boundaries, NULL and overflow controls
• Shared resolver UTs covering ordinary/speculative/string-domain entry points, source-type immutability, unsigned/BIT and unrelated-function controls
• make build-with-prebuilt-native
• Scoped go vet
• Scoped golangci-lint v2.6.2 built with Go 1.26.4: 0 issues
• exact_integer_arguments.sql: 24/24 statements, twice on the same instance
• func_string_substringindex.test: 105/105 statements, twice on the same instance
• MySQL 8.4.8 comparison for DECIMAL PERIOD_ADD/PERIOD_DIFF/HEX and SQL EXECUTE reproductions
• git diff --check

[qodo-code-review]

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 →

[fengttt]

I am not convinced this is the right fix. So there is a function that expects an int argument. We give it a decimal. So there is an automatic cast. All fine. But the fix really should be all about this cast semantics and behaviour, and should have absolutely nothing to do with substring_index function.

This is a bad fix. We should clearly spec beaviour/semantics of these function argument cast and implement a full solution. fix one function by one function is not correct.

[aunjgr]

Reviewed all 13 changed files at b878d1c against base 315502e.

[P1] The new capability threshold is already allocated on main. pkg/defines/const.go assigns v61 to DECIMAL SUBSTRING_INDEX; remoterun.go permits overloads 3/4 at that threshold. Current upstream main assigns v61 to index-metadata provenance (and v62 to VARCHAR OCT). Those workers do not implement the new SUBSTRING_INDEX overloads, so the gate can admit an unsupported remote expression. Merge current main and allocate a fresh, coordinated capability, updating sender/receiver tests and feature detection together. Do not reuse another feature's version.

[P2] The exact-decimal fix still excludes Decimal256. The new overloads and getDecimalCount cover only Decimal64/128. Decimal256 still resolves through the DOUBLE overload, whose getCount truncates. Thus a count such as CAST(1.5 AS DECIMAL(65,1)) retains the old one-component result rather than the rounded two-component result; casting the same value to a narrower DECIMAL changes behavior. Cover the full supported exact-decimal family, preferably through the shared argument-conversion contract requested in the existing owner review. Add resolver and positive/negative half-boundary tests for Decimal256.

The Decimal256 dispatch observation from the prior reviewer remains semantically valid: the overload table, matcher and kernels are unchanged from reviewed 72515a5. I inspected those paths and reused that prior focused resolver evidence, rather than claiming a new local SQL run. Current-head CI is green. Masks, NULL propagation, selected string domain, existing float truncation, and persisted legacy-overload protection were reviewed; no further confirmed blocker found.

[aptend]

Re-reviewed exact head d35c693a313ec1fbe97a2256a3e3b9d26be32ece, including every historical review/thread, the delta since the last reviewed head, and the complete current diff. The earlier FLOAT64, exact-decimal, Decimal256, overload-ID, result-domain, selection-mask, and exhausted protocol-version findings are addressed by moving DECIMAL counts onto the shared implicit BIGINT cast. One rolling-upgrade correctness blocker remains: this changes the runtime meaning of an existing serialized CAST overload without a new capability fence, so result correctness depends on which-version CN executes the projection.

Validation on the exact head: git diff --check; full pkg/sql/plan/function and pkg/sql/plan tests; focused implicit-cast, SUBSTRING_INDEX, and prepared-specialization tests. A temporary source-level regression test confirmed the base executor’s multi-step Decimal128.Scale(-37) converts the just-below-half counterexample to 1; it was removed afterward. MySQL 9.6 returns the empty string for the exact DECIMAL counterexample. The current PR body is also stale: it still says the FLOAT64 overload is rounded and claims CGo headers blocked tests, while the exact implementation preserves FLOAT64 truncation and all focused tests pass.

[ck89119]

@fengttt Thanks, I agree with the direction. I reworked the fix around the shared function-argument cast contract rather than adding DECIMAL-specific SUBSTRING_INDEX kernels.

The revised change:

• removes the DECIMAL64/128-specific overloads and kernels, the associated MORPC capability, and the persisted-expression special case;
• routes DECIMAL64, DECIMAL128, and DECIMAL256 counts through the existing INT64 overload using the ordinary implicit cast;
• implements exact DECIMAL-to-BIGINT conversion in the common cast path, with a single HALF_UP rounding step and BIGINT saturation, avoiding both FLOAT64 precision loss and high-scale double rounding;
• preserves the existing FLOAT64/UINT64/INT64 overload IDs and the established DOUBLE truncation behavior;
• ensures typed numeric variables used by SQL EXECUTE are materialized in their source domain before rebinding, so prepared execution uses the same shared cast contract.

I added resolver, legacy-overload, exact half-boundary, high-scale double-rounding, Decimal256, literal/table/prepared, and DOUBLE compatibility coverage. The focused and owning-package tests, embedded SQL regression, and BVT all pass.

[aptend]

Deep re-review of exact head 3f00cc8. I read all six historical review threads and their resolved/outdated state, checked the delta since my previous reviewed head d35c693, and re-audited the complete PR diff. The earlier DOUBLE-domain, exact Decimal64/128/256 rounding, overload-ID, BIT(64), and test lifecycle issues are closed. Four blockers remain: normal CAST now regresses unrelated SQL behavior, the replacement cast kernel still violates selection masks, the new protocol number collides with other in-flight contracts, and the exact head is not gofmt-clean. Validation: focused protocol/planner/function tests and focused race tests pass; a review-only selection-mask counterexample fails deterministically and was removed; full affected packages pass; git diff --check passes; exact-head CI independently fails UT Coverage, proxy/pessimistic BVT, and SCA for the concrete cases below.

[aptend]

Re-reviewed the complete diff and all prior review/thread history at exact head dfb5033. The earlier normal DECIMAL-to-BIGINT overflow, FunctionSelectList, and protocol-version blockers are closed by the latest change against the current base. One required-check blocker remains inline. Focused SUBSTRING_INDEX/cast and prepared-plan tests passed; the exact-head formatting check still fails.

[fengttt]

My question is not answered/resolved, so why am I requested for review again?

[fengttt]

My question is not answered/resolved, so why am I requested for review again?

@ck89119
The answered PR/code diff, does not answer my question at all. You need to go back to design, draw board and fix everything at data type cast level. This feature/fix has nothing to do with substring_index itself except it is a test case.

[aptend]

Deep re-review of exact head 28a209593b415a99ab019b5e894fd7dc013da7d6. I read the complete review/comment/thread history, checked the delta since my previous reviewed head dfb50334fede74608e5477eeb82e8d04e10d80b1, and re-audited the full current diff. The earlier overflow, FunctionSelectList, protocol-version, and gofmt blockers are closed. One blocking contract/design gap remains: the patch opts only SUBSTRING_INDEX into exact DECIMAL integer-argument conversion, so the same fixed-integer argument contract still gives contradictory results in other built-ins. The exact counterexample and impact are inline.

Validation on this head: git diff --check; gofmt check; focused function/planner tests; TestIssue28401SubstringIndexDecimalCount; and a temporary embedded-cluster cross-function counterexample (removed afterward). The current exact-head CI failure is in the unrelated pkg/vectorindex JSON fixture (TestAppendColumnBuffer_AllTypes/json), with coverage failing only because that producer did not publish a complete profile.

[aptend]

Deep re-review completed at exact head 7a39aad.

I read the full review/thread history, rechecked the delta since the prior reviewed head, and reviewed the complete PR diff. The earlier blockers are closed: the shared integer-argument metadata now sends exact DECIMAL/signed inputs through INT64 and unsigned/BIT inputs through UINT64 without changing the global cast table or existing overload IDs; prepared-statement rebinding consults the same contract; FLOAT and string behavior remain on their legacy paths. Overflow follows the documented ordinary-cast error policy.

Validation on the pinned worktree: git diff --check; gofmt -d on all changed Go files; race-enabled targeted function and plan tests; embedded-cluster TestIssue28401SubstringIndexDecimalCount. All passed. Current build, SCA, coverage, and proxy BVT checks are also green; remaining CI jobs were still running at submission time.

[ck89119]

@fengttt 已修改修复思路，本次修改主要调整如下：

• 删除 substringIndexTypeMatch，以及 binder 中按 SUBSTRING_INDEX 函数名插入显式 CAST 的特判；也撤回了此前对显式 DECIMAL→BIGINT kernel 的修改。
• 在函数注册中增加 integerArguments 参数约束，由共享的 checkArgumentTypes 在 overload 选择前应用：DECIMAL/有符号整数走 INT64，BIT/无符号整数走 UINT64；FLOAT 和字符串保留原有路径，不修改全局转换代价。
• SUBSTRING_INDEX 的 count、PERIOD_ADD/PERIOD_DIFF 的两个参数和 HEX 的数值参数共用这套机制，复用普通隐式 CAST，不新增函数专用转换 kernel。
• SQL EXECUTE 的参数来源类型保留也读取同一注册元数据，不再按函数名特判，修复了 PERIOD_ADD/PERIOD_DIFF 的 prepared DECIMAL 参数报错及 HEX 的精度问题。

越界策略已明确选择 MO 的报错行为，不采用 MySQL 的成功加 warning；这是有意保留的兼容性差异，已写入 PR 描述和回归测试。现有显式 CAST 的行为不变。

[aunjgr]

Reviewed revision: 7a39aad0a1a7da39108717530184069d26f0d1f9.

Independent review: no blocking finding. Checked integer-argument metadata across ordinary, speculative and string-domain resolution, prepared source specialization, input-type immutability, DECIMAL/unsigned/BIT/NULL/overflow controls and public SQL cases.

The scoped policy is explicit: DECIMAL integer-argument overflow errors, FLOAT behavior is unchanged, and existing serialized overload executors remain intact. No global conversion-cost change.

Reused exact-head CI run 34822808760; no local kernel/BVT rerun.