Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

array_min / array_max behavior with NaN and NULL #21065

Closed
amitkdutta opened this issue Oct 6, 2023 · 7 comments
Closed

array_min / array_max behavior with NaN and NULL #21065

amitkdutta opened this issue Oct 6, 2023 · 7 comments
Assignees
Labels

Comments

@amitkdutta
Copy link
Contributor

In Presto, array_max or array_min both returns NAN if at least one element in the array is NAN.

SELECT array_max(ARRAY [4.0, nan(), NULL])  --> NAN
SELECT array_min(ARRAY [4.0, nan(), NULL])  --> NAN

Wondering if the behavior is correct or not.

CC: @kaikalur @prithvip @mbasmanova

@amitkdutta amitkdutta added the bug label Oct 6, 2023
@mbasmanova mbasmanova changed the title Array Min Max behavior with NAN and NUL array_min / array_max behavior with NaN and NULL Oct 6, 2023
@mbasmanova
Copy link
Contributor

CC: @tdcmeehan @aditi-pandit

@kaikalur
Copy link
Contributor

kaikalur commented Oct 7, 2023

It's unfortunate but it's what it is :( We can't change without breaking existing stuff. Maybe we lump all these things together in a legacy flag so we can fix them in native. I would generally like ARRAY_MAX having similar behavior as MAX, but its not:

maxpresto:di> select max(x) from (values 4.0,nan(),null) T(x);
 _col0
-------
   4.0


presto:di> select array_max(x) from (values array[4.0,nan(),null]) T(x);
 _col0
-------
   NaN

@mbasmanova
Copy link
Contributor

@kaikalur I see that min and max ignore NULL and NaN:

presto:di> select min(x), max(x) from (values 4.0,nan(),null) T(x);
 _col0 | _col1
-------+-------
   4.0 |   4.0
(1 row)

presto:di> select min(x), max(x) from (values 4.0,null) T(x);
 _col0 | _col1
-------+-------
   4.0 |   4.0
(1 row)

array_min, array_max return NaN if any element is NaN, NULL if no element is NaN, but there is a NULL element, min/max otherwise.

@Akanksha-kedia
Copy link
Contributor

@amitkdutta can someone help to assign this i want to work on this.

@tdcmeehan
Copy link
Contributor

It seems like the path forward here is to fix this and gate it by a flag. I agree with @kaikalur that it seems like consistency with min/max is the way to go.

@amitkdutta
Copy link
Contributor Author

Thanks @tdcmeehan. Let's keep the legacy behavior by default.
@mbasmanova In Velox, we want legacy and new behavior (where new is array_min and array_max matches min and max) both and keep legacy enabled by default?

@mbasmanova
Copy link
Contributor

@amitkdutta Amit, ideally, we would avoid introducing legacy behavior in Velox, but it may not be possible due to existing workloads. Assuming this is not an option, let's keep the legacy behavior by default and add a flag to enable "new" behavior consistent with min and max aggregate functions.

Going forward, let's make sure to document edge cases of functions to allow for consistency check when function is introduced for the first time.

amitkdutta pushed a commit to amitkdutta/velox that referenced this issue Oct 18, 2023
Summary: Based on  prestodb/presto#21065

Differential Revision: D50386287
amitkdutta added a commit to amitkdutta/velox that referenced this issue Oct 18, 2023
amitkdutta added a commit to amitkdutta/velox that referenced this issue Oct 19, 2023
Summary:
Pull Request resolved: facebookincubator#7128

Based on  prestodb/presto#21065

Differential Revision: D50386287

fbshipit-source-id: 404c7c885c54d3f70215811a1e2e7bc5874c50c9
amitkdutta pushed a commit to amitkdutta/velox that referenced this issue Oct 19, 2023
amitkdutta pushed a commit to amitkdutta/velox that referenced this issue Oct 19, 2023
amitkdutta pushed a commit to amitkdutta/velox that referenced this issue Oct 19, 2023
amitkdutta pushed a commit to amitkdutta/velox that referenced this issue Oct 19, 2023
amitkdutta pushed a commit to amitkdutta/velox that referenced this issue Oct 19, 2023
amitkdutta pushed a commit to amitkdutta/velox that referenced this issue Oct 19, 2023
amitkdutta pushed a commit to amitkdutta/velox that referenced this issue Oct 19, 2023
amitkdutta pushed a commit to amitkdutta/velox that referenced this issue Oct 20, 2023
Summary:

Based on  prestodb/presto#21065

Reviewed By: mbasmanova

Differential Revision: D50386287
amitkdutta pushed a commit to amitkdutta/velox that referenced this issue Oct 21, 2023
Summary:

Based on  prestodb/presto#21065

Reviewed By: mbasmanova

Differential Revision: D50386287
amitkdutta pushed a commit to amitkdutta/velox that referenced this issue Oct 21, 2023
Summary:

Based on  prestodb/presto#21065

Reviewed By: mbasmanova

Differential Revision: D50386287
amitkdutta pushed a commit to amitkdutta/velox that referenced this issue Oct 21, 2023
Summary:

Based on  prestodb/presto#21065

Reviewed By: mbasmanova

Differential Revision: D50386287
amitkdutta pushed a commit to amitkdutta/velox that referenced this issue Oct 21, 2023
Summary:

Based on  prestodb/presto#21065

Reviewed By: mbasmanova

Differential Revision: D50386287
facebook-github-bot pushed a commit to facebookincubator/velox that referenced this issue Oct 21, 2023
…o. (#7128)

Summary:
Pull Request resolved: #7128

Based on  prestodb/presto#21065

Reviewed By: mbasmanova

Differential Revision: D50386287

fbshipit-source-id: 15dd1594cb0976eb4f87b4f29531998a1c7466cb
@elharo elharo closed this as completed Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

No branches or pull requests

6 participants