wgtmac commented on code in PR #206:
URL: https://github.com/apache/iceberg-cpp/pull/206#discussion_r2386614311
##########
src/iceberg/expression/literal.cc:
##########
@@ -283,28 +433,29 @@ std::string Literal::ToString() const {
return std::to_string(std::get<double>(value_));
}
case TypeId::kString: {
- return std::get<std::string>(value_);
+ return "\"" + std::get<std::string>(value_) + "\"";
}
- case TypeId::kBinary: {
+ case TypeId::kBinary:
+ case TypeId::kFixed: {
const auto& binary_data = std::get<std::vector<uint8_t>>(value_);
- std::string result;
- result.reserve(binary_data.size() * 2); // 2 chars per byte
+ std::string result = "X'";
+ result.reserve(/*prefix*/ 2 + /*suffix*/ 1 + /*data*/ binary_data.size()
* 2);
for (const auto& byte : binary_data) {
std::format_to(std::back_inserter(result), "{:02X}", byte);
}
+ result.push_back('\'');
return result;
}
- case TypeId::kDecimal:
- case TypeId::kUuid:
- case TypeId::kFixed:
- case TypeId::kDate:
case TypeId::kTime:
case TypeId::kTimestamp:
case TypeId::kTimestampTz: {
- throw IcebergError("Not implemented: ToString for " + type_->ToString());
+ return std::to_string(std::get<int64_t>(value_));
Review Comment:
I think these switch cases can be easily rewritten by `return
std::to_string(std::get<typename LiteralTraits<type_id>::ValueType>(value_));`
once https://github.com/apache/iceberg-cpp/pull/185 is merged.
##########
src/iceberg/expression/literal.cc:
##########
@@ -256,6 +402,10 @@ std::partial_ordering Literal::operator<=>(const Literal&
other) const {
}
std::string Literal::ToString() const {
+ auto unsupported_error = [this]() {
+ return std::format("ToString not supported for type: {}",
type_->ToString());
+ };
+
Review Comment:
```suggestion
```
##########
src/iceberg/expression/literal.h:
##########
@@ -56,7 +58,7 @@ class ICEBERG_EXPORT Literal {
double, // for double
std::string, // for string
std::vector<uint8_t>, // for binary, fixed
- std::array<uint8_t, 16>, // for uuid and decimal
+ std::array<uint8_t, 16>, // for uuid
Review Comment:
ditto
##########
src/iceberg/transform_function.cc:
##########
@@ -27,6 +27,7 @@
#include "iceberg/expression/literal.h"
#include "iceberg/type.h"
+#include "iceberg/util/int128.h"
Review Comment:
ditto
##########
test/literal_test.cc:
##########
@@ -175,12 +204,11 @@ TEST(LiteralTest, FloatComparison) {
}
TEST(LiteralTest, FloatCastTo) {
- auto float_literal = Literal::Float(3.14f);
+ auto float_literal = Literal::Float(2.0f);
// Cast to Double
- auto double_result = float_literal.CastTo(iceberg::float64());
- ASSERT_THAT(double_result, IsOk());
- EXPECT_EQ(double_result->type()->type_id(), TypeId::kDouble);
+ AssertCastSucceeds(float_literal.CastTo(iceberg::float64()), TypeId::kDouble,
+ static_cast<double>(2.0f));
Review Comment:
```suggestion
2.0);
```
##########
test/literal_test.cc:
##########
@@ -120,23 +133,31 @@ TEST(LiteralTest, LongCastTo) {
auto long_literal = Literal::Long(42L);
// Cast to Int (within range)
- auto int_result = long_literal.CastTo(iceberg::int32());
- ASSERT_THAT(int_result, IsOk());
- EXPECT_EQ(int_result->type()->type_id(), TypeId::kInt);
- EXPECT_EQ(int_result->ToString(), "42");
+ AssertCastSucceeds(long_literal.CastTo(iceberg::int32()), TypeId::kInt, 42);
// Cast to Float
- auto float_result = long_literal.CastTo(iceberg::float32());
- ASSERT_THAT(float_result, IsOk());
- EXPECT_EQ(float_result->type()->type_id(), TypeId::kFloat);
+ AssertCastSucceeds(long_literal.CastTo(iceberg::float32()), TypeId::kFloat,
42.0f);
// Cast to Double
- auto double_result = long_literal.CastTo(iceberg::float64());
- ASSERT_THAT(double_result, IsOk());
- EXPECT_EQ(double_result->type()->type_id(), TypeId::kDouble);
+ AssertCastSucceeds(long_literal.CastTo(iceberg::float64()), TypeId::kDouble,
42.0);
+
+ // Cast to Date
+ AssertCastSucceeds(long_literal.CastTo(iceberg::date()), TypeId::kDate, 42);
+
+ // Cast to Time
+ AssertCastSucceeds(long_literal.CastTo(iceberg::time()), TypeId::kTime,
+ static_cast<int64_t>(42));
Review Comment:
```suggestion
int64_t{42});
```
Same for below
##########
src/iceberg/expression/literal.h:
##########
@@ -27,6 +27,8 @@
#include "iceberg/result.h"
#include "iceberg/type.h"
+#include "iceberg/util/decimal.h"
+#include "iceberg/util/int128.h"
Review Comment:
Revert these related lines?
##########
src/iceberg/expression/literal.cc:
##########
@@ -283,28 +433,29 @@ std::string Literal::ToString() const {
return std::to_string(std::get<double>(value_));
}
case TypeId::kString: {
- return std::get<std::string>(value_);
+ return "\"" + std::get<std::string>(value_) + "\"";
}
- case TypeId::kBinary: {
+ case TypeId::kBinary:
+ case TypeId::kFixed: {
const auto& binary_data = std::get<std::vector<uint8_t>>(value_);
- std::string result;
- result.reserve(binary_data.size() * 2); // 2 chars per byte
+ std::string result = "X'";
+ result.reserve(/*prefix*/ 2 + /*suffix*/ 1 + /*data*/ binary_data.size()
* 2);
for (const auto& byte : binary_data) {
std::format_to(std::back_inserter(result), "{:02X}", byte);
}
+ result.push_back('\'');
return result;
}
- case TypeId::kDecimal:
- case TypeId::kUuid:
- case TypeId::kFixed:
- case TypeId::kDate:
case TypeId::kTime:
case TypeId::kTimestamp:
case TypeId::kTimestampTz: {
- throw IcebergError("Not implemented: ToString for " + type_->ToString());
+ return std::to_string(std::get<int64_t>(value_));
+ }
+ case TypeId::kDate: {
+ return std::to_string(std::get<int32_t>(value_));
}
default: {
- throw IcebergError("Unknown type: " + type_->ToString());
+ return unsupported_error();
Review Comment:
```suggestion
return std::format("invalid literal of type {}", type_->ToString());
```
##########
test/literal_test.cc:
##########
@@ -30,6 +30,22 @@
namespace iceberg {
+namespace {
+
+// Helper function to assert that a CastTo operation succeeds and checks
+// the resulting type and value.
+template <typename T>
+void AssertCastSucceeds(const Result<Literal>& result, TypeId expected_type_id,
+ const T& expected_value) {
+ ASSERT_THAT(result, IsOk());
+ EXPECT_EQ(result->type()->type_id(), expected_type_id);
+ ASSERT_NO_THROW(EXPECT_EQ(std::get<T>(result->value()), expected_value))
Review Comment:
```suggestion
EXPECT_EQ(std::get<T>(result->value()), expected_value)
```
Why adding `ASSERT_NO_THROW` here if we already have `EXPECT_EQ`?
##########
test/literal_test.cc:
##########
@@ -254,6 +305,149 @@ TEST(LiteralTest, BinaryComparison) {
EXPECT_EQ(binary2 <=> binary1, std::partial_ordering::greater);
}
+TEST(LiteralTest, BinaryCastTo) {
+ std::vector<uint8_t> data4 = {0x01, 0x02, 0x03, 0x04};
+ auto binary_literal = Literal::Binary(data4);
+
+ // Cast to Fixed with matching length
+ AssertCastSucceeds(binary_literal.CastTo(iceberg::fixed(4)), TypeId::kFixed,
data4);
Review Comment:
```suggestion
AssertCastSucceeds(binary_literal.CastTo(fixed(4)), TypeId::kFixed, data4);
```
##########
test/literal_test.cc:
##########
@@ -150,6 +171,14 @@ TEST(LiteralTest, LongCastToIntOverflow) {
auto min_result = min_long.CastTo(iceberg::int32());
ASSERT_THAT(min_result, IsOk());
EXPECT_TRUE(min_result->IsBelowMin());
+
+ max_result = max_long.CastTo(iceberg::date());
Review Comment:
```suggestion
max_result = max_long.CastTo(date());
```
##########
test/literal_test.cc:
##########
@@ -205,6 +233,29 @@ TEST(LiteralTest, DoubleComparison) {
EXPECT_EQ(double2 <=> double1, std::partial_ordering::greater);
}
+TEST(LiteralTest, DoubleCastTo) {
+ auto double_literal = Literal::Double(2.0);
+
+ // Cast to Float
+ AssertCastSucceeds(double_literal.CastTo(iceberg::float32()),
TypeId::kFloat, 2.0f);
+}
+
+TEST(LiteralTest, DoubleCastToOverflow) {
+ // Test overflow cases for Double to Float
+ auto max_double =
+ Literal::Double(static_cast<double>(std::numeric_limits<float>::max()) *
2);
+ auto min_double =
+ Literal::Double(-static_cast<double>(std::numeric_limits<float>::max())
* 2);
+
+ auto max_result = max_double.CastTo(iceberg::float32());
Review Comment:
```suggestion
auto max_result = max_double.CastTo(float32());
```
##########
test/literal_test.cc:
##########
@@ -30,6 +30,22 @@
namespace iceberg {
+namespace {
+
+// Helper function to assert that a CastTo operation succeeds and checks
+// the resulting type and value.
+template <typename T>
+void AssertCastSucceeds(const Result<Literal>& result, TypeId expected_type_id,
+ const T& expected_value) {
+ ASSERT_THAT(result, IsOk());
+ EXPECT_EQ(result->type()->type_id(), expected_type_id);
Review Comment:
```suggestion
ASSERT_EQ(result->type()->type_id(), expected_type_id);
```
##########
test/literal_test.cc:
##########
@@ -254,6 +305,149 @@ TEST(LiteralTest, BinaryComparison) {
EXPECT_EQ(binary2 <=> binary1, std::partial_ordering::greater);
}
+TEST(LiteralTest, BinaryCastTo) {
+ std::vector<uint8_t> data4 = {0x01, 0x02, 0x03, 0x04};
+ auto binary_literal = Literal::Binary(data4);
+
+ // Cast to Fixed with matching length
+ AssertCastSucceeds(binary_literal.CastTo(iceberg::fixed(4)), TypeId::kFixed,
data4);
+
+ // Cast to Fixed with different length should fail
+ EXPECT_THAT(binary_literal.CastTo(iceberg::fixed(5)),
Review Comment:
```suggestion
EXPECT_THAT(binary_literal.CastTo(fixed(5)),
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
