Skip to content
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.

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

[Map] Rework MAP creation method behavior when input is NULL #11730

Merged
merged 9 commits into from
Apr 20, 2024

Conversation

Tishj
Copy link
Contributor

@Tishj Tishj commented Apr 19, 2024

This PR fixes #11115

Old behavior:

MAP(NULL, [1,2,3]) -> Error: Only LIST types are accepted arguments
MAP([1,2,3], NULL) -> Error: Only LIST types are accepted arguments
MAP(NULL, NULL) -> {} (empty map)
MAP(NULL::INT[], [1,2,3]) -> Error: Key list can not be NULL
MAP([1,2,3], NULL::INT[]) -> Error: Value list can not be NULL
MAP(NULL::INT[], NULL::INT[]) -> Error: Key list can not be NULL

New behavior:

MAP(NULL, [1,2,3]) -> NULL
MAP([1,2,3], NULL) -> NULL
MAP(NULL, NULL) -> NULL
MAP(NULL::INT[], [1,2,3]) -> NULL
MAP([1,2,3], NULL::INT[]) -> NULL
MAP(NULL::INT[], NULL::INT[]) -> NULL

@duckdb-draftbot duckdb-draftbot marked this pull request as draft April 19, 2024 08:27
@Tishj Tishj marked this pull request as ready for review April 19, 2024 08:52
@duckdb-draftbot duckdb-draftbot marked this pull request as draft April 19, 2024 09:15
@Tishj
Copy link
Contributor Author

Tishj commented Apr 19, 2024

It seems in the pandas analyzer we already didn't use this NULL_KEY_LIST invalid-reason because it got converted to a STRUCT instead

    def test_map_nullkeylist(self, pandas, duckdb_cursor):
        x = pandas.DataFrame([[{'key': None, 'value': None}]])
        # Isn't actually converted to MAP because isinstance(None, list) != True
        converted_col = duckdb_cursor.sql("select * from x").df()
        duckdb_col = duckdb_cursor.sql("SELECT {key: NULL, value: NULL} as '0'").df()
        pandas.testing.assert_frame_equal(duckdb_col, converted_col)

Perhaps we want to change this to also created a NULL MAP
Currently the requirements to have a dict be seen as a MAP is:

  • there have to be two items
  • they have to be named 'key' and 'value' respectively
  • the type of the value of each of those items need to be list

This would relax the last requirement to be:

  • the value of each of those items have to be either None or a list

@Tishj Tishj marked this pull request as ready for review April 19, 2024 12:02
@duckdb-draftbot duckdb-draftbot marked this pull request as draft April 19, 2024 14:12
@Tishj Tishj marked this pull request as ready for review April 19, 2024 17:14
@NickCrews
Copy link

What should TYPEOF(MAP_KEYS(MAP(NULL, [1,2,3]))) be?

@Tishj
Copy link
Contributor Author

Tishj commented Apr 19, 2024

What should TYPEOF(MAP_KEYS(MAP(NULL, [1,2,3]))) be?

Different PR, I'll push that when this is merged as it depends on this

@Tishj Tishj requested a review from Mytherin April 20, 2024 06:57
@Mytherin Mytherin merged commit bfb8f48 into duckdb:main Apr 20, 2024
49 checks passed
@Mytherin
Copy link
Collaborator

Thanks! LGTM

github-actions bot pushed a commit to duckdb/duckdb-r that referenced this pull request May 5, 2024
Merge pull request duckdb/duckdb#11730 from Tishj/map_null_behavior_rework
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maps can't handle NULL keys or values
3 participants