[Bartok9]

Summary

• The Trino cursor type parser maps DECIMAL(p) (precision only) and bare DECIMAL to a PyArrow decimal with a default scale of 9 instead of scale 0.
• This mistypes every precision-only DECIMAL column, and for small precisions (e.g. DECIMAL(5)) produces scale > precision, which PyArrow rejects outright.

Root Cause

Symptom — Columns declared as DECIMAL(p) (or bare DECIMAL) in Trino come back from wren with a fractional scale (e.g. DECIMAL(10) → decimal128(10, 9)). For small precisions like DECIMAL(5), materialising the result table raises pyarrow.lib.ArrowInvalid because the requested scale (9) exceeds the precision (5).

Root cause — In _trino_data_type_to_arrow (core/wren/src/wren/connector/trino.py), the T.DECIMAL branch initialises precision, scale = 38, 9. The scale is only overwritten when an explicit second parameter is present. But in Trino DECIMAL(p) means precision p with scale 0, and bare DECIMAL is DECIMAL(38, 0). The non-zero default scale is wrong.

Evidence — Before the fix, on current main:

decimal(10)   -> decimal128(10, 9)   # should be (10, 0)
decimal       -> decimal128(38, 9)   # should be (38, 0)
decimal(5)    -> decimal128(5, 9)    # scale > precision; ArrowInvalid when building the array
decimal(10,2) -> decimal128(10, 2)   # explicit scale unaffected (correct)

The existing test even codified the wrong default (("decimal", pa.decimal128(38, 9))); it is corrected here to (38, 0).

Fix + why this level — Change the default to precision, scale = 38, 0. This is the correct layer: the cursor-type → Arrow mapping is the single place Trino DECIMAL types are interpreted, so fixing the default here corrects schema typing, value coercion, and avoids the downstream ArrowInvalid without touching any caller. The explicit DECIMAL(p, s) path is unchanged.

