Fix map_top_n to use keys to break ties on NULL values. by kgpai · Pull Request #22979 · prestodb/presto

kgpai · 2024-06-10T22:01:19Z

Description

Fix map_top_n to use keys to break ties for NULL values.

Motivation and Context

Map_top_n currently uses keys to break ties when values match. This is however not done when values are null , which leads to some non determinism when compared agains the presto_native implementation.

fixes #22778

Impact

Since current behavior is non deterministic, this has no impact on existing queries.

Test Plan

See attached unit test.

Release Notes

== RELEASE NOTES ==
General Changes
* Make map_top_n function more deterministic by using keys to break ties.

elharo · 2024-06-11T11:42:55Z

presto-main/src/test/java/com/facebook/presto/operator/scalar/sql/TestMapTopNFunction.java

    {
        assertFunction("MAP_TOP_N(NULL, 1)", mapType(UNKNOWN, UNKNOWN), null);
+
+        // If values are null , then use keys to break ties.


no space before comma

elharo

"Since current behavior is non deterministic, this has no impact on existing queries."

Is it truly random, or is it possibly deterministic for some users in a way they might be depending on? I do think this one should be called out in the release notes.

rschlussel · 2024-06-11T15:19:16Z

Please update the documentation as well.

kgpai · 2024-06-13T17:36:39Z

@elharo Current behavior would depend on order of values in the valueBlock - I presume this means its dependent on order of input, and thus might be non deterministic. I will update and call out the present change to make it deterministic in the documentation.

kgpai · 2024-06-13T17:55:48Z

cc: @elharo @rschlussel Please have a look when possible. Thanks !

rschlussel · 2024-06-14T13:23:07Z

presto-docs/src/main/sphinx/functions/map.rst

 .. function:: map_top_n_keys(x(K,V), n) -> array(K)

-    Returns top ``n`` keys in the map ``x`` by sorting its keys in descending order.
+    Returns top ``n`` keys in the map ``x`` by sorting its keys in descending order. Keys are used to break ties with the max key being chosen. Both keys and values should be orderable.


I think you updated the wrong documentation. This should be at map_top_n (a few entries below), not map_top_n_keys

Apologies and Thank you @rschlussel . Fixed.

steveburnett

Thanks for the doc update! Two small nits, nothing big.

steveburnett · 2024-06-18T14:53:37Z

presto-docs/src/main/sphinx/functions/map.rst

 .. function:: map_top_n(x(K,V), n) -> map(K, V)

-    Truncates map items. Keeps only the top N elements by value.
+    Truncates map items. Keeps only the top N elements by value.  Keys are used to break ties with the max key being chosen. Both keys and values should be orderable.


Suggested change

Truncates map items. Keeps only the top N elements by value. Keys are used to break ties with the max key being chosen. Both keys and values should be orderable.

Truncates map items. Keeps only the top ``n`` elements by value. Keys are used to break ties with the max key being chosen. Both keys and values should be orderable.

Nit, formatting.

steveburnett · 2024-06-18T14:58:34Z

presto-docs/src/main/sphinx/functions/map.rst


-    Truncates map items. Keeps only the top N elements by value.
+    Truncates map items. Keeps only the top N elements by value.  Keys are used to break ties with the max key being chosen. Both keys and values should be orderable.
    ``n`` must be a non-negative integer.::


Suggested change

``n`` must be a non-negative integer.::

``n`` must be a non-negative integer. ::

Nit: no space between the . and the first : results in "a non-negative integer.: " as shown in this screenshot

kgpai · 2024-06-18T20:30:41Z

@steveburnett Thank you, made the changes you requested.

steveburnett

One final nit in formatting, nothing else!

presto-docs/src/main/sphinx/functions/map.rst

kgpai · 2024-06-20T23:07:49Z

@steveburnett Thank you again ! Made changes as per your request.

steveburnett

LGTM! (docs)

Pull updated branch, reviewed new local docs build, looks good. Thanks!

rschlussel · 2024-06-21T15:43:15Z

@elharo can you review please?

elharo · 2024-06-21T15:58:59Z

presto-main/src/main/java/com/facebook/presto/operator/scalar/sql/MapSqlFunctions.java

    public static String mapTopN()
    {
-        return "RETURN IF(n < 0, fail('n must be greater than or equal to 0'), map_from_entries(slice(array_sort(map_entries(map_filter(input, (k, v) -> v is not null)), (x, y) -> IF(x[2] < y[2], 1, IF(x[2] = y[2], IF(x[1] < y[1], 1, -1), -1))) || map_entries(map_filter(input, (k, v) -> v is null)), 1, n)))";
+        return "RETURN IF(n < 0, fail('n must be greater than or equal to 0'), map_from_entries(slice(array_sort(map_entries(map_filter(input, (k, v) -> v is not null)), (x, y) -> IF(x[2] < y[2], 1, IF(x[2] = y[2], IF(x[1] < y[1], 1, -1), -1))) "


Not required but if there's any plausible way to use indentation and line breaks here to make this code easier to read, that would help a lot. Without that, it's very hard to follow the logic.

kgpai requested a review from a team as a code owner June 10, 2024 22:01

kgpai requested review from presto-oss and rschlussel June 10, 2024 22:01

elharo requested changes Jun 11, 2024

View reviewed changes

kgpai force-pushed the map_top_n_fix branch from 48560a8 to 13c694f Compare June 13, 2024 17:55

kgpai requested a review from steveburnett as a code owner June 13, 2024 17:55

rschlussel reviewed Jun 14, 2024

View reviewed changes

kgpai force-pushed the map_top_n_fix branch from 13c694f to b5adb06 Compare June 17, 2024 23:24

steveburnett requested changes Jun 18, 2024

View reviewed changes

kgpai force-pushed the map_top_n_fix branch from b5adb06 to d6e184a Compare June 18, 2024 20:24

steveburnett requested changes Jun 18, 2024

View reviewed changes

presto-docs/src/main/sphinx/functions/map.rst Outdated Show resolved Hide resolved

Fix map_top_n to use keys to break ties on NULL values. prestodb#22778

e4e3f47

kgpai force-pushed the map_top_n_fix branch from ff4e305 to e4e3f47 Compare June 20, 2024 23:50

rschlussel approved these changes Jun 21, 2024

View reviewed changes

steveburnett approved these changes Jun 21, 2024

View reviewed changes

elharo approved these changes Jun 21, 2024

View reviewed changes

kgpai merged commit 6a0b194 into prestodb:master Jul 8, 2024

tdcmeehan mentioned this pull request Aug 23, 2024

Add release notes for 0.289 #23513

Merged

34 tasks

	Truncates map items. Keeps only the top N elements by value. Keys are used to break ties with the max key being chosen. Both keys and values should be orderable.
	Truncates map items. Keeps only the top ``n`` elements by value. Keys are used to break ties with the max key being chosen. Both keys and values should be orderable.

	``n`` must be a non-negative integer.::
	``n`` must be a non-negative integer. ::

Conversation

kgpai commented Jun 10, 2024 • edited by rschlussel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Release Notes

Uh oh!

elharo Jun 11, 2024

Choose a reason for hiding this comment

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

rschlussel commented Jun 11, 2024

Uh oh!

kgpai commented Jun 13, 2024

Uh oh!

kgpai commented Jun 13, 2024

Uh oh!

rschlussel Jun 14, 2024

Choose a reason for hiding this comment

Uh oh!

kgpai Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

steveburnett Jun 18, 2024

Choose a reason for hiding this comment

Uh oh!

steveburnett Jun 18, 2024

Choose a reason for hiding this comment

Uh oh!

kgpai commented Jun 18, 2024

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kgpai commented Jun 20, 2024

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

rschlussel commented Jun 21, 2024

Uh oh!

elharo Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kgpai commented Jun 10, 2024 •

edited by rschlussel

Loading