feat: add predicate module with Literal, LeafPredicate, and CompoundPredicate - #27
Conversation
…redicate Migrate the core predicate framework from the Alibaba repository: Public headers (include/paimon/predicate/): - predicate.h: base Predicate interface - leaf_predicate.h: leaf predicate for single-field conditions - compound_predicate.h: And/Or compound predicates - function.h: LeafFunction/CompoundFunction base - function_visitor.h: visitor pattern for predicate functions - literal.h: type-erased Literal value holder Internal implementation (src/paimon/common/predicate/): - Function hierarchy: leaf_function, leaf_unary_function, null_false_leaf_binary_function, string_leaf_binary_function, multi_literals_leaf_function, compound_function - Predicate impls: leaf_predicate_impl, compound_predicate_impl - predicate_filter.h: row filtering via predicates - literal.cpp: Literal construction/comparison/serialization - literal_converter.h/.cpp: Arrow-to-Literal type conversion - leaf_predicate.cpp, compound_predicate.cpp Tests: - literal_test.cpp: Literal value construction and comparison - literal_converter_test.cpp: Arrow array to Literal conversion Migrate-by: Aone Copilot (Claude)
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Thanks for the PR. I think this needs changes before it can be merged into main.
The new predicate sources currently depend on several project headers/classes that are not present in this PR or in the PR base branch, so the PR cannot compile as a standalone change against main. For example:
src/paimon/common/predicate/literal.cppincludespaimon/common/utils/field_type_utils.handpaimon/common/utils/fields_comparator.h.src/paimon/common/predicate/literal_converter.cppincludespaimon/common/utils/date_time_utils.h,paimon/common/utils/field_type_utils.h, andpaimon/common/utils/string_utils.h.src/paimon/common/predicate/literal_converter_test.cppincludespaimon/common/data/binary_row.h,paimon/common/utils/decimal_utils.h, andpaimon/testing/utils/binary_row_generator.h.src/paimon/common/predicate/literal_test.cppincludespaimon/common/utils/decimal_utils.h.
Please either add/migrate these missing dependencies in this PR, or retarget/rebase this PR on the dependency PR/branch that introduces them. After the PR becomes buildable on its target branch, I can continue with a deeper review of the predicate logic.
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Thanks for the clarification. I re-reviewed the current predicate code while not treating the missing dependency files as a blocker in this pass. I still think this needs changes before merge because of the implementation issues below.
-
Literalequality is inconsistent withstd::hash<Literal>/HashCode().
operator==treatsFLOAT/DOUBLEvalues as equal with an absolute epsilon, whileHashCode()hashes the exact floating-point value. Also, decimal equality delegates toDecimal::CompareTo(), so decimals with different scales can compare equal numerically, butHashCode()hashes the unscaled bits and scale. This violates the requirement that equal keys have equal hashes and can breakstd::unordered_*users of the providedstd::hash<Literal>specialization. Please either make equality exact for these types or normalize the hash to match the equality semantics. -
LiteralConverter::GetLiteralFromDecimalArray()reconstructs negative decimal values by left-shiftingdecimal.high_bits()after casting it to signed__int128_t. Left-shifting a negative signed value is undefined behavior in C++, so negative Decimal128 values can hit UB. Please compose the 128-bit value through an unsigned type first and cast toDecimal::int128_tafter the bits are assembled. -
NullFalseLeafBinaryFunctiononly rejectsliterals.size() < 1, then silently ignores all literals afterliterals[0]. Binary predicates such as equal/less-than/starts-with should require exactly one literal; otherwise malformed predicates with extra literals evaluate against the first literal and can return incorrect filter results. Please change these checks to requireliterals.size() == 1. -
LeafPredicateImpldoes not validate negative field indexes, and the stats overload does not validate the index againstmin_values,max_values, ornull_countsbefore accessing them. A negativefield_index_passes the current upper-bound checks, and out-of-range stats indexes can be read directly. Please validatefield_index_ >= 0and the relevant field counts before all accesses, ideally at construction or consistently in eachTest()overload.
Superseded by a corrected re-review after the latest PR update.
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the latest PR head and agree that the previous comments about extra literal count and defensive field-index checks should not block this PR. I also verified that the Decimal128 reconstruction issue has been fixed.
There is still one correctness issue left in the Literal equality/hash contract:
Literal::operator== now delegates to CompareTo(). For decimals, Decimal::CompareTo() treats numerically equal values with different scales as equal, for example Decimal(23, 3, 100000) and Decimal(23, 0, 100). However, Literal::HashCode() still hashes the raw decimal high bits, low bits, and scale, so those two equal Literals can produce different hashes. This violates the requirement for std::hash<Literal> that equal keys have equal hash values and can break std::unordered_* lookups.
Please either make decimal equality match the raw hash semantics, or normalize decimal hashing so it is consistent with Decimal::CompareTo() equality.
Superseded after re-review: the fixed-schema decimal scale invariant is now documented and accepted.
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Thanks for the clarification and the added comment. Given the current Paimon usage invariant that decimal values stored in the same std::unordered_map<Literal, ...> come from the same column and therefore have fixed precision/scale, I agree the mixed-scale decimal hash/equality mismatch does not affect the current code path.
The Decimal128 reconstruction issue is fixed, and I do not have further blocking comments.
2 participants
Purpose
No Linked issue.
Introduce the core predicate framework:
Predicate— base interface for row-level filter predicates (predicate.h)LeafPredicate— single-field predicate with typed function evaluation (leaf_predicate.h/cpp)CompoundPredicate— And/Or composition of child predicates (compound_predicate.h/cpp)Function/FunctionVisitor— base function hierarchy and visitor pattern (function.h,function_visitor.h)Literal— type-erased value holder supporting all Paimon data types with comparison and serialization (literal.h/cpp)LiteralConverter— Arrow array to Literal type conversion (literal_converter.h/cpp)LeafFunction,LeafUnaryFunction,NullFalseLeafBinaryFunction,StringLeafBinaryFunction,MultiLiteralsLeafFunction,CompoundFunctionLeafPredicateImpl/CompoundPredicateImpl— internal predicate evaluation logicPredicateFilter— row filtering via predicatesTests
literal_test.cpp— Literal value construction, comparison, and serialization for all data typesliteral_converter_test.cpp— Arrow array to Literal conversion for primitive, string, decimal, and temporal typesAPI and Format
New public headers under
include/paimon/predicate/.Documentation
Generative AI tooling
Migrate-by: Aone Copilot (Claude)