Scope / risk — Touches only the T.DECIMAL branch in the Trino connector (and the corresponding test expectations). DECIMAL(p, s) behaviour is identical. This mirrors the same fix in the Athena connector (#2403); the two connectors share the Trino-flavoured type grammar.

Verification

• uv run --no-sync pytest tests/connectors/test_trino.py -k data_type — 40 passed (was 37; +3 new cases).
• ruff format --check src/ + ruff check src/ — clean.

Real behavior proof

Captured from a real run on this branch (uv run python3):

decimal(10)   -> decimal128(10, 0)
decimal       -> decimal128(38, 0)
decimal(5)    -> decimal128(5, 0)
decimal(10,2) -> decimal128(10, 2)

Regression cases (added to the parametrized test_parse_trino_data_type) assert the new shape and FAIL on current main:

FAILED test_parse_trino_data_type[decimal-expected18]      assert decimal128(38, 9) == decimal128(38, 0)
FAILED test_parse_trino_data_type[decimal(10)-expected19]  assert decimal128(10, 9) == decimal128(10, 0)
FAILED test_parse_trino_data_type[decimal(5)-expected20]   assert decimal128(5, 9)  == decimal128(5, 0)

All three pass with the fix.

Summary by CodeRabbit

• Bug Fixes
  • Corrected parsing of Trino and Athena DECIMAL types so bare DECIMAL and precision-only DECIMAL(p) now default to scale 0.
  • Improved handling of small-precision decimals (e.g., DECIMAL(5)) to prevent invalid decimal definitions during conversion.
  • Updated and expanded unit-test expectations to match the revised DECIMAL parsing behavior.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 56a40e49-f7e6-433a-82cc-fc2f24326496

📥 Commits

Reviewing files that changed from the base of the PR and between 62a6824 and 005c906.

📒 Files selected for processing (3)

• core/wren/src/wren/connector/trino.py
• core/wren/tests/connectors/test_trino.py
• core/wren/tests/unit/test_athena_connector.py

🚧 Files skipped from review as they are similar to previous changes (3)

• core/wren/src/wren/connector/trino.py
• core/wren/tests/connectors/test_trino.py
• core/wren/tests/unit/test_athena_connector.py

---

Walkthrough

Bare and precision-only Trino DECIMAL types now default to scale 0 instead of 9. Matching Trino and Athena tests were updated, including added coverage for decimal(5).

Changes

DECIMAL Scale Default Fix

Layer / File(s)	Summary
Trino connector decimal default change core/wren/src/wren/connector/trino.py	Changes bare DECIMAL fallback from (38, 9) to (38, 0) and adds comments on Trino precision/scale semantics and PyArrow validity constraints.
Connector decimal expectation matrix core/wren/tests/connectors/test_trino.py, core/wren/tests/unit/test_athena_connector.py	Updates parametrized decimal cases to expect scale 0 for bare decimal and decimal(p) inputs, including added decimal(5) coverage.
Athena decimal regression tests core/wren/tests/unit/test_athena_connector.py	Adds standalone Athena tests for bare decimal, precision-only decimal(p), and decimal(5) to assert scale 0 results.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

• Canner/WrenAI#2403: Also updates DECIMAL-to-PyArrow mapping so bare and precision-only decimals default to scale 0, with related tests.

Suggested reviewers

• goldmedal

Poem

🐇 A decimal hopped in, all tidy and neat,
With scale set to zero, it found its seat.
Trino and Athena now parse with delight,
No scale-too-big trouble, just arrows set right.
Hooray for the rabbits and numbers tonight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is clear and directly describes the main DECIMAL scale fix in Trino, which matches the core change in the PR.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 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/trino.py`:
- Around line 103-109: The Athena DECIMAL parsing logic is still using the old
Trino default and needs the same fix as the Trino connector. Update the DECIMAL
handling in the athena parser to match the semantics used in trino.py: bare
DECIMAL and DECIMAL(p) should default to precision 38 and scale 0, and any
precision-only DECIMAL path should avoid producing scale values larger than
precision. Make the change in the DECIMAL branch of the Athena connector parser
so both connectors stay consistent.

🪄 Autofix (Beta)

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

Run ID: 78d148e0-69e5-4363-9d05-317b227a13bb

📥 Commits

Reviewing files that changed from the base of the PR and between 89ea46c and 782d923.

📒 Files selected for processing (2)

• core/wren/src/wren/connector/trino.py
• core/wren/tests/connectors/test_trino.py

[Bartok9]

Good catch — the Athena connector shares the same Trino-flavoured DECIMAL parsing and had the identical bug. Pushed 3d078e4 applying the same fix there: bare DECIMAL → (38, 0) and DECIMAL(p) → scale 0, with scale capped at precision. Both connectors are now consistent.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

core/wren/src/wren/connector/athena.py (1)

93-107: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add Athena DECIMAL regression tests core/wren/tests/unit/test_athena_connector.py only covers decimal(12,4); add bare decimal and precision-only cases like decimal(10) and decimal(5) to keep Athena aligned with the Trino parser.

🤖 Prompt for 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.

In `@core/wren/src/wren/connector/athena.py` around lines 93 - 107, Add regression
coverage in test_athena_connector for Athena DECIMAL handling beyond
decimal(12,4): include bare decimal, precision-only decimal(10), and a small
precision case like decimal(5) to verify the Athena connector stays aligned with
the Trino parser and the DECIMAL logic in the Athena type conversion path.

🤖 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.

Outside diff comments:
In `@core/wren/src/wren/connector/athena.py`:
- Around line 93-107: Add regression coverage in test_athena_connector for
Athena DECIMAL handling beyond decimal(12,4): include bare decimal,
precision-only decimal(10), and a small precision case like decimal(5) to verify
the Athena connector stays aligned with the Trino parser and the DECIMAL logic
in the Athena type conversion path.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 59517541-cb79-490b-8afe-5b86480b54b2

📥 Commits

Reviewing files that changed from the base of the PR and between 782d923 and 3d078e4.

📒 Files selected for processing (1)

• core/wren/src/wren/connector/athena.py

[Bartok9]

Added the requested Athena DECIMAL regression coverage in test_athena_connector.py — bare decimal → (38, 0), decimal(10) → (10, 0), and the small-precision decimal(5) → (5, 0) case that previously risked scale > precision. pytest -k decimal → 4 passed. Both connectors now have matching coverage.

[Bartok9]

Addressed the CodeRabbit suggestion in 2ed54d6: added test_parse_athena_type_decimal_bare_defaults_to_scale_0, ...precision_only_has_scale_0 (decimal(10)), and ...small_precision_only (decimal(5)) to test_athena_connector.py. All 4 decimal cases pass locally, keeping Athena aligned with the Trino parser.

[goldmedal]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[goldmedal]

@Bartok9, there is some conflict. Could you resolve it?

[Bartok9]

Thanks @goldmedal — and thanks for the review. Conflict resolved: merged latest main (the Athena fix #2403 had landed independently, so I merged it and kept the identical decimal logic). The 9 Trino/Athena DECIMAL tests still pass locally (pytest -k decimal). Branch is back to mergeable.

[Bartok9]

Thanks @goldmedal — conflict resolved. ✅

The branch had picked up a tangled history (duplicate commits + merge commits) after the ibis-drop refactor (#2313) landed on main. I've rebased it onto current main as a single clean commit — the net change is unchanged (3 files, +39/−4):

• core/wren/src/wren/connector/trino.py — DECIMAL(p) → scale 0; bare DECIMAL → (38, 0)
• core/wren/src/wren/connector/athena.py — same semantics, kept aligned with the Trino parser
• regression tests for bare DECIMAL, DECIMAL(p), and the small-precision case (DECIMAL(5)) that previously produced scale > precision → ArrowInvalid

CI is re-running on the clean commit; lint already green. Should be a clean merge now — let me know if you'd like anything else adjusted. 🙏

[goldmedal]

Thanks @Bartok9 look good 👍