-
Notifications
You must be signed in to change notification settings - Fork 70
feat: add row-based immutable data structure #181
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
Conversation
wgtmac
commented
Aug 16, 2025
- Add StructLike, MapLike, and ArrayLike interfaces
- Add wrapper for ManifestFile and ArrowArray
472f2b9 to
60b93f8
Compare
627093a to
d40c29f
Compare
|
@dongxiao1198 @mapleFU @zhjwpku @lidavidm Could you please take a look? |
| std::string_view, // for non-owned string, binary and fixed | ||
| std::shared_ptr<StructLike>, // for struct |
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.
It's a bit confusing, if we want ownership, it might be:
Scalar = variant<Literal, std::shared_ptr<StructLike>, ...>If we don't want ownership, it would be:
<...
std::string_view,
const StructLike*,
const ArrayLike*,
...
>
Mixing string_view and sptr is so weird...
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.
And scalar seems re-invent some member in literal?
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.
These structures are all views. I don't want any ownership for string/binary/fixed/uuid, so use std::string_view and do not reuse Literal. But we need ownership for nested struct/map/list views, so sptr is used.
| /// Wrapper classes for manifest-related data structures that implement | ||
| /// StructLike, ArrayLike, and MapLike interfaces for unified data access. | ||
|
|
||
| #include <functional> |
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.
Why this is included?
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.
Because std::reference_wrapper is used in the header file.
3f0ae8d to
c011303
Compare
| /// | ||
| /// Note that all string and binary values are stored as non-owned string_view | ||
| /// and their lifetime is managed by the wrapped object. | ||
| using Scalar = std::variant<std::monostate, // for null |
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.
Can Scalar be merged with Literal::Value?
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.
They are slightly different:
- scalar employs view-like semantics for var-length types but literal takes the ownership.
- scalar supports nested types but literal does not.
| double, // for double | ||
| std::string_view, // for non-owned string, binary and fixed | ||
| Decimal, // for decimal | ||
| std::shared_ptr<StructLike>, // for struct |
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.
In which case will a class except ArrowArrayStructLike inherit StructLike?
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.
Any object that might be evaluated against expression. I've provided implementation for ManifestFile and we may extend it for ManifestList, etc.
- Add StructLike, MapLike, and ArrayLike interfaces - Add wrapper for ManifestFile and ArrowArray
|
Thanks @wgtmac for working on this, and thanks @zhjwpku @HuaHuaY @HeartLinked and @mapleFU for reviewing this 🙌 |