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

Fix serialization for slices of OrderedMap/IterableOrderedMap (#1365) #1418

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

earwin
Copy link
Contributor

@earwin earwin commented Oct 9, 2024

Summary

Array column handling is special-cased for nested Array(Array(Array(Whatever))) cases.
If you want to traverse nested pointers to slices or slices cast to any, you have to special case as well, as was done in #1350.
This broke the inserts for slices of IterableOrderedMap, as the new code unwrapped one pointer too much.
This PR fixes #1350&#1365 by moving interface/pointer unwrapping to the beginning of the nesting loop.

Checklist

  • Unit and integration tests covering the common scenarios were added

@earwin
Copy link
Contributor Author

earwin commented Oct 9, 2024

Slightly more idiomatic version

Comment on lines +57 to +60
//var readMaps []*T1365OrderedMap
//
//err = rows.Scan(&readMaps)
//require.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you feel this is not required, let's remove it.

Suggested change
//var readMaps []*T1365OrderedMap
//
//err = rows.Scan(&readMaps)
//require.NoError(t, err)

Copy link
Contributor

@jkaflik jkaflik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@earwin much appreciated! Well done.

Would be great to remove commented lines. PTAL if you have a time.

@jkaflik jkaflik merged commit 28b2f5d into ClickHouse:main Oct 15, 2024
16 checks passed
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.

2 participants