feat: implement all functions of bound predicates #280
Conversation
wgtmac
changed the title
- Implemented Negate and ToString functions for bound predicates. - Added hash support to Literal. - Refactored BoundSetPredicate to use unordered set for literals. - Refactored predicate unit test to be better organized.
wgtmac
changed the title
|
LGTM |
| BoundPredicate::~BoundPredicate() = default; | ||
|
|
||
| Result<Literal::Value> BoundPredicate::Evaluate(const StructLike& data) const { | ||
| Result<Literal> BoundPredicate::Evaluate(const StructLike& data) const { |
There was a problem hiding this comment.
Not sure if we have a tool to capture the API signature changes in release notes.
There was a problem hiding this comment.
We don't have that yet, which seems fine at this early stage of iceberg-cpp, but we should follow semantic versioning once the project becomes stable, i.e. bump up major version for incompatible API changes.
| } else if constexpr (std::is_same_v<T, std::vector<uint8_t>>) { | ||
| std::size_t hash = 0; | ||
| for (size_t i = 0; i < v.size(); ++i) { | ||
| hash ^= std::hash<uint8_t>{}(v[i]) + kHashPrime + (hash << 6) + (hash >> 2); |
There was a problem hiding this comment.
Hashing identical bytes at different positions can produce same hash (e.g., [1,2] and [2,1] might collide).
Can we add position i into the hash value to decrease the possibility of collide?
There was a problem hiding this comment.
Hashing identical bytes at different positions can produce same hash (e.g., [1,2] and [2,1] might collide).
I don't think this statement is correct, the left and right shifts should ensure a difference.
I ran a quick demo on Godbolt [1], and as you can see, the hashes of [1,2] and [2,1] are different.
There was a problem hiding this comment.
I traced through the logic again and realized you are right. The hash is position-sensitive because each iteration depends on the accumulated hash state from previous iterations.
|
|
||
| Result<bool> BoundLiteralPredicate::Test(const Literal::Value& value) const { | ||
| return NotImplemented("BoundLiteralPredicate::Test not implemented"); | ||
| Result<bool> BoundLiteralPredicate::Test(const Literal& value) const { |
There was a problem hiding this comment.
Can we add null checking?
if (value.IsNull() || literal_.IsNull()) {
return false;
}
There was a problem hiding this comment.
Comparing nulls returns std::partial_ordering::unordered so it is automatically false: https://github.com/apache/iceberg-cpp/blob/main/src/iceberg/expression/literal.cc#L358
|
Let me merge this. Thanks @zhjwpku @HuaHuaY @yingcai-cy @Fokko @shangxinli for the review! |
6 participants
Negate,ToStringandTestfunctions for bound predicate subclasses.Literal.BoundSetPredicateto use unordered set for literals.