wgtmac commented on code in PR #653:
URL: https://github.com/apache/iceberg-cpp/pull/653#discussion_r3285552500
##########
src/iceberg/expression/literal.cc:
##########
@@ -250,14 +263,27 @@ Result<Literal> LiteralCaster::CastFromString(
Result<Literal> LiteralCaster::CastFromTimestamp(
const Literal& literal, const std::shared_ptr<PrimitiveType>& target_type)
{
auto timestamp_val = std::get<int64_t>(literal.value_);
+ const auto& source_timestamp =
+ internal::checked_cast<const TimestampBase&>(*literal.type());
+ const bool source_is_nanos = source_timestamp.time_unit() ==
TimeUnit::kNanosecond;
switch (target_type->type_id()) {
case TypeId::kDate: {
ICEBERG_ASSIGN_OR_RAISE(auto days, TemporalUtils::ExtractDay(literal));
return Literal::Date(std::get<int32_t>(days.value()));
}
+ case TypeId::kTimestamp:
+ return source_is_nanos ? Literal::Timestamp(timestamp_val / 1000)
+ : Literal::Timestamp(timestamp_val);
case TypeId::kTimestampTz:
- return Literal::TimestampTz(timestamp_val);
+ return source_is_nanos ? Literal::TimestampTz(timestamp_val / 1000)
+ : Literal::TimestampTz(timestamp_val);
+ case TypeId::kTimestampNs:
+ return source_is_nanos ? Literal::TimestampNs(timestamp_val)
+ : Literal::TimestampNs(timestamp_val * 1000);
+ case TypeId::kTimestampTzNs:
Review Comment:
Casting from `Timestamp(Ns)` to `TimestampTz(Ns)` is allowed here, but Java
(`TimestampNanoLiteral.to(Type)`) explicitly returns `null` (blocking this
promotion) because timezone information is missing. Should we return
`NotSupported` to match Java?
##########
src/iceberg/expression/literal.cc:
##########
@@ -250,14 +263,27 @@ Result<Literal> LiteralCaster::CastFromString(
Result<Literal> LiteralCaster::CastFromTimestamp(
const Literal& literal, const std::shared_ptr<PrimitiveType>& target_type)
{
auto timestamp_val = std::get<int64_t>(literal.value_);
+ const auto& source_timestamp =
+ internal::checked_cast<const TimestampBase&>(*literal.type());
+ const bool source_is_nanos = source_timestamp.time_unit() ==
TimeUnit::kNanosecond;
switch (target_type->type_id()) {
case TypeId::kDate: {
ICEBERG_ASSIGN_OR_RAISE(auto days, TemporalUtils::ExtractDay(literal));
return Literal::Date(std::get<int32_t>(days.value()));
}
+ case TypeId::kTimestamp:
+ return source_is_nanos ? Literal::Timestamp(timestamp_val / 1000)
Review Comment:
C++ integer division truncates toward zero, causing incorrect results for
negative timestamps (pre-1970) not evenly divisible by 1000. Java uses
`Math.floorDiv`. We should use a floor division helper here.
##########
src/iceberg/test/literal_test.cc:
##########
@@ -580,7 +580,15 @@ INSTANTIATE_TEST_SUITE_P(
BasicLiteralTestParam{.test_name = "TimestampTz",
.literal =
Literal::TimestampTz(1684137600000000LL),
.expected_type_id = TypeId::kTimestampTz,
- .expected_string = "1684137600000000"}),
+ .expected_string = "1684137600000000"},
+ BasicLiteralTestParam{.test_name = "TimestampNs",
+ .literal =
Literal::TimestampNs(1684137600000000001LL),
+ .expected_type_id = TypeId::kTimestampNs,
+ .expected_string = "1684137600000000001"},
+ BasicLiteralTestParam{.test_name = "TimestampTzNs",
+ .literal =
Literal::TimestampTzNs(1684137600000000001LL),
+ .expected_type_id = TypeId::kTimestampTzNs,
Review Comment:
Consider adding cast tests for `TimestampNs` and `TimestampTzNs` (e.g., from
String, and cross-casting between `TimestampNs` and `Timestamp`), especially
for negative timestamps, to ensure rounding parity with Java.
--
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]
