Unexpected results from map_normalized about presto

[rschlussel]

I don't know what the behavior is supposed to be for all these cases (how do you scale -1 and 1 so that they sum to 1?). But the variety of behaviors is a bug and some of these clearly should be summing to 1. e.g. the second example should be returning {1=null, 2=1} and the third should be returning `{1=0.5, 2=0.5}.

The function is definitely lacking tests for any of these examples. https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/operator/scalar/sql/MapNormalizeFunction.java is the function and https://github.com/prestodb/presto/blob/master/presto-main/src/test/java/com/facebook/presto/operator/scalar/sql/TestMapNormalize.java has the tests.

[rschlussel]

(I'll add that this is the first time i've seen this function, so my opinion isn't based on any special knowledge).

[steveburnett]

Thanks for the tag @mbasmanova! Following for potential doc fix PR material.

[kaikalur]

My main goals here are a) let's not unnecessarily implement it natively in prestissimo and b) to show how to pull out nested lambdas that can be outside the loop.

[mbasmanova]

I'm going to match these semantics in Velox and document them clearly: facebookincubator/velox#9086

[kaikalur]

(I'll add that this is the first time i've seen this function, so my opinion isn't based on any special knowledge).

There was an internal Spark UDF with exact same semantics and someone coded it up in presto. I see it used quite a bit though here in prod.

[tdcmeehan]

@kaikalur given that this was developed to match some sort of internal Spark UDF at Meta and it has strange semantics, does it make sense to deprecate this function (we can think of a sensible deprecation strategy)? It could be ported to an internal UDF for Meta?

[kaikalur]

@kaikalur given that this was developed to match some sort of internal Spark UDF at Meta and it has strange semantics, does it make sense to deprecate this function (we can think of a sensible deprecation strategy)? It could be ported to an internal UDF for Meta?

I'm ok with making it meta only! It's a rather strange UDF

[tdcmeehan]

OK. In terms of deprecation, I propose we introduce a feature toggle for this function, and over the course of a few releases change the default from true to false. We can provide a warning in the release notes (linking to a new issue) indicating we are beginning to deprecate the function. Once it's disabled for a few releases we can delete it with final announcement in the release notes.