matrixorigin / matrixorigin/matrixone

[Refactoring]: Centralize integer-parameter coercion in the binding layer

Open
#28,893 0 comments 0 reactions 1 assignee Claimed by @ck89119 View on GitHub
kind/refactoring severity/s0
Dominant language
Go
Stars
1.9k
Forks
311
Avg merge
1d 3h
Merged PRs (30d)
768

Description

### Why do you want to refactor this code?

Functions with semantically integral arguments should declare their parameter types and rely on shared binding-layer coercion. They should not implement separate FLOAT/DECIMAL/string execution overloads merely to convert a count, offset, length, or position to an integer.

The discussion on #28428 exposed this problem: SUBSTRING_INDEX has FLOAT64, UINT64, and INT64 count overloads. Conversion-cost-based selection can send a DECIMAL argument through DOUBLE, after which the function truncates it internally. The ordinary DECIMAL-to-integer conversion already rounds the issue's fractional examples correctly, but is bypassed by that overload choice.

Related paths occur in PERIOD_ADD, PERIOD_DIFF, and HEX. Function-specific type matchers, binder name checks, and executor-local conversion rules make behavior inconsistent across functions and between constants, columns, and prepared parameters.

This issue requests a binding-layer refactor, not another set of per-function compatibility patches.

### Describe the solution you'd like

#### 1. Inventory all semantically integral parameters

Audit all function registrations and relevant binding paths, not only SUBSTRING_INDEX. Produce a reviewed inventory containing:

- Function and parameter position/role, including count, offset, length, position, precision, and other integer-valued arguments where applicable.
- Canonical signedness and width.
- Existing overload identities and executor-local conversions.
- Current constant, column, SQL EXECUTE, and binary prepared-parameter behavior.
- Whether the parameter genuinely allows fractions or a full unsigned range.

Do not infer an integer requirement from a parameter's name alone. Fractional seconds and genuinely floating-point parameters must remain outside integer coercion.

#### 2. Bind against canonical parameter contracts

For new SUBSTRING_INDEX expressions, declare count as INT64 only. Other source types must be coerced by the shared binder before calling the integer executor.

Apply the same architecture throughout the inventory. Use INT64 for signed integer contracts; retain an explicitly justified UINT64 contract where full unsigned semantics are required. Do not blindly convert every integral parameter to INT64.

A function registration should state the parameter contract, not provide its own rounding/conversion implementation. Generic conversion-cost preferences must not override a declared integer requirement. Genuinely overloaded numeric functions must remain unaffected.

#### 3. Define and implement shared coercion semantics

Before implementation, specify a source-to-target matrix covering:

- Signed/unsigned integers and BIT.
- DECIMAL64/128/256, including exact rounding without a DOUBLE intermediate or double rounding.
- FLOAT32/64: explicitly decide and document rounding behavior instead of preserving accidental executor-local casts.
- Strings, booleans, NULL, and other accepted/rejected source domains.
- Range checks, errors, and inactive-row behavior.

**Decision: integer-argument overflow is an error in MatrixOne.** Do not saturate to a boundary plus warning merely for MySQL compatibility. In particular, values outside INT64 range are rejected for the new SUBSTRING_INDEX count contract, including high UINT64/BIT values. Document the intentional compatibility change.

Keep function-argument coercion distinct from explicit CAST and assignment policy where those contracts differ. This refactor does not authorize changing existing explicit CAST semantics or silently weakening assignment range checks.

Constants, column values, SQL EXECUTE variables, and binary prepared parameters should reach the same declared target and shared coercion policy. Preserve source types during prepared binding rather than reinterpreting typed values as prepare-time text.

#### 4. Remove redundant overload selection and local conversions safely

- New bindings should select the canonical integer executor, not numeric overloads that exist only to coerce arguments.
- Remove redundant per-function matchers, binder name checks, and executor-local integer conversions as their consumers migrate.
- Do not delete, renumber, or reuse overload IDs referenced by persisted or serialized plans. Retain compatibility-only execution entries for old identities where required; exclude them from new binding.
- Audit remote execution, persisted expressions, plan rebinding, upgrades, and rollback. If new serialized semantics require a capability, coordinate a unique allocation rather than independently claiming a MORPC version.

#### Acceptance criteria

- A complete parameter inventory and reviewed conversion matrix exist before the broad migration.
- New SUBSTRING_INDEX expressions accept an INT64 count after shared coercion; no FLOAT/UINT count overload is selected for new expressions.
- All other inventoried integer-only parameters use the same binding mechanism with explicit signedness.
- Resolver tests assert selected identities and inserted conversions, including normal and speculative resolution.
- Public SQL tests cover constants, columns, SQL EXECUTE and binary prepared parameters; positive/negative fractional boundaries, NULL, BIT/UINT boundaries, high-precision DECIMAL, and overflow errors.
- Tests protect genuinely fractional arguments and unrelated overloaded functions.
- Existing serialized overloads remain executable according to the documented compatibility policy.
- No global DECIMAL-to-INT cost tweak or per-function conversion kernel substitutes for the shared design.

### Describe alternatives you've considered

- Adding function-specific type matchers or checking function names in the binder: duplicates policy and does not solve the architectural issue.
- Rounding inside the FLOAT executor: changes genuine FLOAT behavior and cannot recover DECIMAL precision already lost during conversion.
- Globally preferring DECIMAL-to-INT over DECIMAL-to-DOUBLE: can misbind functions that actually require fractional values.
- Routing all conversions through explicit CAST: conflates argument, explicit-cast, and assignment overflow policies.
- Immediately deleting old overload entries: breaks persisted and remote plan identities.

### Additional information

Related:

- #28401 — SUBSTRING_INDEX fractional DECIMAL count handling.
- #28496 — PERIOD_ADD/PERIOD_DIFF exact DECIMAL arguments and prepared execution.
- #28428 — motivating PR and design discussion.
- Reviewer discussion: https://github.com/matrixorigin/matrixone/pull/28428#discussion_r4000047925

Observed examples motivating the refactor include PERIOD_ADD(202401, 1.5), PERIOD_DIFF(202402.5, 202401), HEX(CAST('9007199254740993' AS DECIMAL(20,0))), and prepared DECIMAL arguments. MySQL comparisons are useful evidence, but complete MySQL compatibility is not the objective when it conflicts with the chosen overflow-error policy.

### Delivery subtasks

The implementation is split by independently reviewable semantic boundaries. The foundation owns the design review; migration PRs must reuse that approved contract and return to design review if they require new semantics or compatibility mechanisms.

- [ ] #28977 — shared binding contract, compatibility foundation, and representative SUBSTRING_INDEX migration
- [ ] #28978 — string position, count, and length consumers
- [ ] #28979 — numeric, date/time selector, and bounded utility consumers
- [ ] #28980 — bit-pattern and encoding consumers
- [ ] #28981 — FORMAT, MAKEDATE, and MAKETIME special consumers

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.