Skip to content

Support decimal encoding of NaN and Infinities values#11

Closed
wendigo wants to merge 1 commit intomasterfrom
user/decimal-nan-negative
Closed

Support decimal encoding of NaN and Infinities values#11
wendigo wants to merge 1 commit intomasterfrom
user/decimal-nan-negative

Conversation

@wendigo
Copy link
Copy Markdown
Contributor

@wendigo wendigo commented Mar 30, 2026

No description provided.

Comment thread trino.py
Comment on lines +48 to +52
return "NaN"
if value.is_infinite():
if value.is_signed():
return "-Infinity"
return "Infinity"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These are valid values for Trino NUMBER but not for DECIMAL.
If we return NaN for decimal, this should fail somewhere later. I think for DECIMALs it's better to keep failing here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since decimal is shared between decimal and number I think it's better to fail in Trino once nan or infinity is returned for Decimal

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 30, 2026

I looked more into this. I see that Python's decimal.Decimal really corresponds well to Trino's NUMBER type. Match better than to Trino's DECIMAL. So conceptually this change makes sense.

However, TrinoType enum is what it is -- enum of all trino types.
Mapping on Trino side a NUMBER to a DECIMAL feels wrong, one would do the other way around (NUMBER is a strictly wider type than DECIMAL).

If we do not want have more entries in TrinoType enum, then I think we should rename the existing one (DECIMAL → NUMBER).

@wendigo
Copy link
Copy Markdown
Contributor Author

wendigo commented Mar 30, 2026

I can add a separate NUMBER type but it's handling will be similar to the decimal

@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 30, 2026

SGTM

@wendigo
Copy link
Copy Markdown
Contributor Author

wendigo commented Mar 30, 2026

@findepi which version? ;)

@findepi
Copy link
Copy Markdown
Member

findepi commented Mar 30, 2026

this one:

@findepi findepi closed this Mar 30, 2026
@findepi findepi deleted the user/decimal-nan-negative branch March 30, 2026 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants