wgtmac commented on code in PR #238:
URL: https://github.com/apache/iceberg-cpp/pull/238#discussion_r2428103742
##########
src/iceberg/expression/literal.cc:
##########
@@ -193,12 +205,44 @@ std::strong_ordering CompareFloat(T lhs, T rhs) {
return lhs_is_negative <=> rhs_is_negative;
}
+std::strong_ordering CompareDecimal(Literal const& lhs, Literal const& rhs) {
+ ICEBERG_DCHECK(std::holds_alternative<Decimal>(lhs.value()),
+ "LHS of decimal comparison must hold Decimal");
+ ICEBERG_DCHECK(std::holds_alternative<Decimal>(rhs.value()),
+ "RHS of decimal comparison must hold decimal");
+ const auto& lhs_type = std::dynamic_pointer_cast<DecimalType>(lhs.type());
+ const auto& rhs_type = std::dynamic_pointer_cast<DecimalType>(rhs.type());
+ ICEBERG_DCHECK(lhs_type != nullptr, "LHS type must be DecimalType");
+ ICEBERG_DCHECK(rhs_type != nullptr, "RHS type must be DecimalType");
+ auto lhs_decimal = std::get<Decimal>(lhs.value());
+ auto rhs_decimal = std::get<Decimal>(rhs.value());
+ if (lhs_type->scale() == rhs_type->scale()) {
+ return lhs_decimal <=> rhs_decimal;
+ } else if (lhs_type->scale() > rhs_type->scale()) {
+ // Rescale to larger scale
+ auto rhs_res = rhs_decimal.Rescale(rhs_type->scale(), lhs_type->scale());
+ if (!rhs_res) {
+ // Rescale would cause data loss, so lhs is definitely less than rhs
+ return std::strong_ordering::less;
+ }
+ return lhs_decimal <=> rhs_res.value();
+ } else {
+ // Rescale to larger scale
+ auto lhs_res = lhs_decimal.Rescale(lhs_type->scale(), rhs_type->scale());
+ if (!lhs_res) {
+ // Rescale would cause data loss, so lhs is definitely greater than rhs
+ return std::strong_ordering::greater;
+ }
+ return lhs_res.value() <=> rhs_decimal;
+ }
+}
+
bool Literal::operator==(const Literal& other) const { return (*this <=>
other) == 0; }
// Three-way comparison operator
std::partial_ordering Literal::operator<=>(const Literal& other) const {
// If types are different, comparison is unordered
- if (*type_ != *other.type_) {
+ if (type_->type_id() != other.type_->type_id()) {
Review Comment:
We shouldn't change this. Otherwise the assumption no longer holds for types
including fixed and timestamp.
##########
src/iceberg/expression/literal.h:
##########
@@ -76,6 +78,8 @@ class ICEBERG_EXPORT Literal : public util::Formattable {
static Literal UUID(Uuid value);
static Literal Binary(std::vector<uint8_t> value);
static Literal Fixed(std::vector<uint8_t> value);
+ static Literal MakeDecimal(Decimal value, int32_t precision, int32_t scale);
Review Comment:
```suggestion
static Literal Decimal(int128_t value, int32_t precision, int32_t scale);
```
I prefer `int128_t` over `Decimal` because it is no easier to create a
`Decimal` than `int128_t`. In this way, we also don't need to make this
function name a special one.
##########
src/iceberg/test/bucket_util_test.cc:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "iceberg/util/bucket_util.h"
+
+#include <chrono>
+
+#include <gtest/gtest.h>
+
+namespace iceberg {
+
+// The following tests are from
+// https://iceberg.apache.org/spec/#appendix-b-32-bit-hash-requirements
+TEST(BucketUtilsTest, HashHelper) {
+ // int and long
+ EXPECT_EQ(BucketUtils::HashInt(34), 2017239379);
+ EXPECT_EQ(BucketUtils::HashLong(34L), 2017239379);
+
+ // decimal hash
+ auto decimal = Decimal::FromString("14.20");
+ ASSERT_TRUE(decimal.has_value());
+ EXPECT_EQ(BucketUtils::HashBytes(Decimal::ToBigEndian(decimal->value())),
-500754589);
+
+ // date hash
+ // 2017-11-16
+ std::chrono::sys_days sd = std::chrono::year{2017} / 11 / 16;
+ std::chrono::sys_days epoch{std::chrono::year{1970} / 1 / 1};
+ int32_t days = (sd - epoch).count();
+ std::cout << "days: " << days << std::endl;
+ EXPECT_EQ(BucketUtils::HashInt(days), -653330422);
+
+ // time
+ // 22:31:08 in microseconds
+ int64_t time_micros = (22 * 3600 + 31 * 60 + 8) * 1000000LL;
+ std::cout << "time micros: " << time_micros << std::endl;
Review Comment:
```suggestion
```
##########
src/iceberg/test/bucket_util_test.cc:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "iceberg/util/bucket_util.h"
+
+#include <chrono>
+
+#include <gtest/gtest.h>
+
+namespace iceberg {
+
+// The following tests are from
+// https://iceberg.apache.org/spec/#appendix-b-32-bit-hash-requirements
+TEST(BucketUtilsTest, HashHelper) {
+ // int and long
+ EXPECT_EQ(BucketUtils::HashInt(34), 2017239379);
+ EXPECT_EQ(BucketUtils::HashLong(34L), 2017239379);
+
+ // decimal hash
+ auto decimal = Decimal::FromString("14.20");
+ ASSERT_TRUE(decimal.has_value());
+ EXPECT_EQ(BucketUtils::HashBytes(Decimal::ToBigEndian(decimal->value())),
-500754589);
+
+ // date hash
+ // 2017-11-16
+ std::chrono::sys_days sd = std::chrono::year{2017} / 11 / 16;
+ std::chrono::sys_days epoch{std::chrono::year{1970} / 1 / 1};
+ int32_t days = (sd - epoch).count();
+ std::cout << "days: " << days << std::endl;
+ EXPECT_EQ(BucketUtils::HashInt(days), -653330422);
+
+ // time
+ // 22:31:08 in microseconds
+ int64_t time_micros = (22 * 3600 + 31 * 60 + 8) * 1000000LL;
+ std::cout << "time micros: " << time_micros << std::endl;
+ EXPECT_EQ(BucketUtils::HashLong(time_micros), -662762989);
+
+ // timestamp
+ // 2017-11-16T22:31:08 in microseconds
+ std::chrono::system_clock::time_point tp =
+ std::chrono::sys_days{std::chrono::year{2017} / 11 / 16} +
std::chrono::hours{22} +
+ std::chrono::minutes{31} + std::chrono::seconds{8};
+ int64_t timestamp_micros =
+
std::chrono::duration_cast<std::chrono::microseconds>(tp.time_since_epoch())
+ .count();
+ std::cout << "timestamp micros: " << timestamp_micros << std::endl;
+ EXPECT_EQ(BucketUtils::HashLong(timestamp_micros), -2047944441);
+ // 2017-11-16T22:31:08.000001 in microseconds
+ EXPECT_EQ(BucketUtils::HashLong(timestamp_micros + 1), -1207196810);
+
+ // string
+ std::string str = "iceberg";
+ EXPECT_EQ(BucketUtils::HashBytes(std::span<const uint8_t>(
+ reinterpret_cast<const uint8_t*>(str.data()), str.size())),
+ 1210000089);
+
+ // uuid
+ // f79c3e09-677c-4bbd-a479-3f349cb785e7
+ std::array<uint8_t, 16> uuid = {0xf7, 0x9c, 0x3e, 0x09, 0x67, 0x7c, 0x4b,
0xbd,
Review Comment:
It looks better if we use `Uuid::FromString` here.
##########
src/iceberg/test/bucket_util_test.cc:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "iceberg/util/bucket_util.h"
+
+#include <chrono>
+
+#include <gtest/gtest.h>
+
+namespace iceberg {
+
+// The following tests are from
+// https://iceberg.apache.org/spec/#appendix-b-32-bit-hash-requirements
+TEST(BucketUtilsTest, HashHelper) {
+ // int and long
+ EXPECT_EQ(BucketUtils::HashInt(34), 2017239379);
+ EXPECT_EQ(BucketUtils::HashLong(34L), 2017239379);
+
+ // decimal hash
+ auto decimal = Decimal::FromString("14.20");
+ ASSERT_TRUE(decimal.has_value());
+ EXPECT_EQ(BucketUtils::HashBytes(Decimal::ToBigEndian(decimal->value())),
-500754589);
+
+ // date hash
+ // 2017-11-16
+ std::chrono::sys_days sd = std::chrono::year{2017} / 11 / 16;
+ std::chrono::sys_days epoch{std::chrono::year{1970} / 1 / 1};
+ int32_t days = (sd - epoch).count();
+ std::cout << "days: " << days << std::endl;
+ EXPECT_EQ(BucketUtils::HashInt(days), -653330422);
+
+ // time
+ // 22:31:08 in microseconds
+ int64_t time_micros = (22 * 3600 + 31 * 60 + 8) * 1000000LL;
+ std::cout << "time micros: " << time_micros << std::endl;
+ EXPECT_EQ(BucketUtils::HashLong(time_micros), -662762989);
+
+ // timestamp
+ // 2017-11-16T22:31:08 in microseconds
+ std::chrono::system_clock::time_point tp =
+ std::chrono::sys_days{std::chrono::year{2017} / 11 / 16} +
std::chrono::hours{22} +
+ std::chrono::minutes{31} + std::chrono::seconds{8};
+ int64_t timestamp_micros =
+
std::chrono::duration_cast<std::chrono::microseconds>(tp.time_since_epoch())
+ .count();
+ std::cout << "timestamp micros: " << timestamp_micros << std::endl;
Review Comment:
```suggestion
```
##########
src/iceberg/expression/literal.h:
##########
@@ -76,6 +78,8 @@ class ICEBERG_EXPORT Literal : public util::Formattable {
static Literal UUID(Uuid value);
static Literal Binary(std::vector<uint8_t> value);
static Literal Fixed(std::vector<uint8_t> value);
+ static Literal MakeDecimal(Decimal value, int32_t precision, int32_t scale);
Review Comment:
We can also use `static Literal Decimal_(Decimal value, int32_t precision,
int32_t scale);`
or
`static Literal Decimal(::iceberg::Decimal value, int32_t precision, int32_t
scale);`
##########
src/iceberg/test/bucket_util_test.cc:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "iceberg/util/bucket_util.h"
+
+#include <chrono>
+
+#include <gtest/gtest.h>
+
+namespace iceberg {
+
+// The following tests are from
+// https://iceberg.apache.org/spec/#appendix-b-32-bit-hash-requirements
+TEST(BucketUtilsTest, HashHelper) {
+ // int and long
+ EXPECT_EQ(BucketUtils::HashInt(34), 2017239379);
+ EXPECT_EQ(BucketUtils::HashLong(34L), 2017239379);
+
+ // decimal hash
+ auto decimal = Decimal::FromString("14.20");
+ ASSERT_TRUE(decimal.has_value());
+ EXPECT_EQ(BucketUtils::HashBytes(Decimal::ToBigEndian(decimal->value())),
-500754589);
+
+ // date hash
+ // 2017-11-16
Review Comment:
```suggestion
```
This looks unnecessary
##########
src/iceberg/test/literal_test.cc:
##########
@@ -204,6 +204,33 @@ TEST(LiteralTest, DoubleComparison) {
EXPECT_EQ(double2 <=> double1, std::partial_ordering::greater);
}
+// Decimal type tests
+TEST(LiteralTest, DecimalBasics) {
Review Comment:
Please rebase this PR. I think a recent commit has improved this file a lot.
Please follow the similar pattern to use parameterized test.
##########
src/iceberg/test/bucket_util_test.cc:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "iceberg/util/bucket_util.h"
+
+#include <chrono>
+
+#include <gtest/gtest.h>
+
+namespace iceberg {
+
+// The following tests are from
+// https://iceberg.apache.org/spec/#appendix-b-32-bit-hash-requirements
+TEST(BucketUtilsTest, HashHelper) {
+ // int and long
+ EXPECT_EQ(BucketUtils::HashInt(34), 2017239379);
+ EXPECT_EQ(BucketUtils::HashLong(34L), 2017239379);
+
+ // decimal hash
+ auto decimal = Decimal::FromString("14.20");
+ ASSERT_TRUE(decimal.has_value());
+ EXPECT_EQ(BucketUtils::HashBytes(Decimal::ToBigEndian(decimal->value())),
-500754589);
+
+ // date hash
+ // 2017-11-16
+ std::chrono::sys_days sd = std::chrono::year{2017} / 11 / 16;
+ std::chrono::sys_days epoch{std::chrono::year{1970} / 1 / 1};
+ int32_t days = (sd - epoch).count();
+ std::cout << "days: " << days << std::endl;
+ EXPECT_EQ(BucketUtils::HashInt(days), -653330422);
+
+ // time
+ // 22:31:08 in microseconds
+ int64_t time_micros = (22 * 3600 + 31 * 60 + 8) * 1000000LL;
+ std::cout << "time micros: " << time_micros << std::endl;
+ EXPECT_EQ(BucketUtils::HashLong(time_micros), -662762989);
+
+ // timestamp
+ // 2017-11-16T22:31:08 in microseconds
+ std::chrono::system_clock::time_point tp =
Review Comment:
Perhaps we can add some utility functions in the temporal_util.h to create
date/time/timestamp from structural inputs.
##########
src/iceberg/test/bucket_util_test.cc:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "iceberg/util/bucket_util.h"
+
+#include <chrono>
+
+#include <gtest/gtest.h>
+
+namespace iceberg {
+
+// The following tests are from
+// https://iceberg.apache.org/spec/#appendix-b-32-bit-hash-requirements
+TEST(BucketUtilsTest, HashHelper) {
+ // int and long
+ EXPECT_EQ(BucketUtils::HashInt(34), 2017239379);
+ EXPECT_EQ(BucketUtils::HashLong(34L), 2017239379);
+
+ // decimal hash
+ auto decimal = Decimal::FromString("14.20");
+ ASSERT_TRUE(decimal.has_value());
+ EXPECT_EQ(BucketUtils::HashBytes(Decimal::ToBigEndian(decimal->value())),
-500754589);
+
+ // date hash
+ // 2017-11-16
+ std::chrono::sys_days sd = std::chrono::year{2017} / 11 / 16;
+ std::chrono::sys_days epoch{std::chrono::year{1970} / 1 / 1};
+ int32_t days = (sd - epoch).count();
+ std::cout << "days: " << days << std::endl;
Review Comment:
```suggestion
```
##########
src/iceberg/test/transform_test.cc:
##########
@@ -245,6 +246,14 @@ TEST(TransformLiteralTest, BucketTransform) {
constexpr int32_t num_buckets = 4;
auto transform = Transform::Bucket(num_buckets);
+ // uuid
+ // f79c3e09-677c-4bbd-a479-3f349cb785e7
+ std::array<uint8_t, 16> uuid = {0xf7, 0x9c, 0x3e, 0x09, 0x67, 0x7c, 0x4b,
0xbd,
Review Comment:
nit: use `Uuid` here?
##########
src/iceberg/test/transform_test.cc:
##########
@@ -253,23 +262,43 @@ TEST(TransformLiteralTest, BucketTransform) {
const std::vector<Case> cases = {
{.source_type = iceberg::int32(),
- .source = Literal::Int(42),
+ .source = Literal::Int(34),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(34),
+ .expected = Literal::Int(3)},
+ // decimal 14.20
+ {.source_type = iceberg::decimal(4, 2),
+ .source = Literal::MakeDecimal(1420, 4, 2),
.expected = Literal::Int(3)},
+ // 2017-11-16
{.source_type = iceberg::date(),
- .source = Literal::Date(30000),
+ .source = Literal::Date(17486),
.expected = Literal::Int(2)},
- {.source_type = iceberg::int64(),
- .source = Literal::Long(1234567890),
+ // // 22:31:08 in microseconds
+ {.source_type = iceberg::time(),
+ .source = Literal::Time(81068000000),
.expected = Literal::Int(3)},
+ // // 2017-11-16T22:31:08 in microseconds
{.source_type = iceberg::timestamp(),
- .source = Literal::Timestamp(1622547800000000),
- .expected = Literal::Int(1)},
+ .source = Literal::Timestamp(1510871468000000),
+ .expected = Literal::Int(3)},
+ // // 2017-11-16T22:31:08.000001 in microseconds
{.source_type = iceberg::timestamp_tz(),
- .source = Literal::TimestampTz(1622547800000000),
- .expected = Literal::Int(1)},
+ .source = Literal::TimestampTz(1510871468000001),
Review Comment:
Why changing both values here?
##########
src/iceberg/expression/literal.cc:
##########
@@ -307,6 +355,14 @@ std::string Literal::ToString() const {
case TypeId::kDouble: {
return std::to_string(std::get<double>(value_));
}
+ case TypeId::kDecimal: {
+ auto decimal = std::get<Decimal>(value_);
+ auto decimal_type = std::dynamic_pointer_cast<DecimalType>(type_);
+ ICEBERG_DCHECK(decimal_type != nullptr, "Type must be DecimalType");
+ auto result = decimal.ToString(decimal_type->scale());
+ ICEBERG_CHECK(result, "Decimal ToString failed");
+ return *result;
Review Comment:
```suggestion
const auto& decimal_type = internal::checked_cast<DecimalType>(*type_);
auto decimal = std::get<Decimal>(value_);
return decimal.ToString(decimal_type.scale());
```
##########
src/iceberg/expression/literal.cc:
##########
@@ -193,12 +205,44 @@ std::strong_ordering CompareFloat(T lhs, T rhs) {
return lhs_is_negative <=> rhs_is_negative;
}
+std::strong_ordering CompareDecimal(Literal const& lhs, Literal const& rhs) {
+ ICEBERG_DCHECK(std::holds_alternative<Decimal>(lhs.value()),
+ "LHS of decimal comparison must hold Decimal");
+ ICEBERG_DCHECK(std::holds_alternative<Decimal>(rhs.value()),
+ "RHS of decimal comparison must hold decimal");
+ const auto& lhs_type = std::dynamic_pointer_cast<DecimalType>(lhs.type());
+ const auto& rhs_type = std::dynamic_pointer_cast<DecimalType>(rhs.type());
+ ICEBERG_DCHECK(lhs_type != nullptr, "LHS type must be DecimalType");
+ ICEBERG_DCHECK(rhs_type != nullptr, "RHS type must be DecimalType");
+ auto lhs_decimal = std::get<Decimal>(lhs.value());
+ auto rhs_decimal = std::get<Decimal>(rhs.value());
+ if (lhs_type->scale() == rhs_type->scale()) {
+ return lhs_decimal <=> rhs_decimal;
+ } else if (lhs_type->scale() > rhs_type->scale()) {
+ // Rescale to larger scale
+ auto rhs_res = rhs_decimal.Rescale(rhs_type->scale(), lhs_type->scale());
+ if (!rhs_res) {
+ // Rescale would cause data loss, so lhs is definitely less than rhs
Review Comment:
This is not future-proof thus looks unsafe. It is better to explicitly call
`RescaleWouldCauseDataLoss` to know this, then return unordered if any error
happens after calling `Rescale`.
##########
src/iceberg/util/truncate_util.h:
##########
@@ -19,10 +19,13 @@
#pragma once
+#include <cstdint>
#include <string>
#include <utility>
+#include "iceberg/expression/literal.h"
Review Comment:
Use forward declaration for Literal?
--
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]
