Make map and array functions use IS DISTINCT semantics as appropriate

[jirassimok]

This addresses #560.

Release note:

# General
* Compare with "is distinct" semantics in `array_except`, `array_union`, `map_concat`, `map_from_entries`, `multimap_from_entries`, and `multimap_agg`. ({issue}`560`)

[jirassimok]

I've split the refactor commits into #9901; only the last four commits here are part of this PR.

(The failing test is related to that PR, not this one.)

[findepi]

I've split the refactor commits into #9901; only the last four commits here are part of this PR.

thanks

[jirassimok]

I'm not actually sure we should make this change for some of the Map functions (unless we also define Maps as using distinctness instead of equality for key lookups, which I don't think is an especially good idea).

Alternatively, we could make the map functions check for not-equal-but-not-distinct values and fail on them, which I think might be better (though it would make the functions a bit slower to maintain a second TypedSet for that).

(Right now, the only not-equal-but-not-distinct value I know of is NaN, and floating-point types shouldn't really be used as map keys anyway because they don't do equality comparison well.)

[findepi]

rebased after #9905 merged.

[jirassimok]

@martint @dain What do you think?

[dain]

This looks directionally correct and nicely simple :)

[jirassimok]

Rebased and added a test for multimap_agg.

There still is no test for MapToMapCast because I can't think of a good way to test it—I don't know of any values that change distinctness when cast to another type.

[findepi]

I don't know of any values that change distinctness when cast to another type.

double to long ( input 1.1 and 1.2) or vice versa (input: certain consecutive values over 2^53)

[jirassimok]

Casting to duplicate keys is already tested, so I guess I don't need to add a new test for that here.

[jirassimok]

Rebased onto master.

[jirassimok]

@martint Look good to you?

[joshthoward]

@jirassimok I had an offline discussion with @dain and he said that while he thought it looked directionally correct he wouldn't have time to complete a full review.

@findepi or @losipiuk could either of you provide a review to close this issue?

CC: @martint

[losipiuk]

You can somewhat test it with:

select cast(map(array['NaN', 'NaN '], array[1, 2]) as map<double, bigint>);

It will fail now. It used to not.

[jirassimok]

Added a test.

[losipiuk]

Code looks good for me. I would love to learn more about motivation. It is not obvious to me what semantics should be actually, hence why is this a fix for "correctness" issue.
Also: #560 (comment)

@findepi please comment.

[findepi]

Code looks good for me. I would love to learn more about motivation. It is not obvious to me what semantics should be actually, hence why is this a fix for "correctness" issue. Also: #560 (comment)

@findepi please comment.

that's an old conversion... this is @dain 's comment prestodb/presto#11979 (comment)
couple other references you can find @martint involvement there as well
prestodb/presto#11963 prestodb/presto#11978 prestodb/presto#11979 prestodb/presto#11977 prestodb/presto#12080

[jirassimok]

Rebased onto master.

[losipiuk]

please do rebase

[jirassimok]

@dain @losipiuk I believe this is ready to merge.

[losipiuk]

@dain @losipiuk I believe this is ready to merge.

Yeah I think. @jirassimok can you update PR description to use tample (specifically putting release notes entry there) :)

[jirassimok]

Added release note to PR description.

[losipiuk]

rebased