Fix mapping of MAP types with boolean or integer keys #441
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hashhar
force-pushed
the
hashhar/fix-type-mappings
branch
from
6446f7f
to
c74f251
Compare
hovaesco
reviewed
Jan 23, 2024
Comment on lines
+40
to
+43
| if str(value).lower() == 'true': | ||
| return True | ||
| if str(value).lower() == 'false': | ||
| return False |
There was a problem hiding this comment.
is there a way to avoid this and return map keys as boolean instead?
There was a problem hiding this comment.
Technically yes, but it's much harder to do on server. There's already some code FixJsonDataUtils which tries to handle some of this but it doesn't work well or is easy to do for structural types.
Comment on lines
873
to
880
| # structural types - note that none of these below tests work in the Trino JDBC Driver either. | ||
| # Unhashable types like lists and dicts cannot be used as keys so these values cannot be represented as Python | ||
| # objects at all. | ||
| # .add_field(sql="MAP(ARRAY[ARRAY[1, 2]], ARRAY[null])", python={[1, 2]: None}) | ||
| # .add_field(sql="MAP(ARRAY[MAP(ARRAY[1], ARRAY[2])], ARRAY[null])", python={{1: 2}: None}) | ||
|
|
||
| # TODO: fails because server sends [[{"[1, 2]":null}]] as response whereas it sends [[[1,2]]] as response for ROW | ||
| # types that are not enclosed in a MAP while the RowValueMapper expects values to be lists. | ||
| # .add_field(sql="MAP(ARRAY[ROW(1, 2)], ARRAY[CAST(null AS VARCHAR)])", python={(1, 2): None}) |
ebyhr
approved these changes
Jan 24, 2024
1 task
This uncovers a few bugs in the handling of map with boolean, integer or row keys which will be fixed in a future commit.
hashhar
force-pushed
the
hashhar/fix-type-mappings
branch
from
c74f251
to
4706b05
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fix mapping of MAP types with boolean or integer keys.
This is on top of #440 so only last 3 commits need to be reviewed.
Release notes
(x) Release notes are required, with the following suggested text:
* Fix mapping of MAP types with boolean or integer keys.