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.

Sign up for GitHub

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

Jump to bottom

Handle Nan in ArrayMinMax function and remove a todo. #6922

Closed

amitkdutta wants to merge 1 commit into facebookincubator:main from amitkdutta:export-D49935894

Contributor

amitkdutta commented Oct 5, 2023

Differential Revision: D49935894

netlify bot commented Oct 5, 2023 •

edited

Loading

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`4ced2ff`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/652071042b94e000087eca5f

facebook-github-bot added the CLA Signed label

Contributor

facebook-github-bot commented Oct 5, 2023

This pull request was exported from Phabricator. Differential Revision: D49935894

facebook-github-bot added the fb-exported label

amitkdutta requested a review from laithsakka

October 5, 2023 21:45

amitkdutta force-pushed the export-D49935894 branch from c846800 to a31f145 Compare

October 6, 2023 06:44

amitkdutta pushed a commit to amitkdutta/velox that referenced this pull request


          Handle Nan in ArrayMinMax function and remove a todo. (facebookincuba…

a31f145

…tor#6922)

Summary: Pull Request resolved: facebookincubator#6922

Differential Revision: D49935894

Contributor

facebook-github-bot commented Oct 6, 2023

This pull request was exported from Phabricator. Differential Revision: D49935894

amitkdutta changed the title ~~Remove a todo in ArrayMinMaxFunction.~~ Handle Nan in ArrayMinMax function and remove a todo.

Contributor

zacw7 commented Oct 6, 2023

Thanks for the quick fix. Could you please double check if other similar functions also have the same issue such as sum(), avg()?

amitkdutta pushed a commit to amitkdutta/velox that referenced this pull request


          Handle NaN in array_min/max functions (facebookincubator#6922)

5bd6294

Summary:
Also, re-use input strings for result.


Differential Revision: D49935894

amitkdutta force-pushed the export-D49935894 branch from a31f145 to 5bd6294 Compare

October 6, 2023 17:01

Contributor

facebook-github-bot commented Oct 6, 2023

This pull request was exported from Phabricator. Differential Revision: D49935894

amitkdutta pushed a commit to amitkdutta/velox that referenced this pull request


          Handle NaN in array_min/max functions (facebookincubator#6922)

5b15d17

Summary:
Also, re-use input strings for result.


Differential Revision: D49935894

amitkdutta force-pushed the export-D49935894 branch from 5bd6294 to 5b15d17 Compare

October 6, 2023 17:09

Contributor

facebook-github-bot commented Oct 6, 2023

This pull request was exported from Phabricator. Differential Revision: D49935894

amitkdutta pushed a commit to amitkdutta/velox that referenced this pull request


          Handle NaN in array_min/max functions (facebookincubator#6922)

baef804

Summary:
Also, re-use input strings for result.


Differential Revision: D49935894

amitkdutta force-pushed the export-D49935894 branch from 5b15d17 to baef804 Compare

October 6, 2023 17:11

Contributor

facebook-github-bot commented Oct 6, 2023

This pull request was exported from Phabricator. Differential Revision: D49935894

Contributor Author

amitkdutta commented Oct 6, 2023

Thanks for the quick fix. Could you please double check if other similar functions also have the same issue such as sum(), avg()?

Will look after this one.

amitkdutta pushed a commit to amitkdutta/velox that referenced this pull request


          Handle NaN in array_min/max functions (facebookincubator#6922)

9c32781

Summary:
Also, re-use input strings for result.


Differential Revision: D49935894

amitkdutta force-pushed the export-D49935894 branch from baef804 to 9c32781 Compare

October 6, 2023 20:01

Contributor

facebook-github-bot commented Oct 6, 2023

This pull request was exported from Phabricator. Differential Revision: D49935894


          Handle NaN in array_min/max functions (facebookincubator#6922)

4ced2ff

Summary: Pull Request resolved: facebookincubator#6922

Differential Revision: D49935894

amitkdutta force-pushed the export-D49935894 branch from 9c32781 to 4ced2ff Compare

October 6, 2023 20:41

Contributor

facebook-github-bot commented Oct 6, 2023

This pull request was exported from Phabricator. Differential Revision: D49935894

bikramSingh91 reviewed

View reviewed changes

velox/docs/functions/presto/array.rst

               .. function:: array_min(array(E)) -> E
-                  Returns the minimum value of input array. ::
+                  Returns minimum non-NULL element of the array. Returns NULL if array is empty or contains a NULL element.
+                  When E is DOUBLE or REAL, NaN value is considered greater than any non-NaN value. ::

Contributor

bikramSingh91 Oct 6, 2023

Returns NULL if array is empty or contains a NULL element.
NaN value is considered greater than any non-NaN value.

This doesn't seem to hold true completely.
NaN seems to always be returned in both min_by and max_by even if NULL exists which also means its simultaneously considered both highest and lowest value.
Tried these on presto java:

SELECT array_max(ARRAY [nan(), 1.0]); => NaN 
SELECT array_max(ARRAY [NULL, nan(), 1.0]); => NaN 

SELECT array_min(ARRAY [nan(), 1.0]); => NaN 
SELECT array_min(ARRAY [NULL, nan(), 1.0]); => NaN

Contributor Author

amitkdutta Oct 6, 2023

Good catch @bikramSingh91. In Presto, in presence of a NAN, the result is always NAN regardless of the other elements in the array for both min/max functions.

amitkdutta closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed fb-exported