map_extract should actually not return a LIST, the returned list is either empty or of size 1, which is kind of nonsense.
I'm not sure if we want to make that change though (for backwards compatibility reasons)

Creating map_extract_first would kind of cement this into place, making it even harder to change the behavior of map_extract in the future imo

@Tishj I thought the point of map_extract returning a list was to differentiate between a value being NULL and not present at all. Although I agree that it does add a lot of friction, now that we have map_contains and IN support you can check for missing values separately.

@Tishj I thought the point of map_extract returning a list was to differentiate between a value being NULL and not present at all. Although I agree that it does add a lot of friction, now that we have map_contains and IN support you can check for missing values separately.

Hmm that makes some sense, I hadn't thought of that

Also, I'm not sure if it has an impact here, but there is a difference between struct extract and map extract in case-insensitivity

struct keys are case-insensitive, map keys aren't, since they're regular varchars, not identifiers, they are case-sensitive

Json extract keys are also case sensitive, but we allow dot notation for that too, so this is consistent with that, at least.

Also, we also cannot differentiate between list_extract returning NULL due to the index being out-of-range, or a NULL being returned from the specified position in the list. We could extend this logic to map_extract, and conclude that map_extract shouldn't return a list for the sole reason to be able to differentiate. I'm pretty sure that, e.g., Presto also doesn't return a list, but just a scalar value. Maybe we should just change map_extract then instead of adding this function?

Yea I agree, but like I said that would break backwards compatibility, so it's a question if we want to do that.
Right now we're in between, adding map_extract_first would move us much further in the direction of having this map_extract behavior be permanent I feel.

I think either Hannes or Mark should make this call, I'd be in favor of having map_extract return a scalar

Thanks for the PR!

I think the reason map_extract returns a list is because maps in their original implementation were actually multimaps that could hold duplicate keys. That is no longer the case since #3760 - as such I would argue map_extract returning a list is no longer required/a good idea either.

I would be in favor of changing map_extract - but we need to be a bit careful w.r.t. backwards compatibility here.

In any case we should not be modifying the signature of map_extract in a bug-fix release.

Thanks for the discussion and feedback. I've removed map_extract_first, made map_extract now return a scalar value, and fixed related tests.

I've added the "Needs documentation" label as this PR now changes the behavior of map_extract.

Thanks! LGTM - we just need to make sure to mention this change in behavior in the release notes