Skip to content

Use consistent NaN behavior for max functions#5851

Merged
electrum merged 1 commit intotrinodb:masterfrom
electrum:nan
Nov 6, 2020
Merged

Use consistent NaN behavior for max functions#5851
electrum merged 1 commit intotrinodb:masterfrom
electrum:nan

Conversation

@electrum
Copy link
Copy Markdown
Member

@electrum electrum commented Nov 6, 2020

NaN is effectively ignored, unless it is the only value. In other words, it is treated as the maximum value for min, and the minimum value for max.

From an implementation point of view, since it normally compares as the maximal value, we only need to special case it for maximum by reversing its comparator.

@cla-bot cla-bot bot added the cla-signed label Nov 6, 2020
Copy link
Copy Markdown
Member

@dain dain left a comment

Choose a reason for hiding this comment

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

Looks good.

@electrum electrum force-pushed the nan branch 2 times, most recently from a7c0ada to 6e9b1da Compare November 6, 2020 19:25
@findepi
Copy link
Copy Markdown
Member

findepi commented Nov 6, 2020

Title talks about max, i see you;re changing greatest. Are you taking care of array_{min,max} and {min,max}_by as well?
after the change, Is min going to be produce same result as array_min(array_agg(...)) , and same result as reduce(array_agg(...), ... -> least(...)?

@electrum
Copy link
Copy Markdown
Member Author

electrum commented Nov 6, 2020

I consider greatest to be the scalar version of max. It should touch all of the max related functions, unless I missed any. Those should all produce the same result -- that's the goal of making these consistent.

Note that the NULL behavior is different, since array_min / array_max returns NULL if any element is null. The same is true for least / greatest.

@electrum electrum merged commit 278f880 into trinodb:master Nov 6, 2020
@electrum electrum deleted the nan branch November 6, 2020 23:43
@electrum electrum mentioned this pull request Nov 10, 2020
10 tasks
@electrum electrum added this to the 346 milestone Nov 10, 2020
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.

3 participants