Skip to content

Fix NaN values handling in the progressPercentage property#13

Open
stelmukhov wants to merge 3 commits intotrinodb:mainfrom
stelmukhov:feature/nan-progress
Open

Fix NaN values handling in the progressPercentage property#13
stelmukhov wants to merge 3 commits intotrinodb:mainfrom
stelmukhov:feature/nan-progress

Conversation

@stelmukhov
Copy link
Copy Markdown
Member

A simple fix for trino-csharp-client#12 since the progressPercentage property is unused.

Another option is to change the progressPercentage type from long to double,
as it uses OptionalDouble in Trino (QueryProgressStats.java#L34).

Thanks

public long processedBytes { get; set; }
public long peakMemoryBytes { get; set; }
public long spilledBytes { get; set; }
public long progressPercentage { get; set; }
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.

Separate commit .. and also why are you removing this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Since it's unused and there are other properties that are also missing here, I decided that the simplest fix is to remove it. However, I can also change its type to double, as defined here: QueryProgressStats.java#L34

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, I'm ok either way, do we know why it's unused in Java? Is this a Presto legacy? I'd keep it as it's optional until it's removed from the Java

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.

Wdyt @wendigo @electrum .. should we remove here and also in the JDBC driver?

@stelmukhov stelmukhov requested a review from mosabua February 25, 2025 10:13
@QimbE
Copy link
Copy Markdown

QimbE commented Feb 19, 2026

Hi @mosabua @georgewfisher ,
We’re using this library in production and recently hit the issue that is fixed by this PR. The PR has been approved for a while, but it hasn’t been merged.
Could you please merge it and cut a new release when you have a moment? It would unblock us and likely other users as well.
Thank you

@wendigo
Copy link
Copy Markdown

wendigo commented Feb 19, 2026

@martint can I get maintainer status here? :)

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.

5 participants