Skip to content

Conversation

@losipiuk
Copy link
Member

@losipiuk losipiuk commented Sep 24, 2024

Ensure that approx_percentile performs explicit input validation and throw TrinoException with proper error code.

Relates to #11156

Description

Additional context and related issues

Release notes

( ) 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.
(x) Release notes are required, with the following suggested text:

# Section
* Improve error reporting on input validation for `approx_percentile` function ({issue}`23546`)

@cla-bot cla-bot bot added the cla-signed label Sep 24, 2024
@losipiuk losipiuk requested review from martint and wendigo September 24, 2024 15:05
Copy link
Member

Choose a reason for hiding this comment

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

This will cause value to be boxed to a Double on every invocation. In the past, we've seen this cause performance problems in some cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm - did not think of that.

Would having

    public static void checkCondition(boolean condition, ErrorCodeSupplier errorCode, Supplier<String> errorMessage) {
        if (!condition) {
            throw new TrinoException(errorCode, errorMessage.get());
        }
    }

and then doing

checkCondition(Double.isFinite(value), INVALID_FUNCTION_ARGUMENT, () -> String.format("value must be finite; was %s", value));

help?

Would boxing happen only on call to lambda then. I think so but not 100% sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added extra commit

Ensure we are not boxing primitive values passed as arguments in case
checkCondition method is not inlined.
Ensure that approx_percentile performs explicit input validation and
throw TrinoException with proper error code.
@losipiuk losipiuk force-pushed the lukaszos/improve-input-validation-for-approx-percentile-39d73c branch from 352154e to 152fef6 Compare September 24, 2024 16:31
@losipiuk losipiuk merged commit 60c494c into trinodb:master Sep 25, 2024
@github-actions github-actions bot added this to the 459 milestone Sep 25, 2024
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