Skip to content

Commit 2c4019e

Browse files
authored
refact: Refactor Literal operator== to delegate to CompareTo for consistent equality semantics (alibaba#313)
1 parent 8421526 commit 2c4019e

3 files changed

Lines changed: 29 additions & 60 deletions

File tree

include/paimon/predicate/literal.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,13 @@ class PAIMON_EXPORT Literal {
8989
std::string ToString() const;
9090

9191
/// Gets the hash code for this literal.
92+
/// @note HashCode() hashes the exact bit representation (including Decimal scale), while
93+
/// operator== delegates to CompareTo() which uses numeric equality (e.g. decimals with
94+
/// different scales can compare equal). This means the hash-equality contract (equal objects
95+
/// must have equal hashes) may be violated for Decimal literals with different scales. In
96+
/// practice this is safe because all current std::unordered_map<Literal, ...> usages (bitmap
97+
/// file index) only store values from the same column, which guarantees a fixed precision and
98+
/// scale.
9299
size_t HashCode() const;
93100

94101
/// Compares this literal with another literal. The comparison follows SQL semantics for the

src/paimon/common/predicate/literal.cpp

Lines changed: 5 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -339,7 +339,8 @@ Result<int32_t> Literal::CompareTo(const Literal& other) const {
339339
case FieldType::BINARY: {
340340
std::string_view v1(impl_->value_.Buffer, impl_->size_);
341341
std::string_view v2(other.impl_->value_.Buffer, other.impl_->size_);
342-
return (*this == other) ? 0 : (v1 < v2 ? -1 : 1);
342+
int32_t cmp = v1.compare(v2);
343+
return cmp < 0 ? -1 : (cmp > 0 ? 1 : 0);
343344
}
344345
case FieldType::TIMESTAMP:
345346
return impl_->value_.TimestampVal == other.impl_->value_.TimestampVal
@@ -361,65 +362,11 @@ bool Literal::operator==(const Literal& other) const {
361362
if (this == &other) {
362363
return true;
363364
}
364-
if (GetType() != other.GetType() || IsNull() != other.IsNull()) {
365+
auto result = CompareTo(other);
366+
if (!result.ok()) {
365367
return false;
366368
}
367-
if (IsNull()) {
368-
return true;
369-
}
370-
if (GetType() != FieldType::FLOAT && GetType() != FieldType::DOUBLE &&
371-
HashCode() != other.HashCode()) {
372-
return false;
373-
}
374-
switch (GetType()) {
375-
case FieldType::BOOLEAN:
376-
return impl_->value_.BooleanVal == other.impl_->value_.BooleanVal;
377-
case FieldType::TINYINT:
378-
return impl_->value_.TinyIntVal == other.impl_->value_.TinyIntVal;
379-
case FieldType::SMALLINT:
380-
return impl_->value_.SmallIntVal == other.impl_->value_.SmallIntVal;
381-
case FieldType::INT:
382-
return impl_->value_.IntVal == other.impl_->value_.IntVal;
383-
case FieldType::BIGINT:
384-
return impl_->value_.BigIntVal == other.impl_->value_.BigIntVal;
385-
case FieldType::FLOAT: {
386-
if (std::isnan(impl_->value_.FloatVal) && std::isnan(other.impl_->value_.FloatVal)) {
387-
return true;
388-
}
389-
if (impl_->value_.FloatVal == INFINITY && other.impl_->value_.FloatVal == INFINITY) {
390-
return true;
391-
}
392-
if (impl_->value_.FloatVal == -INFINITY && other.impl_->value_.FloatVal == -INFINITY) {
393-
return true;
394-
}
395-
return std::fabs(impl_->value_.FloatVal - other.impl_->value_.FloatVal) < 1E-5;
396-
}
397-
case FieldType::DOUBLE: {
398-
if (std::isnan(impl_->value_.DoubleVal) && std::isnan(other.impl_->value_.DoubleVal)) {
399-
return true;
400-
}
401-
if (impl_->value_.DoubleVal == INFINITY && other.impl_->value_.DoubleVal == INFINITY) {
402-
return true;
403-
}
404-
if (impl_->value_.DoubleVal == -INFINITY &&
405-
other.impl_->value_.DoubleVal == -INFINITY) {
406-
return true;
407-
}
408-
return std::fabs(impl_->value_.DoubleVal - other.impl_->value_.DoubleVal) < 1E-5;
409-
}
410-
case FieldType::STRING:
411-
case FieldType::BINARY:
412-
return impl_->size_ == other.impl_->size_ &&
413-
memcmp(impl_->value_.Buffer, other.impl_->value_.Buffer, impl_->size_) == 0;
414-
case FieldType::TIMESTAMP:
415-
return impl_->value_.TimestampVal == other.impl_->value_.TimestampVal;
416-
case FieldType::DECIMAL:
417-
return impl_->value_.DecimalVal == other.impl_->value_.DecimalVal;
418-
case FieldType::DATE:
419-
return impl_->value_.IntVal == other.impl_->value_.IntVal;
420-
default:
421-
return false;
422-
}
369+
return result.value() == 0;
423370
}
424371

425372
bool Literal::operator!=(const Literal& r) const {

src/paimon/core/casting/cast_executor_test.cpp

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -162,8 +162,23 @@ class CastExecutorTest : public ::testing::Test {
162162
for (size_t i = 0; i < src_data.size(); i++) {
163163
ASSERT_OK_AND_ASSIGN(Literal result, cast_executor->Cast(src_literals[i], target_type));
164164
ASSERT_FALSE(result.IsNull());
165-
ASSERT_EQ(target_literals[i], result)
166-
<< target_literals[i].ToString() << "->" << result.ToString();
165+
if (target_literals[i] != result) {
166+
// Fall back to approximate comparison for floating-point types, because
167+
// different conversion paths (e.g. decimal->float vs float literal) may
168+
// produce slightly different bit representations.
169+
if (target_field_type == FieldType::FLOAT) {
170+
ASSERT_NEAR(target_literals[i].GetValue<float>(), result.GetValue<float>(),
171+
1E-5)
172+
<< target_literals[i].ToString() << "->" << result.ToString();
173+
} else if (target_field_type == FieldType::DOUBLE) {
174+
ASSERT_NEAR(target_literals[i].GetValue<double>(), result.GetValue<double>(),
175+
1E-5)
176+
<< target_literals[i].ToString() << "->" << result.ToString();
177+
} else {
178+
ASSERT_EQ(target_literals[i], result)
179+
<< target_literals[i].ToString() << "->" << result.ToString();
180+
}
181+
}
167182
}
168183
// test null
169184
Literal null_literal(src_type);

0 commit comments

Comments
 (0)