fix(wren): preserve wide MySQL and Doris decimals - #2657
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review. WalkthroughThe MySQL connector supports Arrow ChangesMySQL decimal conversion
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change preserves wide MySQL and Doris decimal values through exact Arrow decimals or string fallback, with targeted regression coverage and CI updates. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant MySQLCursor
participant _build_mysql_arrow_table
participant _mysql_decimal_type_for_values
participant PyArrow
MySQLCursor->>_build_mysql_arrow_table: metadata and fetched rows
_build_mysql_arrow_table->>_mysql_decimal_type_for_values: decimal metadata, unsigned flag, and values
_mysql_decimal_type_for_values->>PyArrow: decimal128 or decimal256 column
_mysql_decimal_type_for_values->>PyArrow: exact string column for unsupported values
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/wren/src/wren/connector/mysql.py`:
- Around line 359-373: Update _mysql_decimal_type_for_values so precision
overflow at the 76-digit boundary is resolved by reducing target_scale while
retaining enough integer digits for observed values, returning pa.decimal256(76,
adjusted_scale) when representable. For cases such as display_length=78,
scale=75, and value 12, produce DECIMAL(76,74); return pa.string() only when the
minimum required integer and scale digits still exceed 76.
In `@core/wren/tests/connectors/test_mysql_connector.py`:
- Around line 151-168: Update
test_decimal_multiplication_above_arrow_limit_uses_exact_string to avoid relying
on MySQL DECIMAL arithmetic for products exceeding its 65-digit precision. Use
the existing fake-f coverage for the string fallback, or switch this test to a
backend that preserves the full product precision, while retaining assertions
for the exact string result and Arrow-limit behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cab54815-02ba-4b68-ac98-6b38b09cf101
📒 Files selected for processing (4)
.github/workflows/wren-ci.ymlcore/wren/src/wren/connector/mysql.pycore/wren/tests/connectors/test_mysql_connector.pycore/wren/tests/unit/test_mysql_helpers.py
|
Reviewed the branch locally. The core fix looks correct — I fuzzed Three things worth addressing: 1. The empty / all-NULL fallback to
|
goldmedal
left a comment
goldmedal
left a comment
There was a problem hiding this comment.
Verified locally rather than by reading: fetched the branch, ran tests/unit/test_mysql_helpers.py (37 passed), and exercised _build_mysql_arrow_table / _mysql_decimal_type_for_values directly with a stub cursor. All 9 CI checks are green.
The value-aware approach fixes more than the reported case. On main, metadata that understates scale raises ArrowInvalid: Rescaling Decimal value would cause data loss; this branch handles it correctly. Nice catch on the M = length - (0 if unsigned else 1) - ... docstring too, the previous formula contradicted the code.
Four things I'd like addressed before merge, left inline: a measured hot-path regression, gratuitous escalation to decimal256, an undocumented result-dependent column type, and a path that is now unreachable in production.
| flags, | ||
| precision=precision, | ||
| scale=scale, | ||
| values=[row[i] for row in rows], |
There was a problem hiding this comment.
Hot-path regression: values is materialized for every column (Major)
This builds [row[i] for row in rows] for every column, and then _build_mysql_column builds the same per-column list a second time below. Only DECIMAL columns consume values, so every non-decimal column pays for a wasted full-result copy.
Measured on 20 columns x 200k rows, best of 3 runs, stub cursor so only this function is timed:
| column type | before | after |
|---|---|---|
| INT | 71 ms | 108 ms (+52%) |
| DECIMAL(12,4) | 483 ms | 1738 ms (+260%) |
| VARCHAR | 448 ms | 481 ms |
The decimal cost is _mysql_decimal_value_shape calling Decimal.as_tuple() on 4M values. For a semantic layer, decimal-heavy result sets are the common case, not the exception.
Suggestions, cheapest first:
- Materialize the columns once (
columns = list(zip(*rows))) and reuse them for both the type derivation and_build_mysql_column. - Pass
valuesonly whentype_code in _mysql_decimal_codes(), so non-decimal columns are untouched. - Consider optimistic-then-fallback: try
pa.array(values, type=<metadata type>)first and run the value-shape scan only when it raisesArrowInvalid. That keeps the common path at the previous cost while staying exact for the pathological cases this PR targets.
| return pa.decimal256(_ARROW_DECIMAL256_MAX_PRECISION, target_scale) | ||
|
|
||
| target_scale = max(scale, value_scale) | ||
| target_integer_digits = max(precision - scale, integer_digits) |
There was a problem hiding this comment.
Unnecessary escalation past the decimal128 boundary (Major)
Reserving the declared integer capacity even when no fetched value needs it lets a widened scale push the column into decimal256 gratuitously:
_mysql_decimal_type_for_values(40, 4, False, [Decimal("1.234567")])
# -> decimal256(40, 6)That is a declared DECIMAL(38, 4) whose observed value needs 1 integer digit and scale 6. decimal128(38, 6) holds it comfortably, and decimal128 is considerably better supported downstream than decimal256 (polars and several engines handle decimal256 poorly or not at all).
Please trim unused integer capacity to stay within precision 38 when the observed values fit, and escalate to decimal256 only when they genuinely do not.
| integer_digits = max(shape[0] for shape in shapes if shape is not None) | ||
| value_scale = max(shape[1] for shape in shapes if shape is not None) | ||
| if integer_digits + value_scale > _ARROW_DECIMAL256_MAX_PRECISION: | ||
| return pa.string() |
There was a problem hiding this comment.
Result-dependent column type deserves an explicit contract (Major)
The derived type depends on which rows come back, and MySqlConnector.query() applies a LIMIT via _apply_limit, with the MCP path probing at limit + 1:
_mysql_decimal_type_for_values(67, 0, False, [D("9"*70)]) # decimal256(70, 0)
_mysql_decimal_type_for_values(67, 0, False, [D("9"*70), D("9"*77)]) # pa.string()So the same query at LIMIT 1 and LIMIT 2 can return a numeric column and a string column, and one outlier row silently degrades the whole column to text. That may well be the right trade-off versus raising, but today it is both silent and undocumented. Please:
- log a warning when a decimal column falls back to
pa.string(), naming the column, and - state the contract in the
_build_mysql_arrow_table/_mysql_decimal_type_for_valuesdocstring, so consumers know the decimal type is derived per result set rather than per column.
| is_unsigned=bool(flags & FLAG.UNSIGNED), | ||
| values=values, | ||
| ) | ||
| return _arrow_decimal_from_mysql_field( |
There was a problem hiding this comment.
Minor
- This branch is now unreachable in production: the only caller of
_mysql_field_arrow_typealways passes a list forvalues, sovalues is not Noneis always true and_arrow_decimal_from_mysql_fieldsurvives as a test-only helper with 7 unit tests behind it. Fixing the first comment by passingvaluesonly for decimal type codes makes this branch live again; otherwise please drop it so the tests describe real behaviour. tests/connectors/test_mysql_connector.py:166—assert len(expected) in {77, 80}is loose for a parametrized test; carry the expected digit count in the parameters so each case asserts its own value.- No coverage for negative over-wide values. I checked and the behaviour is correct (
-9...9at 77 digits becomes a string, at 76 digits staysdecimal256(76, 0)), so it is worth pinning down next totests/unit/test_mysql_helpers.py:295.
|
Hi @Ray0907, to clear my review queue, I converted this PR to a draft. After addressing the comment, feel free to request my review. |
9cdd99a to
60fe177
Compare
|
@goldmedal Ready for re-review. Addressed all remaining comments:
Rebased onto the latest |
2 participants
Summary
decimal128through precision 38 anddecimal256through precision 76.understates the actual precision.
including a safe schema fallback for empty or all-null over-wide results.
What failure does this repair?
Wide MySQL DECIMAL values were always assigned
decimal128, so a value such asDECIMAL(65,30)failed while constructing the Arrow table.MySQL expression metadata can also disagree with the returned value. On MySQL 8.0.36:
The driver reported precision 66 (which derived an Arrow precision of 65), but returned
a 77-digit
Decimal. The connector then raised:The same failure is reproducible with
DECIMAL(40,0) * DECIMAL(40,0), which returns an80-digit value. This change derives the final column type after fetching values: values
that fit within 76 digits remain exact Arrow decimals, while wider values are emitted as
exact strings instead of raising or rounding.
How is it tested?
tests/unit/test_mysql_helpers.pycovers decimal128/decimal256 boundaries, unsignedmetadata, value-aware integer and scale widening, over-wide string fallback, and
empty/all-null schemas.
tests/connectors/test_mysql_connector.pyruns against MySQL 8.0.36 and coversDECIMAL(65,30), 66-digit addition, 77/80-digit multiplication, and wideSUMmetadata.
Local verification:
git diff --checkpassed.Duplicate check
Searched open PRs in
Canner/WrenAIforMySQL DECIMAL,Decimal256, anddecimalin titles; no duplicates were found.
Summary by CodeRabbit
Bug Fixes
Tests