feat: sort field/order json serialize/deserialization #64
Conversation
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
Generally LGTM.
@lidavidm @zhjwpku @yingcai-cy WDYT?
gty404
force-pushed
the
to_json_sort_order
branch
from
97a2b14 to
4ab5529
Compare
| kDescending, | ||
| }; | ||
| /// \brief Get the relative sort direction name | ||
| ICEBERG_EXPORT constexpr std::string_view SortDirectionToString(SortDirection direction) { |
There was a problem hiding this comment.
nit: we should probably only declare these functions here and put all implementation in the .cc file.
| auto json_value = std::move(_tmp_##json_value.value()); | ||
| } // namespace | ||
|
|
||
| nlohmann::json ToJson(const SortField& sort_field) { |
There was a problem hiding this comment.
I was thinking about adding ToJson and FromJson member functions to each Spec classes, but your way might be better, users don't need to touch the json ser/der functions, right?
There was a problem hiding this comment.
Yes, there is no need to introduce json either.
There was a problem hiding this comment.
Yes, in this way we don't need to expose our internal json dependency.
| .message = "IdentityTransformFunction::Transform"}); | ||
| } | ||
|
|
||
| expected<std::unique_ptr<TransformFunction>, Error> TransformFunctionFromString( |
There was a problem hiding this comment.
Not related, but should we do something like define iceberg::Result<T> = expected<T, iceberg::Error>?
| } | ||
| } | ||
|
|
||
| #define TRY_ASSIGN(json_value, expr) \ |
There was a problem hiding this comment.
We'll probably want to make a broader version of this macro. (Not necessarily in this PR)
FWIW, you could steal the Arrow version, which uses a counter to autogenerate the name, so it can be used like ARROW_ASSIGN_OR_RAISE(auto value, ...) or ARROW_ASSIGN_OR_RAISE(foo.bar, ...) too
There was a problem hiding this comment.
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
Thank you for working on this! There is no other concern from the community, I'm going to merge this one now.
|
Thank you @Xuanwo! |
5 participants
No description provided.