Skip to content

chore(ibis-server): Fix real type & nested composite type mapping for Trino engine#1456

Merged
goldmedal merged 1 commit intoCanner:mainfrom
okayhooni:hotfix/trino-composite-type-mapping
Mar 17, 2026
Merged

chore(ibis-server): Fix real type & nested composite type mapping for Trino engine#1456
goldmedal merged 1 commit intoCanner:mainfrom
okayhooni:hotfix/trino-composite-type-mapping

Conversation

@okayhooni
Copy link
Copy Markdown
Contributor

@okayhooni okayhooni commented Mar 17, 2026

  • Add real type mapping to RustWrenEngineColumnType.REAL in Trino type mapping
  • Preserve original type string for complex/parametric types (array, map, row), consistent with BigQuery and Databricks implementations
  • Normalize type string before regex substitution to avoid redundant .lower() call

example)

  • before)
image
  • after)
image

Summary by CodeRabbit

Bug Fixes

  • Enhanced Trino data type handling with support for the "real" type and improved processing of complex types (arrays, maps, rows).
  • Optimized type normalization for more consistent type mapping and fewer unmapped type warnings.

@github-actions github-actions bot added ibis python Pull requests that update Python code labels Mar 17, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 17, 2026

📝 Walkthrough

Walkthrough

Added "real" type mapping to TRINO_TYPE_MAPPING and refactored TrinoMetadata._transform_column_type to normalize lowercased input types and special-case complex parametric types (array, map, row) to return original data_type unchanged.

Changes

Cohort / File(s) Summary
Type Mapping and Normalization
ibis-server/app/model/metadata/trino.py
Added "real" type mapping to TRINO_TYPE_MAPPING; refactored _transform_column_type to normalize lowercased types, special-case complex parametric types, and remove parameters from normalized values before mapping lookup.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Suggested labels

ibis, python

Suggested reviewers

  • goldmedal
  • douenergy

Poem

A real type mapping blooms, 🌱
Types dance through lowercase streams,
Complex cases shine so bright,
Parameters fade from sight,
Data flows just right! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check ✅ Passed The title clearly describes the main changes: fixing type mapping for both the 'real' type and nested composite types in the Trino engine, which aligns with the PR objectives.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

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

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

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

⚠️ Outside diff range comments (1)
ibis-server/app/model/metadata/trino.py (1)

143-157: ⚠️ Potential issue | 🟡 Minor

Return type annotation does not match implementation.

Line 143 declares RustWrenEngineColumnType, but the method also returns str at line 157 for complex types. Update the signature and docstring to use a union type.

Note: The comment references "Databricks implementations" for consistency, but Databricks does not return raw strings—it always returns RustWrenEngineColumnType values. BigQuery correctly uses the union type approach shown in the proposed fix.

🔧 Proposed fix
-    def _transform_column_type(self, data_type: str) -> RustWrenEngineColumnType:
+    def _transform_column_type(self, data_type: str) -> str | RustWrenEngineColumnType:
         """Transform Trino data type to RustWrenEngineColumnType.
 
         Args:
             data_type: The Trino data type string
 
         Returns:
-            The corresponding RustWrenEngineColumnType
+            The corresponding RustWrenEngineColumnType or original string for
+            complex types
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@ibis-server/app/model/metadata/trino.py` around lines 143 - 157, The
_transform_column_type method currently declares a return type of
RustWrenEngineColumnType but returns raw strings for complex/parametric types;
change the signature to return a union (RustWrenEngineColumnType | str) and
update the docstring to state it may return either a RustWrenEngineColumnType or
the original type string for complex types (preserve behavior for
array/map/row). Ensure any needed typing imports (e.g., from typing import Union
or using PEP 604 syntax) are added and keep the logic in _transform_column_type
unchanged except for the annotated return type and docstring.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@ibis-server/app/model/metadata/trino.py`:
- Around line 143-157: The _transform_column_type method currently declares a
return type of RustWrenEngineColumnType but returns raw strings for
complex/parametric types; change the signature to return a union
(RustWrenEngineColumnType | str) and update the docstring to state it may return
either a RustWrenEngineColumnType or the original type string for complex types
(preserve behavior for array/map/row). Ensure any needed typing imports (e.g.,
from typing import Union or using PEP 604 syntax) are added and keep the logic
in _transform_column_type unchanged except for the annotated return type and
docstring.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b9f42d23-7e7a-4c8c-9dd5-3958e995a7f4

📥 Commits

Reviewing files that changed from the base of the PR and between 642b7ee and 9cb147c.

📒 Files selected for processing (1)
  • ibis-server/app/model/metadata/trino.py

@okayhooni okayhooni changed the title chore) Fix real type & nested composite type mapping for Trino engine chore(ibis-server): Fix real type & nested composite type mapping for Trino engine Mar 17, 2026
Copy link
Copy Markdown
Contributor

@goldmedal goldmedal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @okayhooni 👍

@goldmedal goldmedal merged commit 264d1fa into Canner:main Mar 17, 2026
6 of 8 checks passed
chilijung pushed a commit that referenced this pull request Mar 18, 2026
nhaluc1005 pushed a commit to nhaluc1005/text2sql-practice that referenced this pull request Apr 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ibis python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants