Skip to content

Commit 8d3f274

Browse files
HeartLinkedHeartLinked
committed
fix review
1 parent bcdfa60 commit 8d3f274

4 files changed

Lines changed: 26 additions & 53 deletions

File tree

‎src/iceberg/expression/literal.cc‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
#include "iceberg/exception.h"
2626
#include "iceberg/util/conversions.h"
27+
#include "iceberg/util/macros.h"
2728

2829
namespace iceberg {
2930

@@ -150,7 +151,8 @@ Literal Literal::Binary(std::vector<uint8_t> value) {
150151
return {Value{std::move(value)}, binary()};
151152
}
152153

153-
Literal Literal::Fixed(std::vector<uint8_t> value, int32_t length) {
154+
Literal Literal::Fixed(std::vector<uint8_t> value) {
155+
auto length = static_cast<int32_t>(value.size());
154156
return {Value{std::move(value)}, fixed(length)};
155157
}
156158

@@ -194,7 +196,7 @@ bool Literal::operator==(const Literal& other) const { return (*this <=> other)
194196
// Three-way comparison operator
195197
std::partial_ordering Literal::operator<=>(const Literal& other) const {
196198
// If types are different, comparison is unordered
197-
if (type_->type_id() != other.type_->type_id()) {
199+
if (*type_ != *other.type_) {
198200
return std::partial_ordering::unordered;
199201
}
200202

@@ -256,14 +258,6 @@ std::partial_ordering Literal::operator<=>(const Literal& other) const {
256258
}
257259

258260
case TypeId::kFixed: {
259-
// Fixed types can only be compared if they have the same length
260-
auto& this_fixed_type = static_cast<const FixedType&>(*type_);
261-
auto& other_fixed_type = static_cast<const FixedType&>(*other.type_);
262-
263-
if (this_fixed_type.length() != other_fixed_type.length()) {
264-
return std::partial_ordering::unordered;
265-
}
266-
267261
auto& this_val = std::get<std::vector<uint8_t>>(value_);
268262
auto& other_val = std::get<std::vector<uint8_t>>(other.value_);
269263
return this_val <=> other_val;

‎src/iceberg/expression/literal.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ class ICEBERG_EXPORT Literal : public util::Formattable {
7272
static Literal Double(double value);
7373
static Literal String(std::string value);
7474
static Literal Binary(std::vector<uint8_t> value);
75-
static Literal Fixed(std::vector<uint8_t> value, int32_t length);
75+
static Literal Fixed(std::vector<uint8_t> value);
7676

7777
/// \brief Create a literal representing a null value.
7878
static Literal Null(std::shared_ptr<PrimitiveType> type) {

‎src/iceberg/test/literal_test.cc‎

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,6 @@ TEST(LiteralTest, LongCastTo) {
136136
EXPECT_EQ(double_result->type()->type_id(), TypeId::kDouble);
137137
}
138138

139-
// Test overflow cases
140139
TEST(LiteralTest, LongCastToIntOverflow) {
141140
auto max_long =
142141
Literal::Long(static_cast<int64_t>(std::numeric_limits<int32_t>::max()) + 1);
@@ -495,23 +494,22 @@ INSTANTIATE_TEST_SUITE_P(
495494
// Fixed type
496495
LiteralRoundTripParam{"FixedLength4",
497496
{0x01, 0x02, 0x03, 0x04},
498-
Literal::Fixed({0x01, 0x02, 0x03, 0x04}, 4),
497+
Literal::Fixed({0x01, 0x02, 0x03, 0x04}),
499498
fixed(4)},
500499
LiteralRoundTripParam{
501500
"FixedLength8",
502501
{0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0xFF, 0x00, 0x11},
503-
Literal::Fixed({0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0xFF, 0x00, 0x11}, 8),
502+
Literal::Fixed({0xAA, 0xBB, 0xCC, 0xDD, 0xEE, 0xFF, 0x00, 0x11}),
504503
fixed(8)},
505504
LiteralRoundTripParam{
506505
"FixedLength16",
507506
{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0A, 0x0B, 0x0C,
508507
0x0D, 0x0E, 0x0F},
509508
Literal::Fixed({0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09,
510-
0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F},
511-
16),
509+
0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F}),
512510
fixed(16)},
513511
LiteralRoundTripParam{
514-
"FixedSingleByte", {0xFF}, Literal::Fixed({0xFF}, 1), fixed(1)},
512+
"FixedSingleByte", {0xFF}, Literal::Fixed({0xFF}), fixed(1)},
515513

516514
// Temporal types
517515
LiteralRoundTripParam{"DateEpoch", {0, 0, 0, 0}, Literal::Date(0), date()},
@@ -562,7 +560,19 @@ TEST(LiteralSerializationTest, EmptyString) {
562560
EXPECT_TRUE(empty_bytes->empty());
563561

564562
auto deserialize_result = Literal::Deserialize(*empty_bytes, string());
565-
EXPECT_THAT(deserialize_result, IsError(ErrorKind::kInvalidArgument));
563+
ASSERT_THAT(deserialize_result, IsOk());
564+
EXPECT_TRUE(std::get<std::string>(deserialize_result->value()).empty());
565+
}
566+
567+
TEST(LiteralSerializationTest, EmptyBinary) {
568+
auto empty_binary = Literal::Binary({});
569+
auto empty_bytes = empty_binary.Serialize();
570+
ASSERT_TRUE(empty_bytes.has_value());
571+
EXPECT_TRUE(empty_bytes->empty());
572+
573+
auto deserialize_result = Literal::Deserialize(*empty_bytes, binary());
574+
ASSERT_THAT(deserialize_result, IsOk());
575+
EXPECT_TRUE(std::get<std::vector<uint8_t>>(deserialize_result->value()).empty());
566576
}
567577

568578
// Type promotion tests
@@ -574,20 +584,12 @@ TEST(LiteralSerializationTest, TypePromotion) {
574584
EXPECT_EQ(long_result->type()->type_id(), TypeId::kLong);
575585
EXPECT_EQ(std::get<int64_t>(long_result->value()), 32L);
576586

577-
auto long_bytes = long_result->Serialize();
578-
ASSERT_TRUE(long_bytes.has_value());
579-
EXPECT_EQ(long_bytes->size(), 8);
580-
581587
// 4-byte float data can be deserialized as double
582588
std::vector<uint8_t> float_data = {0, 0, 128, 63};
583589
auto double_result = Literal::Deserialize(float_data, float64());
584590
ASSERT_TRUE(double_result.has_value());
585591
EXPECT_EQ(double_result->type()->type_id(), TypeId::kDouble);
586-
EXPECT_EQ(std::get<double>(double_result->value()), 1.0);
587-
588-
auto double_bytes = double_result->Serialize();
589-
ASSERT_TRUE(double_bytes.has_value());
590-
EXPECT_EQ(double_bytes->size(), 8);
592+
EXPECT_DOUBLE_EQ(std::get<double>(double_result->value()), 1.0);
591593
}
592594

593595
} // namespace iceberg

‎src/iceberg/util/conversions.cc‎

Lines changed: 3 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -77,14 +77,7 @@ Result<std::vector<uint8_t>> ToBytesImpl<TypeId::kBinary>(const Literal::Value&
7777

7878
template <>
7979
Result<std::vector<uint8_t>> ToBytesImpl<TypeId::kFixed>(const Literal::Value& value) {
80-
if (std::holds_alternative<std::vector<uint8_t>>(value)) {
81-
return std::get<std::vector<uint8_t>>(value);
82-
} else {
83-
std::string actual_type =
84-
std::visit([](auto&& arg) -> std::string { return typeid(arg).name(); }, value);
85-
return InvalidArgument("Invalid value type for Fixed literal, got type: {}",
86-
actual_type);
87-
}
80+
return std::get<std::vector<uint8_t>>(value);
8881
}
8982

9083
#define DISPATCH_LITERAL_TO_BYTES(type_id) \
@@ -134,28 +127,20 @@ Result<std::vector<uint8_t>> Conversions::ToBytes(const Literal& literal) {
134127

135128
Result<Literal::Value> Conversions::FromBytes(const PrimitiveType& type,
136129
std::span<const uint8_t> data) {
137-
if (data.empty()) {
138-
return InvalidArgument("Cannot deserialize empty value");
139-
}
140-
141130
const auto type_id = type.type_id();
142-
143131
switch (type_id) {
144132
case TypeId::kBoolean: {
145133
ICEBERG_ASSIGN_OR_RAISE(auto value, ReadLittleEndian<uint8_t>(data));
146134
return Literal::Value{static_cast<bool>(value != 0x00)};
147135
}
148-
149136
case TypeId::kInt: {
150137
ICEBERG_ASSIGN_OR_RAISE(auto value, ReadLittleEndian<int32_t>(data));
151138
return Literal::Value{value};
152139
}
153-
154140
case TypeId::kDate: {
155141
ICEBERG_ASSIGN_OR_RAISE(auto value, ReadLittleEndian<int32_t>(data));
156142
return Literal::Value{value};
157143
}
158-
159144
case TypeId::kLong:
160145
case TypeId::kTime:
161146
case TypeId::kTimestamp:
@@ -171,12 +156,10 @@ Result<Literal::Value> Conversions::FromBytes(const PrimitiveType& type,
171156
}
172157
return Literal::Value{value};
173158
}
174-
175159
case TypeId::kFloat: {
176160
ICEBERG_ASSIGN_OR_RAISE(auto value, ReadLittleEndian<float>(data));
177161
return Literal::Value{value};
178162
}
179-
180163
case TypeId::kDouble: {
181164
if (data.size() < 8) {
182165
// Type was promoted from float to double
@@ -187,16 +170,11 @@ Result<Literal::Value> Conversions::FromBytes(const PrimitiveType& type,
187170
return Literal::Value{double_value};
188171
}
189172
}
190-
191-
case TypeId::kString: {
173+
case TypeId::kString:
192174
return Literal::Value{
193175
std::string(reinterpret_cast<const char*>(data.data()), data.size())};
194-
}
195-
196-
case TypeId::kBinary: {
176+
case TypeId::kBinary:
197177
return Literal::Value{std::vector<uint8_t>(data.begin(), data.end())};
198-
}
199-
200178
case TypeId::kFixed: {
201179
const auto& fixed_type = static_cast<const FixedType&>(type);
202180
if (data.size() != fixed_type.length()) {
@@ -206,7 +184,6 @@ Result<Literal::Value> Conversions::FromBytes(const PrimitiveType& type,
206184
return Literal::Value{std::vector<uint8_t>(data.begin(), data.end())};
207185
}
208186
// TODO(Li Feiyang): Add support for UUID and Decimal
209-
210187
default:
211188
return NotSupported("Deserialization for type {} is not supported",
212189
type.ToString());

0 commit comments

Comments
 (0)