wgtmac commented on code in PR #206:
URL: https://github.com/apache/iceberg-cpp/pull/206#discussion_r2338449317
##########
src/iceberg/expression/literal.cc:
##########
@@ -109,17 +172,230 @@ Result<Literal> LiteralCaster::CastFromLong(
Result<Literal> LiteralCaster::CastFromFloat(
const Literal& literal, const std::shared_ptr<PrimitiveType>& target_type)
{
auto float_val = std::get<float>(literal.value_);
- auto target_type_id = target_type->type_id();
- switch (target_type_id) {
+ switch (target_type->type_id()) {
case TypeId::kDouble:
return Literal::Double(static_cast<double>(float_val));
+ // TODO(Li Feiyang): Implement cast from Float to decimal
default:
return NotSupported("Cast from Float to {} is not supported",
target_type->ToString());
}
}
+Result<Literal> LiteralCaster::CastFromDouble(
+ const Literal& literal, const std::shared_ptr<PrimitiveType>& target_type)
{
+ auto double_val = std::get<double>(literal.value_);
+
+ switch (target_type->type_id()) {
+ case TypeId::kFloat: {
+ if (double_val > static_cast<double>(std::numeric_limits<float>::max()))
{
+ return AboveMaxLiteral(target_type);
+ }
+ if (double_val <
static_cast<double>(std::numeric_limits<float>::lowest())) {
+ return BelowMinLiteral(target_type);
+ }
+ return Literal::Float(static_cast<float>(double_val));
+ }
+ default:
+ return NotSupported("Cast from Double to {} is not supported",
+ target_type->ToString());
+ }
+}
+
+Result<Literal> LiteralCaster::CastFromString(
+ const Literal& literal, const std::shared_ptr<PrimitiveType>& target_type)
{
+ const auto& str_val = std::get<std::string>(literal.value_);
+ std::istringstream in{str_val};
+ std::tm tm = {};
+
+ switch (target_type->type_id()) {
+ case TypeId::kDate: {
+ // Parse "YYYY-MM-DD" into days since 1970-01-01 epoch.
+ in >> std::get_time(&tm, "%Y-%m-%d");
+
+ if (in.fail() || tm.tm_mday == 0 || in.peek() != EOF) {
+ return NotSupported("Failed to parse '{}' as a valid Date (expected
YYYY-MM-DD)",
Review Comment:
`NotSupported` means that we cannot do an operation. Here we just fail to do
an operation so it is better to use `InvalidArgument`.
Same for below.
##########
src/iceberg/expression/literal.h:
##########
@@ -71,6 +71,7 @@ class ICEBERG_EXPORT Literal {
static Literal Double(double value);
static Literal String(std::string value);
static Literal Binary(std::vector<uint8_t> value);
+ static Literal Fixed(std::vector<uint8_t> value);
Review Comment:
Finally I think `String`, `Binary` and `Fixed` should also accept
std::string_view, std::span<const uint8_t> and other acceptable value types.
##########
test/literal_test.cc:
##########
@@ -134,9 +140,33 @@ TEST(LiteralTest, LongCastTo) {
auto double_result = long_literal.CastTo(iceberg::float64());
ASSERT_THAT(double_result, IsOk());
EXPECT_EQ(double_result->type()->type_id(), TypeId::kDouble);
+
+ // Cast to Date
+ auto date_result = long_literal.CastTo(iceberg::date());
+ ASSERT_THAT(date_result, IsOk());
+ EXPECT_EQ(date_result->type()->type_id(), TypeId::kDate);
+ EXPECT_EQ(date_result->ToString(), "42");
Review Comment:
Shouldn't we directly test the value instead of converting to string?
##########
src/iceberg/expression/literal.cc:
##########
@@ -109,17 +172,230 @@ Result<Literal> LiteralCaster::CastFromLong(
Result<Literal> LiteralCaster::CastFromFloat(
const Literal& literal, const std::shared_ptr<PrimitiveType>& target_type)
{
auto float_val = std::get<float>(literal.value_);
- auto target_type_id = target_type->type_id();
- switch (target_type_id) {
+ switch (target_type->type_id()) {
case TypeId::kDouble:
return Literal::Double(static_cast<double>(float_val));
+ // TODO(Li Feiyang): Implement cast from Float to decimal
default:
return NotSupported("Cast from Float to {} is not supported",
target_type->ToString());
}
}
+Result<Literal> LiteralCaster::CastFromDouble(
+ const Literal& literal, const std::shared_ptr<PrimitiveType>& target_type)
{
+ auto double_val = std::get<double>(literal.value_);
+
+ switch (target_type->type_id()) {
+ case TypeId::kFloat: {
+ if (double_val > static_cast<double>(std::numeric_limits<float>::max()))
{
+ return AboveMaxLiteral(target_type);
+ }
+ if (double_val <
static_cast<double>(std::numeric_limits<float>::lowest())) {
+ return BelowMinLiteral(target_type);
+ }
+ return Literal::Float(static_cast<float>(double_val));
+ }
+ default:
+ return NotSupported("Cast from Double to {} is not supported",
+ target_type->ToString());
+ }
+}
+
+Result<Literal> LiteralCaster::CastFromString(
+ const Literal& literal, const std::shared_ptr<PrimitiveType>& target_type)
{
+ const auto& str_val = std::get<std::string>(literal.value_);
+ std::istringstream in{str_val};
+ std::tm tm = {};
+
+ switch (target_type->type_id()) {
+ case TypeId::kDate: {
+ // Parse "YYYY-MM-DD" into days since 1970-01-01 epoch.
+ in >> std::get_time(&tm, "%Y-%m-%d");
+
+ if (in.fail() || tm.tm_mday == 0 || in.peek() != EOF) {
Review Comment:
Why only checking tm.tm_mday but not other fields? Will `in.fail()` report
this kind of errors?
##########
src/iceberg/expression/literal.cc:
##########
@@ -109,17 +172,230 @@ Result<Literal> LiteralCaster::CastFromLong(
Result<Literal> LiteralCaster::CastFromFloat(
const Literal& literal, const std::shared_ptr<PrimitiveType>& target_type)
{
auto float_val = std::get<float>(literal.value_);
- auto target_type_id = target_type->type_id();
- switch (target_type_id) {
+ switch (target_type->type_id()) {
case TypeId::kDouble:
return Literal::Double(static_cast<double>(float_val));
+ // TODO(Li Feiyang): Implement cast from Float to decimal
default:
return NotSupported("Cast from Float to {} is not supported",
target_type->ToString());
}
}
+Result<Literal> LiteralCaster::CastFromDouble(
+ const Literal& literal, const std::shared_ptr<PrimitiveType>& target_type)
{
+ auto double_val = std::get<double>(literal.value_);
+
+ switch (target_type->type_id()) {
+ case TypeId::kFloat: {
+ if (double_val > static_cast<double>(std::numeric_limits<float>::max()))
{
+ return AboveMaxLiteral(target_type);
+ }
+ if (double_val <
static_cast<double>(std::numeric_limits<float>::lowest())) {
+ return BelowMinLiteral(target_type);
+ }
+ return Literal::Float(static_cast<float>(double_val));
+ }
+ default:
+ return NotSupported("Cast from Double to {} is not supported",
+ target_type->ToString());
+ }
+}
+
+Result<Literal> LiteralCaster::CastFromString(
+ const Literal& literal, const std::shared_ptr<PrimitiveType>& target_type)
{
+ const auto& str_val = std::get<std::string>(literal.value_);
+ std::istringstream in{str_val};
+ std::tm tm = {};
+
+ switch (target_type->type_id()) {
+ case TypeId::kDate: {
+ // Parse "YYYY-MM-DD" into days since 1970-01-01 epoch.
+ in >> std::get_time(&tm, "%Y-%m-%d");
+
+ if (in.fail() || tm.tm_mday == 0 || in.peek() != EOF) {
Review Comment:
```suggestion
if (in.fail() || tm.tm_mday == 0 || !in.eof()) {
```
##########
src/iceberg/expression/literal.cc:
##########
@@ -19,13 +19,34 @@
#include "iceberg/expression/literal.h"
+#include <chrono>
#include <cmath>
#include <concepts>
+#include <cstdint>
+#include <iomanip>
#include "iceberg/exception.h"
namespace iceberg {
+namespace {
+
+int32_t MicrosToDays(int64_t micros_since_epoch) {
+ std::chrono::microseconds micros(micros_since_epoch);
+ auto days_duration = std::chrono::floor<std::chrono::days>(micros);
+ return static_cast<int32_t>(days_duration.count());
+}
+
+time_t timegm_custom(std::tm* tm) {
Review Comment:
Do you mean https://en.cppreference.com/w/cpp/chrono/c/mktime.html?
##########
src/iceberg/expression/literal.cc:
##########
@@ -109,17 +172,230 @@ Result<Literal> LiteralCaster::CastFromLong(
Result<Literal> LiteralCaster::CastFromFloat(
const Literal& literal, const std::shared_ptr<PrimitiveType>& target_type)
{
auto float_val = std::get<float>(literal.value_);
- auto target_type_id = target_type->type_id();
- switch (target_type_id) {
+ switch (target_type->type_id()) {
case TypeId::kDouble:
return Literal::Double(static_cast<double>(float_val));
+ // TODO(Li Feiyang): Implement cast from Float to decimal
default:
return NotSupported("Cast from Float to {} is not supported",
target_type->ToString());
}
}
+Result<Literal> LiteralCaster::CastFromDouble(
+ const Literal& literal, const std::shared_ptr<PrimitiveType>& target_type)
{
+ auto double_val = std::get<double>(literal.value_);
+
+ switch (target_type->type_id()) {
+ case TypeId::kFloat: {
+ if (double_val > static_cast<double>(std::numeric_limits<float>::max()))
{
+ return AboveMaxLiteral(target_type);
+ }
+ if (double_val <
static_cast<double>(std::numeric_limits<float>::lowest())) {
+ return BelowMinLiteral(target_type);
+ }
+ return Literal::Float(static_cast<float>(double_val));
+ }
+ default:
+ return NotSupported("Cast from Double to {} is not supported",
+ target_type->ToString());
+ }
+}
+
+Result<Literal> LiteralCaster::CastFromString(
+ const Literal& literal, const std::shared_ptr<PrimitiveType>& target_type)
{
+ const auto& str_val = std::get<std::string>(literal.value_);
+ std::istringstream in{str_val};
+ std::tm tm = {};
+
+ switch (target_type->type_id()) {
+ case TypeId::kDate: {
+ // Parse "YYYY-MM-DD" into days since 1970-01-01 epoch.
Review Comment:
As discussed before, we need to move these utility functions to a new
`iceberg/util/date_time_util.h` and add exhaustive test cases for them.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
