-
Notifications
You must be signed in to change notification settings - Fork 55
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
base: main
Are you sure you want to change the base?
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