-
Notifications
You must be signed in to change notification settings - Fork 173
Fix Nested Array #171
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
Merged
Merged
Fix Nested Array #171
Conversation
This file contains hidden or 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
Enmk
commented
Apr 28, 2022
Enmk
commented
Apr 28, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need to go over some details and the tests. Submeting partial review so we can work in parallel.
4fa4c9e
to
17ddbdb
Compare
Known limitations: - Doesn't work with nested arrays.
More tests Stub for wrapping ColumnArray into ColumnArrayT
Added new and updated existing tests
utils.h(214,1): error C2872: 'details': ambiguous symbol
Using it at appropriate places instead of Slice(0, 0), since later may cause issues in some context (like empty arrays)
Even more tests
Array(Array(Array(UInt64)))
Added tests for wrapping 2D Array
- Fixed ColumnArray::Slice - Added tests for ColumnArray::GetAsColumn, ColumnArray::Slice - Fixed construction of ColumnArrayT, and updated ColumnArrayT::Wrap - Renamed and rearranged some other Array-related tests
17ddbdb
to
6708b82
Compare
arthurpassos
approved these changes
May 4, 2022
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.
Fixes
ColumnArray::Slice()
andColumnArray::GetAsColumn()
that worked poorly with 2D-arrays (Array(Array(X))
)Previous implementation was using
Append
, rather thanAppendAsColumn
, flattening nested arrays.New APIs
Column::CloneEmpty
- idiomatic way to make an empty clone of a column instead ofc->Slice(0, 0)
ColumnArray::GetAsColumnTyped<T>
- syntax sugar to simplify accessing array values.ColumnArray<T>
template that simplifies and optimizes both reading and appending data toArray
.ColumnArrayT<X>::operator[]
andColumnArrayT<X>::At()
provide a wrapper rather than a full-blown column. So the underlying column is not copied, but rather accessed via read-only "view".ColumnArrayT<X>::Append()
overloads work for anything iteratable. Users don't have to wrap values intoColumnX
anymore.ColumnArrayT<X>::operator[]
andColumnArrayT<X>::At()
provide type-safe access to the typed data.ColumnArray
instance can be type-safe wrapped into appropriateColumnArrayT<NestedColumnType>
so users can benefit from features above for columns obtained fromBlock
.Examples
Array(UInt64)
:Array(FixedString(6))
:Array(Array(UInt64))
See unit-tests for more examples.
Closes: #43