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

Return err instead of panic for invalid map keys #74

Merged
merged 1 commit into from
Feb 19, 2023

Conversation

begelundmuller
Copy link
Contributor

It seems DuckDB supports more map key types than Go, causing the assignment out[key] = val to panic. This PR adds a check that map key types are comparable and returns an error if not.

An alternative would be to not scan DuckDB maps into Go map, but instead directly return the list object containing {"key": ..., "value": ...} maps. What do you prefer?

@marcboeker marcboeker merged commit fdeb27f into marcboeker:master Feb 19, 2023
@marcboeker
Copy link
Owner

Good catch, thanks for the fix! I suggest we keep it as it is at the moment and if there is demand for non comparable map keys we can switch to the list approach.

@begelundmuller
Copy link
Contributor Author

Sounds good – thanks for the fast review!

@begelundmuller
Copy link
Contributor Author

begelundmuller commented Feb 22, 2023

Just adding a note that I discovered that the Python driver takes the {"key": ..., "value": ...} approach:

>>> duckdb.sql("SELECT map(['B', 'A'], [map([1, 2], [3,4]), map([1, 2], [3,4])]);").fetchall()
[({'key': ['B', 'A'], 'value': [{'key': [1, 2], 'value': [3, 4]}, {'key': [1, 2], 'value': [3, 4]}]},)]

Might be worth doing at some point to support all cases, even though it has worse ergonomics.

@marcboeker
Copy link
Owner

This looks so weird to me, but yeah it would make sense to be more flexible. Shouldn't be that complicated to implement though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants