Skip to content

Add support to NUMBER to Python UDFs#28901

Merged
wendigo merged 2 commits intomasterfrom
user/serafin/number-udf
Mar 30, 2026
Merged

Add support to NUMBER to Python UDFs#28901
wendigo merged 2 commits intomasterfrom
user/serafin/number-udf

Conversation

@wendigo
Copy link
Copy Markdown
Contributor

@wendigo wendigo commented Mar 27, 2026

Description

Additional context and related issues

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

## Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Mar 27, 2026
@wendigo wendigo requested a review from findepi March 27, 2026 17:23
case StandardTypes.DOUBLE -> TrinoType.DOUBLE;
case StandardTypes.REAL -> TrinoType.REAL;
case StandardTypes.DECIMAL -> TrinoType.DECIMAL;
case StandardTypes.DECIMAL, StandardTypes.NUMBER -> TrinoType.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.

What is the semantics of TrinoType enum?
I don't see javadoc nor usages on it... Why is it safe to present NUMBER as a decimal?

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.

It's a translation between the host and the wasm environment. Types are covered here: https://github.com/trinodb/trino-wasm-python/blob/master/pyhost.h#L10

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.

to me this looks like we need to expand that enum when a new trino type is added

""";

assertThat(assertions.query(
query + "SELECT multiply(NUMBER '1.12345', NUMBER '2.54321')"))
Copy link
Copy Markdown
Member

@findepi findepi Mar 30, 2026

Choose a reason for hiding this comment

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

Can you please add a test with NULL, NaN and ±Infinity?

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.

NaN and +/- Infinity are not supported by wasm python yet so nothing to test.

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.

the behavior should still be tested, regardless of current behavior

@wendigo wendigo requested a review from findepi March 30, 2026 11:06
@wendigo wendigo merged commit c30dd5d into master Mar 30, 2026
3 of 14 checks passed
@wendigo wendigo deleted the user/serafin/number-udf branch March 30, 2026 11:07
@github-actions github-actions bot added this to the 481 milestone Mar 30, 2026
@ebyhr ebyhr mentioned this pull request Mar 31, 2026
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