Conversation
evindj
force-pushed
the
unbound_term_serde
branch
from
602bef1 to
684d85c
Compare
src/iceberg/expression/json_serde.cc
Outdated
|
|
||
| Result<std::shared_ptr<NamedReference>> NamedReferenceFromJson( | ||
| const nlohmann::json& json) { | ||
| if (!json.is_string()) { |
src/iceberg/expression/json_serde.cc
Outdated
| * under the License. | ||
| */ | ||
|
|
||
| #include <cctype> |
| nlohmann::json ToJson(const UnboundTransform& transform) { | ||
| auto& mutable_transform = const_cast<UnboundTransform&>(transform); | ||
| nlohmann::json json; | ||
| json[kType] = std::string(kTransform); |
There was a problem hiding this comment.
| json[kType] = std::string(kTransform); | |
| json[kType] = kTransform; |
I don't think the string ctor around kTransform is necessary. The same applies to other places as well.
There was a problem hiding this comment.
windows does not seem to like it when we don't do the explicit copy.
There was a problem hiding this comment.
Oh, I'm not entirely sure, but I noticed a similar usage here: https://github.com/apache/iceberg-cpp/blob/main/src/iceberg/json_serde.cc#L1295
evindj
force-pushed
the
unbound_term_serde
branch
from
684d85c to
8093c92
Compare
src/iceberg/expression/json_serde.cc
Outdated
|
|
||
| Result<std::shared_ptr<UnboundTransform>> UnboundTransformFromJson( | ||
| const nlohmann::json& json) { | ||
| if (json.is_object() && json.contains(kType) && json[kType] == kTransform && |
There was a problem hiding this comment.
From the build error msg, it seems windows don't like json[kType] == kTransform, you might change this back.
src/iceberg/expression/json_serde.cc
Outdated
| nlohmann::json json; | ||
| json[kType] = kTransform; | ||
| json[kTransform] = transform.transform()->ToString(); | ||
| json[kTerm] = std::string(mutable_transform.reference()->name()); |
There was a problem hiding this comment.
| json[kTerm] = std::string(mutable_transform.reference()->name()); | |
| json[kTerm] = mutable_transform.reference()->name(); |
line 146 seems ok, so the same apply to line 148?
There was a problem hiding this comment.
I donl think like 146 is ok, after some research, I think what is happening here is the following: MSVC is having hard times deciding what overload for operators to use between
nlohmann::json::operator== (from nlohmann_json library)
std::operator==<char, std::char_traits> for string_view (from MSVC's STL)
a similar confusion is happening for constructors. I can push a another diff to confirm, I will push another version to confirm
There was a problem hiding this comment.
Good to know. It seems that Compiler Explorer doesn't currently support libraries with MSVC[1], so we have to rely on the CI to confirm that.
evindj
force-pushed
the
unbound_term_serde
branch
2 times, most recently
from
2c7a523 to
6e6031b
Compare
evindj
force-pushed
the
unbound_term_serde
branch
from
6e6031b to
f7a9960
Compare
2 participants
This is the second PR in addressing serde for expression for predicate push down. It focuses on unbound transforms and and named references.
Added unit tests to ensure that serialization happens correctly. while name references will map just names like schema columns, unbound transforms will make sure take care of transform serde. for example it will ensure
{ "type": "transform", "transform": "bucket[16]", "term": "id" }is converted to an object and vice versa.