You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "zhjwpku (via GitHub)" <gi...@apache.org> on 2025/07/27 16:00:42 UTC
[PR] feat: implement Literal Transform [iceberg-cpp]
zhjwpku opened a new pull request, #156:
URL: https://github.com/apache/iceberg-cpp/pull/156
(no comment)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2258733759
##########
src/iceberg/expression/literal.cc:
##########
@@ -126,12 +126,23 @@ Literal::Literal(Value value, std::shared_ptr<PrimitiveType> type)
: value_(std::move(value)), type_(std::move(type)) {}
// Factory methods
+
Review Comment:
```suggestion
```
##########
src/iceberg/expression/literal.cc:
##########
@@ -126,12 +126,23 @@ Literal::Literal(Value value, std::shared_ptr<PrimitiveType> type)
: value_(std::move(value)), type_(std::move(type)) {}
// Factory methods
+
Literal Literal::Boolean(bool value) { return {Value{value}, iceberg::boolean()}; }
Literal Literal::Int(int32_t value) { return {Value{value}, iceberg::int32()}; }
+Literal Literal::Date(int32_t value) { return {Value{value}, iceberg::date()}; }
Review Comment:
nit: we don't need `iceberg::` namespace prefix.
##########
src/iceberg/transform_function.h:
##########
@@ -97,11 +97,11 @@ class YearTransform : public TransformFunction {
/// \param source_type Must be a timestamp type.
explicit YearTransform(std::shared_ptr<Type> const& source_type);
- /// \brief Extracts the year from each timestamp in the input array.
- Result<ArrowArray> Transform(const ArrowArray& input) override;
+ /// \brief Extract a date or timestamp year, as years from 1970.
Review Comment:
Also uses `Extracts` to be consistent. Same for below.
##########
src/iceberg/util/truncate_utils.h:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <string>
+#include <utility>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg {
+
+ICEBERG_EXPORT class TruncateUtils {
+ public:
+ /// \brief Truncate a UTF-8 string to a specified number of code points.
+ ///
+ /// \param source The input string to truncate.
+ /// \param L The maximum number of code points allowed in the output string.
+ /// \return A valid UTF-8 string truncated to L code points.
+ /// If the input string is already valid and has fewer than L code points, it is
+ /// returned unchanged.
+ static std::string TruncateUTF8(std::string&& source, size_t L) {
+ size_t code_point_count = 0;
+ size_t safe_point = 0;
+
+ for (size_t i = 0; i < source.size(); ++i) {
+ // Start of a new UTF-8 code point
+ if ((source[i] & 0xC0) != 0x80) {
+ code_point_count++;
+ if (code_point_count > static_cast<size_t>(L)) {
+ safe_point = i;
+ break;
+ }
+ }
+ }
+
+ if (safe_point != 0) {
+ // Resize the string to the safe point
+ source.resize(safe_point);
+ }
+
+ return std::move(source);
+ }
+
+ /// \brief Truncate an integer v, either int32_t or int64_t, to v - (v % W).
+ ///
+ /// The remainder, v % W, must be positive. For languages where % can produce negative
+ /// values, the correct truncate function is: v - (((v % W) + W) % W)
+ template <typename T>
Review Comment:
nit: use concepts to limit T to int32_t and int64_t
##########
src/iceberg/transform_function.h:
##########
@@ -116,11 +116,11 @@ class MonthTransform : public TransformFunction {
/// \param source_type Must be a timestamp type.
explicit MonthTransform(std::shared_ptr<Type> const& source_type);
- /// \brief Extracts the month (1-12) from each timestamp in the input array.
- Result<ArrowArray> Transform(const ArrowArray& input) override;
+ /// \brief Extract a date or timestamp month, as months from 1970-01-01.
Review Comment:
ditto
##########
src/iceberg/transform_function.h:
##########
@@ -97,11 +97,11 @@ class YearTransform : public TransformFunction {
/// \param source_type Must be a timestamp type.
explicit YearTransform(std::shared_ptr<Type> const& source_type);
- /// \brief Extracts the year from each timestamp in the input array.
- Result<ArrowArray> Transform(const ArrowArray& input) override;
+ /// \brief Extract a date or timestamp year, as years from 1970.
Review Comment:
The comment is incorrect.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2257645006
##########
src/iceberg/transform_function.cc:
##########
@@ -420,8 +428,8 @@ Result<std::unique_ptr<TransformFunction>> HourTransform::Make(
VoidTransform::VoidTransform(std::shared_ptr<Type> const& source_type)
: TransformFunction(TransformType::kVoid, source_type) {}
-Result<std::optional<Literal>> VoidTransform::Transform(const Literal& literal) {
- return std::nullopt;
+Result<Literal> VoidTransform::Transform(const Literal& literal) {
+ return Literal::Null();
Review Comment:
Yeah, I read the transform section again and made some changes accordingly.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2251858717
##########
src/iceberg/transform_function.cc:
##########
@@ -91,6 +140,46 @@ Result<ArrowArray> TruncateTransform::Transform(const ArrowArray& input) {
return NotImplemented("TruncateTransform::Transform");
}
+Result<std::optional<Literal>> TruncateTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply truncate transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+
+ switch (source_type()->type_id()) {
+ case TypeId::kInt: {
+ auto value = std::get<int32_t>(literal.value());
+ return Literal::Int(value % width_);
Review Comment:
Double checked rust impl, what you proposed is the right way, changed.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2254800010
##########
test/transform_test.cc:
##########
@@ -193,4 +187,316 @@ TEST(TransformResultTypeTest, NegativeCases) {
}
}
+TEST(TransformLiteralTest, IdentityTransform) {
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::boolean(),
+ .source = Literal::Boolean(true),
+ .expected = Literal::Boolean(true)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(42)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Date(30000)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Long(1234567890)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000000),
+ .expected = Literal::Timestamp(1622547800000000)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000000),
+ .expected = Literal::TimestampTz(1622547800000000)},
+ {.source_type = iceberg::float32(),
+ .source = Literal::Float(3.14),
+ .expected = Literal::Float(3.14)},
+ {.source_type = iceberg::float64(),
+ .source = Literal::Double(1.23e-5),
+ .expected = Literal::Double(1.23e-5)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("Hello, World!"),
+ .expected = Literal::String("Hello, World!")},
+ {.source_type = iceberg::binary(),
+ .source = Literal::Binary({0x01, 0x02, 0x03}),
+ .expected = Literal::Binary({0x01, 0x02, 0x03})},
+ };
+
+ for (const auto& c : cases) {
+ auto transform = Transform::Identity();
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind identity transform";
+
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformLiteralTest, BucketTransform) {
+ constexpr int32_t num_buckets = 4;
+ auto transform = Transform::Bucket(num_buckets);
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::date(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Int(2)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("test"),
+ .expected = Literal::Int(3)},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind bucket transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformLiteralTest, TruncateTransform) {
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ int32_t width;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .width = 5,
+ .source = Literal::Int(123456),
+ .expected = Literal::Int(123455)},
+ {.source_type = iceberg::string(),
+ .width = 5,
+ .source = Literal::String("Hello, World!"),
+ .expected = Literal::String("Hello")},
+ {.source_type = iceberg::string(),
+ .width = 5,
+ .source = Literal::String("😜🧐🤔🤪🥳"),
+ // Truncate to 5 bytes, the safe point should be four bytes which fits the first
Review Comment:
I checked https://en.wikipedia.org/wiki/UTF-8, UTF-8 encodes code points in one to four bytes, the first byte of code point always follow `(value[i] & 0xC0) != 0x80`, so I just do some simple math to resize the string.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2263156741
##########
src/iceberg/transform_function.cc:
##########
@@ -176,11 +318,39 @@ Result<std::unique_ptr<TransformFunction>> MonthTransform::Make(
DayTransform::DayTransform(std::shared_ptr<Type> const& source_type)
: TransformFunction(TransformType::kDay, source_type) {}
-Result<ArrowArray> DayTransform::Transform(const ArrowArray& input) {
- return NotImplemented("DayTransform::Transform");
+Result<Literal> DayTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply day transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+ if (literal.IsNull()) [[unlikely]] {
+ return Literal::Null(iceberg::int32());
Review Comment:
Because the return type should be `int` per the spec [1].
[1] https://iceberg.apache.org/spec/#partition-transforms
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#issuecomment-3170547982
> Perhaps we can assert return literal type is equal to result type?
That would require introducing a temporary variable before the return statement. I think if we have thorough test cases, the assert might not be necessary.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2249924107
##########
src/iceberg/util/macros.h:
##########
@@ -36,3 +36,8 @@
#define ICEBERG_ASSIGN_OR_RAISE(lhs, rexpr) \
ICEBERG_ASSIGN_OR_RAISE_IMPL(ICEBERG_ASSIGN_OR_RAISE_NAME(result_, __COUNTER__), lhs, \
rexpr)
+
+#define NANOARROW_RETURN_IF_NOT_OK(status, error) \
+ if (status != NANOARROW_OK) [[unlikely]] { \
Review Comment:
Yeah, I move it back :)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2249923932
##########
src/iceberg/expression/literal.h:
##########
@@ -85,6 +88,9 @@ class ICEBERG_EXPORT Literal {
/// \brief Get the literal type.
const std::shared_ptr<PrimitiveType>& type() const;
+ /// \brief Get the literal value.
+ const Value& value() const { return value_; }
Review Comment:
Reasonable, done.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2262747242
##########
src/iceberg/util/truncate_utils.h:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <string>
+#include <utility>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg {
+
+ICEBERG_EXPORT class TruncateUtils {
+ public:
+ /// \brief Truncate a UTF-8 string to a specified number of code points.
+ ///
+ /// \param source The input string to truncate.
+ /// \param L The maximum number of code points allowed in the output string.
+ /// \return A valid UTF-8 string truncated to L code points.
+ /// If the input string is already valid and has fewer than L code points, it is
+ /// returned unchanged.
+ static std::string TruncateUTF8(std::string&& source, size_t L) {
Review Comment:
Why not
```
static std::string TruncateUTF8(std::string source, size_t L) {
```
##########
src/iceberg/expression/literal.h:
##########
@@ -48,27 +48,35 @@ class ICEBERG_EXPORT Literal {
bool operator==(const AboveMax&) const = default;
std::strong_ordering operator<=>(const AboveMax&) const = default;
};
-
- using Value = std::variant<bool, // for boolean
- int32_t, // for int, date
- int64_t, // for long, timestamp, timestamp_tz, time
- float, // for float
- double, // for double
- std::string, // for string
+ using Value = std::variant<std::monostate, // for null
Review Comment:
Previously I may think this only present the literal value, but this is also ok to me
##########
src/iceberg/transform_function.cc:
##########
@@ -87,14 +129,52 @@ TruncateTransform::TruncateTransform(std::shared_ptr<Type> const& source_type,
int32_t width)
: TransformFunction(TransformType::kTruncate, source_type), width_(width) {}
-Result<ArrowArray> TruncateTransform::Transform(const ArrowArray& input) {
- return NotImplemented("TruncateTransform::Transform");
-}
+Result<Literal> TruncateTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply truncate transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+ if (literal.IsNull()) [[unlikely]] {
+ // Return null as is
+ return literal;
+ }
-Result<std::shared_ptr<Type>> TruncateTransform::ResultType() const {
- return source_type();
+ switch (source_type()->type_id()) {
+ case TypeId::kInt: {
+ auto value = std::get<int32_t>(literal.value());
+ return Literal::Int(TruncateUtils::TruncateInteger(value, width_));
+ }
+ case TypeId::kLong: {
+ auto value = std::get<int64_t>(literal.value());
+ return Literal::Long(TruncateUtils::TruncateInteger(value, width_));
+ }
+ case TypeId::kDecimal: {
+ // TODO(zhjwpku): Handle decimal truncation logic here
+ return NotImplemented("Truncate for Decimal is not implemented yet");
+ }
+ case TypeId::kString: {
+ // Strings are truncated to a valid UTF-8 string with no more than L code points.
+ auto value = std::get<std::string>(literal.value());
+ return Literal::String(TruncateUtils::TruncateUTF8(std::move(value), width_));
+ }
+ case TypeId::kBinary: {
+ /// In contrast to strings, binary values do not have an assumed encoding and are
+ /// truncated to L bytes.
+ auto value = std::get<std::vector<uint8_t>>(literal.value());
+ if (value.size() > static_cast<size_t>(width_)) {
+ value.resize(width_);
+ }
+ return Literal::Binary(value);
Review Comment:
```suggestion
return Literal::Binary(std::move(value));
```
##########
src/iceberg/transform_function.cc:
##########
@@ -176,11 +318,39 @@ Result<std::unique_ptr<TransformFunction>> MonthTransform::Make(
DayTransform::DayTransform(std::shared_ptr<Type> const& source_type)
: TransformFunction(TransformType::kDay, source_type) {}
-Result<ArrowArray> DayTransform::Transform(const ArrowArray& input) {
- return NotImplemented("DayTransform::Transform");
+Result<Literal> DayTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply day transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+ if (literal.IsNull()) [[unlikely]] {
+ return Literal::Null(iceberg::int32());
Review Comment:
why not date here?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2265094924
##########
src/iceberg/util/truncate_utils.h:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <string>
+#include <utility>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg {
+
+ICEBERG_EXPORT class TruncateUtils {
Review Comment:
Ok, I found that upgrade https://github.com/pre-commit/mirrors-clang-format to v20.1.8 works, so changed to the suggested approach, thanks.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2249939683
##########
src/iceberg/transform_function.cc:
##########
@@ -51,6 +60,57 @@ Result<ArrowArray> BucketTransform::Transform(const ArrowArray& input) {
return NotImplemented("BucketTransform::Transform");
}
+Result<std::optional<Literal>> BucketTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply bucket transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+ int32_t hash_value = 0;
+ switch (source_type()->type_id()) {
Review Comment:
I've implemented this idea in `BucketTransform::Transform`. Since we're not handling all of the variant's alternative types, I added a branch with std::unreachable(). I'm not sure if this is the best approach, please take another look when you have a chance. @lidavidm @mapleFU @wgtmac
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2234342526
##########
src/iceberg/transform_function.cc:
##########
@@ -51,6 +60,57 @@ Result<ArrowArray> BucketTransform::Transform(const ArrowArray& input) {
return NotImplemented("BucketTransform::Transform");
}
+Result<std::optional<Literal>> BucketTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply bucket transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+ int32_t hash_value = 0;
+ switch (source_type()->type_id()) {
Review Comment:
Seems `literal.type() == source_type()` is enforced?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2234191106
##########
src/iceberg/transform_function.cc:
##########
@@ -51,6 +60,57 @@ Result<ArrowArray> BucketTransform::Transform(const ArrowArray& input) {
return NotImplemented("BucketTransform::Transform");
}
+Result<std::optional<Literal>> BucketTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply bucket transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+ int32_t hash_value = 0;
+ switch (source_type()->type_id()) {
Review Comment:
Since we don't actually care about the type, you could `std::visit` the `value()` instead?
##########
test/transform_test.cc:
##########
@@ -193,4 +193,296 @@ TEST(TransformResultTypeTest, NegativeCases) {
}
}
+TEST(TransformFunctionTransformTest, IdentityTransform) {
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::boolean(),
+ .source = Literal::Boolean(true),
+ .expected = Literal::Boolean(true)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(42)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Date(30000)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Long(1234567890)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000),
+ .expected = Literal::Timestamp(1622547800000)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000),
+ .expected = Literal::TimestampTz(1622547800000)},
+ {.source_type = iceberg::float32(),
+ .source = Literal::Float(3.14),
+ .expected = Literal::Float(3.14)},
+ {.source_type = iceberg::float64(),
+ .source = Literal::Double(2.71828),
+ .expected = Literal::Double(2.71828)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("Hello, World!"),
+ .expected = Literal::String("Hello, World!")},
+ {.source_type = iceberg::binary(),
+ .source = Literal::Binary({0x01, 0x02, 0x03}),
+ .expected = Literal::Binary({0x01, 0x02, 0x03})},
+ };
+
+ for (const auto& c : cases) {
+ auto transform = Transform::Identity();
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind identity transform";
+
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, BucketTransform) {
+ constexpr int32_t num_buckets = 4;
+ auto transform = Transform::Bucket(num_buckets);
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::date(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Int(2)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("test"),
+ .expected = Literal::Int(3)},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind bucket transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, TruncateTransform) {
+ constexpr int32_t width = 5;
+ auto transform = Transform::Truncate(width);
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(123456),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("Hello, World!"),
+ .expected = Literal::String("Hello")},
+ {.source_type = iceberg::binary(),
+ .source = Literal::Binary({0x01, 0x02, 0x03, 0x04, 0x05, 0x06}),
+ .expected = Literal::Binary({0x01, 0x02, 0x03, 0x04, 0x05})},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind truncate transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, YearTransform) {
+ auto transform = Transform::Year();
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000),
+ .expected = Literal::Int(2021)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000),
+ .expected = Literal::Int(2021)},
+ {.source_type = iceberg::date(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Int(2052)},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind year transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, MonthTransform) {
+ auto transform = Transform::Month();
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000),
+ .expected = Literal::Int(617)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000),
+ .expected = Literal::Int(617)},
+ {.source_type = iceberg::date(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Int(985)},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind month transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, DayTransform) {
+ auto transform = Transform::Day();
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000),
+ .expected = Literal::Date(18779)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000),
+ .expected = Literal::Date(18779)},
+ {.source_type = iceberg::date(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Date(30000)},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind day transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, HourTransform) {
+ auto transform = Transform::Hour();
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000),
+ .expected = Literal::Int(450707)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000),
+ .expected = Literal::Int(450707)},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind hour transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, VoidTransform) {
+ auto transform = Transform::Void();
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::boolean(), .source = Literal::Boolean(true)},
+ {.source_type = iceberg::int32(), .source = Literal::Int(42)},
+ {.source_type = iceberg::int32(), .source = Literal::Date(30000)},
+ {.source_type = iceberg::int64(), .source = Literal::Long(1234567890)},
+ {.source_type = iceberg::timestamp(), .source = Literal::Timestamp(1622547800000)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000)},
+ {.source_type = iceberg::float32(), .source = Literal::Float(3.14)},
+ {.source_type = iceberg::float64(), .source = Literal::Double(2.71828)},
+ {.source_type = iceberg::string(), .source = Literal::String("Hello, World!")},
+ {.source_type = iceberg::binary(), .source = Literal::Binary({0x01, 0x02, 0x03})},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind void transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result == std::nullopt)
Review Comment:
nit, but gtest gives better failure messages if you use `ASSERT_EQ(std::nullopt, result);` for this kind of thing
##########
src/iceberg/transform_function.cc:
##########
@@ -124,6 +224,34 @@ Result<ArrowArray> YearTransform::Transform(const ArrowArray& input) {
return NotImplemented("YearTransform::Transform");
}
+Result<std::optional<Literal>> YearTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply year transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+
+ using namespace std::chrono;
+ switch (source_type()->type_id()) {
+ case TypeId::kDate: {
+ auto value = std::get<int32_t>(literal.value());
+ auto epoch = sys_days(year{1970} / January / 1);
+ auto ymd = year_month_day(epoch + days{value});
+ return Literal::Int(static_cast<int32_t>(ymd.year()));
+ }
+ case TypeId::kTimestamp:
+ case TypeId::kTimestampTz: {
+ auto value = std::get<int64_t>(literal.value());
+ // Convert milliseconds-since-epoch into a `year_month_day` object
Review Comment:
Aren't timestamps in microseconds?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#issuecomment-3167406604
@Fokko @zeroshade Could you help review this? Thanks!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2249923817
##########
src/iceberg/transform.h:
##########
@@ -172,6 +174,8 @@ class ICEBERG_EXPORT TransformFunction {
TransformFunction(TransformType transform_type, std::shared_ptr<Type> source_type);
/// \brief Transform an input array to a new array
virtual Result<ArrowArray> Transform(const ArrowArray& data) = 0;
Review Comment:
I'm not sure this is a good API, since it requires constructing ArrowSchema on the fly. I think multi-args transforms could be handled in other ways, I'll leave it as is for now.
##########
src/iceberg/expression/literal.h:
##########
@@ -63,7 +63,10 @@ class ICEBERG_EXPORT Literal {
/// \brief Factory methods for primitive types
static Literal Boolean(bool value);
static Literal Int(int32_t value);
+ static Literal Date(int32_t value);
static Literal Long(int64_t value);
+ static Literal Timestamp(int64_t value);
Review Comment:
done
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2236887137
##########
src/iceberg/transform_function.cc:
##########
@@ -51,6 +60,57 @@ Result<ArrowArray> BucketTransform::Transform(const ArrowArray& input) {
return NotImplemented("BucketTransform::Transform");
}
+Result<std::optional<Literal>> BucketTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply bucket transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+ int32_t hash_value = 0;
+ switch (source_type()->type_id()) {
Review Comment:
Ok, I will try this later, thanks for the input.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2260487542
##########
src/iceberg/transform_function.h:
##########
@@ -97,11 +97,11 @@ class YearTransform : public TransformFunction {
/// \param source_type Must be a timestamp type.
explicit YearTransform(std::shared_ptr<Type> const& source_type);
- /// \brief Extracts the year from each timestamp in the input array.
- Result<ArrowArray> Transform(const ArrowArray& input) override;
+ /// \brief Extract a date or timestamp year, as years from 1970.
Review Comment:
iceberg-rust use the same description[1], so I'm inclined to keep it consistent.
[1]: https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/transform/temporal.rs#L44
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2265093063
##########
src/iceberg/util/truncate_utils.h:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <string>
+#include <utility>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg {
+
+ICEBERG_EXPORT class TruncateUtils {
Review Comment:
I initially used this approach, but the pre-commit clang-format hook misformatted the file, so I followed the StringUtils approach instead.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2264677423
##########
src/iceberg/util/truncate_utils.h:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.
+ */
+
+#pragma once
+
+#include <string>
+#include <utility>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg {
+
+ICEBERG_EXPORT class TruncateUtils {
Review Comment:
```suggestion
class ICEBERG_EXPORT TruncateUtils {
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2259339955
##########
src/iceberg/transform_function.h:
##########
@@ -97,11 +97,11 @@ class YearTransform : public TransformFunction {
/// \param source_type Must be a timestamp type.
explicit YearTransform(std::shared_ptr<Type> const& source_type);
- /// \brief Extracts the year from each timestamp in the input array.
- Result<ArrowArray> Transform(const ArrowArray& input) override;
+ /// \brief Extract a date or timestamp year, as years from 1970.
Review Comment:
I agree the comment is not clear, but that's from the spec, see [1]. We might also want to change the spec if it's not clear.
[1] https://iceberg.apache.org/spec/#partition-transforms
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2236885010
##########
test/transform_test.cc:
##########
@@ -193,4 +193,296 @@ TEST(TransformResultTypeTest, NegativeCases) {
}
}
+TEST(TransformFunctionTransformTest, IdentityTransform) {
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::boolean(),
+ .source = Literal::Boolean(true),
+ .expected = Literal::Boolean(true)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(42)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Date(30000)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Long(1234567890)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000),
+ .expected = Literal::Timestamp(1622547800000)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000),
+ .expected = Literal::TimestampTz(1622547800000)},
+ {.source_type = iceberg::float32(),
+ .source = Literal::Float(3.14),
+ .expected = Literal::Float(3.14)},
+ {.source_type = iceberg::float64(),
+ .source = Literal::Double(2.71828),
+ .expected = Literal::Double(2.71828)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("Hello, World!"),
+ .expected = Literal::String("Hello, World!")},
+ {.source_type = iceberg::binary(),
+ .source = Literal::Binary({0x01, 0x02, 0x03}),
+ .expected = Literal::Binary({0x01, 0x02, 0x03})},
+ };
+
+ for (const auto& c : cases) {
+ auto transform = Transform::Identity();
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind identity transform";
+
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, BucketTransform) {
+ constexpr int32_t num_buckets = 4;
+ auto transform = Transform::Bucket(num_buckets);
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::date(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Int(2)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("test"),
+ .expected = Literal::Int(3)},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind bucket transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, TruncateTransform) {
+ constexpr int32_t width = 5;
+ auto transform = Transform::Truncate(width);
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(123456),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("Hello, World!"),
+ .expected = Literal::String("Hello")},
+ {.source_type = iceberg::binary(),
+ .source = Literal::Binary({0x01, 0x02, 0x03, 0x04, 0x05, 0x06}),
+ .expected = Literal::Binary({0x01, 0x02, 0x03, 0x04, 0x05})},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind truncate transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, YearTransform) {
+ auto transform = Transform::Year();
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000),
+ .expected = Literal::Int(2021)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000),
+ .expected = Literal::Int(2021)},
+ {.source_type = iceberg::date(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Int(2052)},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind year transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, MonthTransform) {
+ auto transform = Transform::Month();
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000),
+ .expected = Literal::Int(617)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000),
+ .expected = Literal::Int(617)},
+ {.source_type = iceberg::date(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Int(985)},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind month transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, DayTransform) {
+ auto transform = Transform::Day();
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000),
+ .expected = Literal::Date(18779)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000),
+ .expected = Literal::Date(18779)},
+ {.source_type = iceberg::date(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Date(30000)},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind day transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, HourTransform) {
+ auto transform = Transform::Hour();
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000),
+ .expected = Literal::Int(450707)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000),
+ .expected = Literal::Int(450707)},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind hour transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, VoidTransform) {
+ auto transform = Transform::Void();
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::boolean(), .source = Literal::Boolean(true)},
+ {.source_type = iceberg::int32(), .source = Literal::Int(42)},
+ {.source_type = iceberg::int32(), .source = Literal::Date(30000)},
+ {.source_type = iceberg::int64(), .source = Literal::Long(1234567890)},
+ {.source_type = iceberg::timestamp(), .source = Literal::Timestamp(1622547800000)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000)},
+ {.source_type = iceberg::float32(), .source = Literal::Float(3.14)},
+ {.source_type = iceberg::float64(), .source = Literal::Double(2.71828)},
+ {.source_type = iceberg::string(), .source = Literal::String("Hello, World!")},
+ {.source_type = iceberg::binary(), .source = Literal::Binary({0x01, 0x02, 0x03})},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind void transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result == std::nullopt)
Review Comment:
fixed.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2236884274
##########
src/iceberg/transform_function.cc:
##########
@@ -124,6 +224,34 @@ Result<ArrowArray> YearTransform::Transform(const ArrowArray& input) {
return NotImplemented("YearTransform::Transform");
}
+Result<std::optional<Literal>> YearTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply year transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+
+ using namespace std::chrono;
+ switch (source_type()->type_id()) {
+ case TypeId::kDate: {
+ auto value = std::get<int32_t>(literal.value());
+ auto epoch = sys_days(year{1970} / January / 1);
+ auto ymd = year_month_day(epoch + days{value});
+ return Literal::Int(static_cast<int32_t>(ymd.year()));
+ }
+ case TypeId::kTimestamp:
+ case TypeId::kTimestampTz: {
+ auto value = std::get<int64_t>(literal.value());
+ // Convert milliseconds-since-epoch into a `year_month_day` object
Review Comment:
You are right, fixed.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2249924228
##########
src/iceberg/expression/literal.cc:
##########
@@ -208,12 +216,30 @@ std::partial_ordering Literal::operator<=>(const Literal& other) const {
return this_val <=> other_val;
}
+ case TypeId::kDate: {
Review Comment:
Good catch, done that way.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2251729713
##########
src/iceberg/transform_function.cc:
##########
@@ -51,6 +61,45 @@ Result<ArrowArray> BucketTransform::Transform(const ArrowArray& input) {
return NotImplemented("BucketTransform::Transform");
}
+Result<std::optional<Literal>> BucketTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
Review Comment:
Agreed, we may incorporate this alongside our logging infrastructure.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2251860669
##########
test/transform_test.cc:
##########
@@ -193,4 +193,297 @@ TEST(TransformResultTypeTest, NegativeCases) {
}
}
+TEST(TransformFunctionTransformTest, IdentityTransform) {
Review Comment:
done
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2251855818
##########
src/iceberg/transform_function.cc:
##########
@@ -91,6 +140,46 @@ Result<ArrowArray> TruncateTransform::Transform(const ArrowArray& input) {
return NotImplemented("TruncateTransform::Transform");
}
+Result<std::optional<Literal>> TruncateTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply truncate transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+
+ switch (source_type()->type_id()) {
+ case TypeId::kInt: {
+ auto value = std::get<int32_t>(literal.value());
+ return Literal::Int(value % width_);
+ }
+ case TypeId::kLong: {
+ auto value = std::get<int64_t>(literal.value());
+ return Literal::Long(value % width_);
+ }
+ case TypeId::kDecimal: {
+ // TODO(zhjwpku): Handle decimal truncation logic here
+ return NotImplemented("Truncate for Decimal is not implemented yet");
+ }
+ case TypeId::kString: {
+ auto value = std::get<std::string>(literal.value());
+ if (value.size() > static_cast<size_t>(width_)) {
+ value.resize(width_);
Review Comment:
Good catch, I didn't realize that before, please check again if I'm doing it right now.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2255686870
##########
test/transform_test.cc:
##########
@@ -24,6 +24,7 @@
#include <gmock/gmock.h>
#include <gtest/gtest.h>
+#include <iceberg/expression/literal.h>
Review Comment:
```suggestion
#include "iceberg/expression/literal.h"
```
##########
src/iceberg/type_fwd.h:
##########
@@ -36,6 +36,8 @@ enum class TypeId {
kStruct,
kList,
kMap,
+ kNull, // Note: A type having no physical storage. This is not an iceberg type, we add
Review Comment:
See my comment above.
##########
src/iceberg/transform_function.cc:
##########
@@ -420,8 +428,8 @@ Result<std::unique_ptr<TransformFunction>> HourTransform::Make(
VoidTransform::VoidTransform(std::shared_ptr<Type> const& source_type)
: TransformFunction(TransformType::kVoid, source_type) {}
-Result<std::optional<Literal>> VoidTransform::Transform(const Literal& literal) {
- return std::nullopt;
+Result<Literal> VoidTransform::Transform(const Literal& literal) {
+ return Literal::Null();
Review Comment:
Per the spec, `All transforms must return `null` for a `null` input value`. We need to check if the input literal is a null for every kind of transform.
##########
src/iceberg/transform_function.cc:
##########
@@ -150,17 +150,25 @@ Result<std::optional<Literal>> TruncateTransform::Transform(const Literal& liter
return NotImplemented("Truncate for Decimal is not implemented yet");
}
case TypeId::kString: {
+ // Strings are truncated to a valid UTF-8 string with no more than L code points.
auto value = std::get<std::string>(literal.value());
- if (value.size() > static_cast<size_t>(width_)) {
- size_t safe_point = width_;
- while (safe_point > 0 && (value[safe_point] & 0xC0) == 0x80) {
- // Find the last valid UTF-8 character boundary before or at width_
- safe_point--;
+ size_t code_point_count = 0;
+ size_t safe_point = 0;
+
+ for (size_t i = 0; i < value.size(); ++i) {
+ // Start of a new UTF-8 code point
+ if ((value[i] & 0xC0) != 0x80) {
Review Comment:
nit: add a `iceberg/util/truncate_util.h` so we can test individual functions (utf8, integer, decimal, etc.) better without the boilerplate to create literal and transform.
##########
src/iceberg/type.h:
##########
@@ -191,6 +191,23 @@ class ICEBERG_EXPORT MapType : public NestedType {
/// Primitive types do not have nested fields.
/// @{
+/// \brief A data type that has no physical storage.
+/// Technically, this is a primitive type, we treat it as a primitive type for
+/// convenience.
+class ICEBERG_EXPORT NullType : public PrimitiveType {
Review Comment:
I don't think adding `NullType` and `TypeId::kNull` is the right path forward. It is confusing since it does not exist in the spec and may conflict with the purpose of `UnknownType` in v3. I'd suggest to revert changes related to this new type.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2260487542
##########
src/iceberg/transform_function.h:
##########
@@ -97,11 +97,11 @@ class YearTransform : public TransformFunction {
/// \param source_type Must be a timestamp type.
explicit YearTransform(std::shared_ptr<Type> const& source_type);
- /// \brief Extracts the year from each timestamp in the input array.
- Result<ArrowArray> Transform(const ArrowArray& input) override;
+ /// \brief Extract a date or timestamp year, as years from 1970.
Review Comment:
iceberg-rust use the same description[1], so I'm inclined to keep it consistent.
[1] https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/transform/temporal.rs#L44
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2234367958
##########
src/iceberg/transform_function.cc:
##########
@@ -51,6 +60,57 @@ Result<ArrowArray> BucketTransform::Transform(const ArrowArray& input) {
return NotImplemented("BucketTransform::Transform");
}
+Result<std::optional<Literal>> BucketTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply bucket transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+ int32_t hash_value = 0;
+ switch (source_type()->type_id()) {
Review Comment:
Right, but in this particular case, the "logical" type doesn't matter, right? The only thing that matters is the "physical" data type, so you could use std::visit + static_assert to enforce that all branches are taken without having to list out each Iceberg type
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2257646368
##########
src/iceberg/transform_function.cc:
##########
@@ -150,17 +150,25 @@ Result<std::optional<Literal>> TruncateTransform::Transform(const Literal& liter
return NotImplemented("Truncate for Decimal is not implemented yet");
}
case TypeId::kString: {
+ // Strings are truncated to a valid UTF-8 string with no more than L code points.
auto value = std::get<std::string>(literal.value());
- if (value.size() > static_cast<size_t>(width_)) {
- size_t safe_point = width_;
- while (safe_point > 0 && (value[safe_point] & 0xC0) == 0x80) {
- // Find the last valid UTF-8 character boundary before or at width_
- safe_point--;
+ size_t code_point_count = 0;
+ size_t safe_point = 0;
+
+ for (size_t i = 0; i < value.size(); ++i) {
+ // Start of a new UTF-8 code point
+ if ((value[i] & 0xC0) != 0x80) {
Review Comment:
done.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2250886955
##########
src/iceberg/transform_function.cc:
##########
@@ -51,6 +61,45 @@ Result<ArrowArray> BucketTransform::Transform(const ArrowArray& input) {
return NotImplemented("BucketTransform::Transform");
}
+Result<std::optional<Literal>> BucketTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
Review Comment:
Not related to this PR: we should introduce something like `ARROW_DCHECK` to add some explicit debug-level checks.
##########
src/iceberg/transform_function.cc:
##########
@@ -91,6 +140,46 @@ Result<ArrowArray> TruncateTransform::Transform(const ArrowArray& input) {
return NotImplemented("TruncateTransform::Transform");
}
+Result<std::optional<Literal>> TruncateTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply truncate transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+
+ switch (source_type()->type_id()) {
+ case TypeId::kInt: {
+ auto value = std::get<int32_t>(literal.value());
+ return Literal::Int(value % width_);
Review Comment:
This looks incorrect because it is `v - (v % W)` per the spec. We should be consistent with the Java impl:
```java
public static int truncateInt(int width, int value) {
return value - (((value % width) + width) % width);
}
public static long truncateLong(int width, long value) {
return value - (((value % width) + width) % width);
}
```
##########
test/transform_test.cc:
##########
@@ -193,4 +193,297 @@ TEST(TransformResultTypeTest, NegativeCases) {
}
}
+TEST(TransformFunctionTransformTest, IdentityTransform) {
Review Comment:
`TransformFunctionTransformTest` -> `TransformLiteralTest` ?
Let's make it short...
##########
test/transform_test.cc:
##########
@@ -193,4 +193,297 @@ TEST(TransformResultTypeTest, NegativeCases) {
}
}
+TEST(TransformFunctionTransformTest, IdentityTransform) {
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::boolean(),
+ .source = Literal::Boolean(true),
+ .expected = Literal::Boolean(true)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(42)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Date(30000)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Long(1234567890)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000000),
+ .expected = Literal::Timestamp(1622547800000000)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000000),
+ .expected = Literal::TimestampTz(1622547800000000)},
+ {.source_type = iceberg::float32(),
+ .source = Literal::Float(3.14),
+ .expected = Literal::Float(3.14)},
+ {.source_type = iceberg::float64(),
+ .source = Literal::Double(1.23e-5),
+ .expected = Literal::Double(1.23e-5)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("Hello, World!"),
+ .expected = Literal::String("Hello, World!")},
+ {.source_type = iceberg::binary(),
+ .source = Literal::Binary({0x01, 0x02, 0x03}),
+ .expected = Literal::Binary({0x01, 0x02, 0x03})},
+ };
+
+ for (const auto& c : cases) {
+ auto transform = Transform::Identity();
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind identity transform";
+
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, BucketTransform) {
+ constexpr int32_t num_buckets = 4;
+ auto transform = Transform::Bucket(num_buckets);
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::date(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Int(2)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("test"),
+ .expected = Literal::Int(3)},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind bucket transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, TruncateTransform) {
+ constexpr int32_t width = 5;
+ auto transform = Transform::Truncate(width);
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(123456),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("Hello, World!"),
+ .expected = Literal::String("Hello")},
+ {.source_type = iceberg::binary(),
+ .source = Literal::Binary({0x01, 0x02, 0x03, 0x04, 0x05, 0x06}),
+ .expected = Literal::Binary({0x01, 0x02, 0x03, 0x04, 0x05})},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind truncate transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, YearTransform) {
+ auto transform = Transform::Year();
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000000),
Review Comment:
nit: add a comment to print human readable timestamp values.
##########
src/iceberg/transform_function.cc:
##########
@@ -91,6 +140,46 @@ Result<ArrowArray> TruncateTransform::Transform(const ArrowArray& input) {
return NotImplemented("TruncateTransform::Transform");
}
+Result<std::optional<Literal>> TruncateTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply truncate transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+
+ switch (source_type()->type_id()) {
+ case TypeId::kInt: {
+ auto value = std::get<int32_t>(literal.value());
+ return Literal::Int(value % width_);
+ }
+ case TypeId::kLong: {
+ auto value = std::get<int64_t>(literal.value());
+ return Literal::Long(value % width_);
+ }
+ case TypeId::kDecimal: {
+ // TODO(zhjwpku): Handle decimal truncation logic here
+ return NotImplemented("Truncate for Decimal is not implemented yet");
+ }
+ case TypeId::kString: {
+ auto value = std::get<std::string>(literal.value());
+ if (value.size() > static_cast<size_t>(width_)) {
+ value.resize(width_);
Review Comment:
This is not correct. We need to handle UTF8 code point.
##########
src/iceberg/transform.h:
##########
@@ -172,6 +174,8 @@ class ICEBERG_EXPORT TransformFunction {
TransformFunction(TransformType transform_type, std::shared_ptr<Type> source_type);
/// \brief Transform an input array to a new array
virtual Result<ArrowArray> Transform(const ArrowArray& data) = 0;
Review Comment:
I have to admit that `Result<ArrowArray> Transform(const ArrowArray& data)` is a bad design. It was added at the time when we don't have `Literal` yet. We can safely remove it since it is no longer needed.
##########
test/transform_test.cc:
##########
@@ -193,4 +193,297 @@ TEST(TransformResultTypeTest, NegativeCases) {
}
}
+TEST(TransformFunctionTransformTest, IdentityTransform) {
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::boolean(),
+ .source = Literal::Boolean(true),
+ .expected = Literal::Boolean(true)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(42)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Date(30000)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Long(1234567890)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000000),
+ .expected = Literal::Timestamp(1622547800000000)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000000),
+ .expected = Literal::TimestampTz(1622547800000000)},
+ {.source_type = iceberg::float32(),
+ .source = Literal::Float(3.14),
+ .expected = Literal::Float(3.14)},
+ {.source_type = iceberg::float64(),
+ .source = Literal::Double(1.23e-5),
+ .expected = Literal::Double(1.23e-5)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("Hello, World!"),
+ .expected = Literal::String("Hello, World!")},
+ {.source_type = iceberg::binary(),
+ .source = Literal::Binary({0x01, 0x02, 0x03}),
+ .expected = Literal::Binary({0x01, 0x02, 0x03})},
+ };
+
+ for (const auto& c : cases) {
+ auto transform = Transform::Identity();
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind identity transform";
+
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, BucketTransform) {
+ constexpr int32_t num_buckets = 4;
+ auto transform = Transform::Bucket(num_buckets);
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::date(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Int(2)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("test"),
+ .expected = Literal::Int(3)},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind bucket transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, TruncateTransform) {
+ constexpr int32_t width = 5;
+ auto transform = Transform::Truncate(width);
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(123456),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("Hello, World!"),
Review Comment:
Please add some non-ASCII UTF8 strings.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2253515467
##########
src/iceberg/transform.h:
##########
@@ -170,8 +172,8 @@ class ICEBERG_EXPORT TransformFunction {
public:
virtual ~TransformFunction() = default;
TransformFunction(TransformType transform_type, std::shared_ptr<Type> source_type);
- /// \brief Transform an input array to a new array
- virtual Result<ArrowArray> Transform(const ArrowArray& data) = 0;
+ /// \brief Transform an input Literal to a new Literal
+ virtual Result<std::optional<Literal>> Transform(const Literal& literal) = 0;
Review Comment:
Does it make sense to introduce a `void` variant to Literal? It does not deserve to pay for an extra optional value check every time when the void transform is a rare case.
##########
test/transform_test.cc:
##########
@@ -193,4 +187,316 @@ TEST(TransformResultTypeTest, NegativeCases) {
}
}
+TEST(TransformLiteralTest, IdentityTransform) {
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::boolean(),
+ .source = Literal::Boolean(true),
+ .expected = Literal::Boolean(true)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(42)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Date(30000)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Long(1234567890)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000000),
+ .expected = Literal::Timestamp(1622547800000000)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000000),
+ .expected = Literal::TimestampTz(1622547800000000)},
+ {.source_type = iceberg::float32(),
+ .source = Literal::Float(3.14),
+ .expected = Literal::Float(3.14)},
+ {.source_type = iceberg::float64(),
+ .source = Literal::Double(1.23e-5),
+ .expected = Literal::Double(1.23e-5)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("Hello, World!"),
+ .expected = Literal::String("Hello, World!")},
+ {.source_type = iceberg::binary(),
+ .source = Literal::Binary({0x01, 0x02, 0x03}),
+ .expected = Literal::Binary({0x01, 0x02, 0x03})},
+ };
+
+ for (const auto& c : cases) {
+ auto transform = Transform::Identity();
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind identity transform";
+
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformLiteralTest, BucketTransform) {
+ constexpr int32_t num_buckets = 4;
+ auto transform = Transform::Bucket(num_buckets);
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::date(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Int(2)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("test"),
+ .expected = Literal::Int(3)},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind bucket transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformLiteralTest, TruncateTransform) {
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ int32_t width;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .width = 5,
+ .source = Literal::Int(123456),
+ .expected = Literal::Int(123455)},
+ {.source_type = iceberg::string(),
+ .width = 5,
+ .source = Literal::String("Hello, World!"),
+ .expected = Literal::String("Hello")},
+ {.source_type = iceberg::string(),
+ .width = 5,
+ .source = Literal::String("😜🧐🤔🤪🥳"),
+ // Truncate to 5 bytes, the safe point should be four bytes which fits the first
Review Comment:
No, truncation should be based on the number of unicode codepoints (a.k.a. emoji symbols) not the number of characters. Please take a look at https://github.com/apache/iceberg/blob/acecf3cb697a7fa6bfcb1b58fc5d7d70914436e7/api/src/main/java/org/apache/iceberg/util/UnicodeUtil.java#L41-L55
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2254789352
##########
src/iceberg/transform.h:
##########
@@ -170,8 +172,8 @@ class ICEBERG_EXPORT TransformFunction {
public:
virtual ~TransformFunction() = default;
TransformFunction(TransformType transform_type, std::shared_ptr<Type> source_type);
- /// \brief Transform an input array to a new array
- virtual Result<ArrowArray> Transform(const ArrowArray& data) = 0;
+ /// \brief Transform an input Literal to a new Literal
+ virtual Result<std::optional<Literal>> Transform(const Literal& literal) = 0;
Review Comment:
I add a NullType and Literal::Null(), I use `Null` because spec says void transform `Always produces null`.
NullType is inherited from PrimitiveType because Literal holds a PrimitiveType shared ptr.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2264199334
##########
src/iceberg/transform_function.cc:
##########
@@ -176,11 +318,39 @@ Result<std::unique_ptr<TransformFunction>> MonthTransform::Make(
DayTransform::DayTransform(std::shared_ptr<Type> const& source_type)
: TransformFunction(TransformType::kDay, source_type) {}
-Result<ArrowArray> DayTransform::Transform(const ArrowArray& input) {
- return NotImplemented("DayTransform::Transform");
+Result<Literal> DayTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply day transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+ if (literal.IsNull()) [[unlikely]] {
+ return Literal::Null(iceberg::int32());
Review Comment:
Ah, my oversight, updated to `int` and adjusted the corresponding test case.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2262462751
##########
src/iceberg/transform_function.cc:
##########
@@ -148,14 +253,51 @@ Result<std::unique_ptr<TransformFunction>> YearTransform::Make(
MonthTransform::MonthTransform(std::shared_ptr<Type> const& source_type)
: TransformFunction(TransformType::kMonth, source_type) {}
-Result<ArrowArray> MonthTransform::Transform(const ArrowArray& input) {
- return NotImplemented("MonthTransform::Transform");
-}
+Result<Literal> MonthTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply month transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+ if (literal.IsNull()) [[unlikely]] {
+ return Literal::Null(iceberg::int32());
+ }
-Result<std::shared_ptr<Type>> MonthTransform::ResultType() const {
- return iceberg::int32();
+ using namespace std::chrono; // NOLINT
+ switch (source_type()->type_id()) {
+ case TypeId::kDate: {
+ auto value = std::get<int32_t>(literal.value());
Review Comment:
Good idea.
https://github.com/apache/iceberg-cpp/issues/164
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2257636948
##########
src/iceberg/type.h:
##########
@@ -191,6 +191,23 @@ class ICEBERG_EXPORT MapType : public NestedType {
/// Primitive types do not have nested fields.
/// @{
+/// \brief A data type that has no physical storage.
+/// Technically, this is a primitive type, we treat it as a primitive type for
+/// convenience.
+class ICEBERG_EXPORT NullType : public PrimitiveType {
Review Comment:
Yeah, more sense, removed.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2251859955
##########
test/transform_test.cc:
##########
@@ -193,4 +193,297 @@ TEST(TransformResultTypeTest, NegativeCases) {
}
}
+TEST(TransformFunctionTransformTest, IdentityTransform) {
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::boolean(),
+ .source = Literal::Boolean(true),
+ .expected = Literal::Boolean(true)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(42)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Date(30000)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Long(1234567890)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000000),
+ .expected = Literal::Timestamp(1622547800000000)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000000),
+ .expected = Literal::TimestampTz(1622547800000000)},
+ {.source_type = iceberg::float32(),
+ .source = Literal::Float(3.14),
+ .expected = Literal::Float(3.14)},
+ {.source_type = iceberg::float64(),
+ .source = Literal::Double(1.23e-5),
+ .expected = Literal::Double(1.23e-5)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("Hello, World!"),
+ .expected = Literal::String("Hello, World!")},
+ {.source_type = iceberg::binary(),
+ .source = Literal::Binary({0x01, 0x02, 0x03}),
+ .expected = Literal::Binary({0x01, 0x02, 0x03})},
+ };
+
+ for (const auto& c : cases) {
+ auto transform = Transform::Identity();
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind identity transform";
+
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, BucketTransform) {
+ constexpr int32_t num_buckets = 4;
+ auto transform = Transform::Bucket(num_buckets);
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::date(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Int(2)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("test"),
+ .expected = Literal::Int(3)},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind bucket transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, TruncateTransform) {
+ constexpr int32_t width = 5;
+ auto transform = Transform::Truncate(width);
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(123456),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("Hello, World!"),
Review Comment:
Add some emoji cases.
##########
test/transform_test.cc:
##########
@@ -193,4 +193,297 @@ TEST(TransformResultTypeTest, NegativeCases) {
}
}
+TEST(TransformFunctionTransformTest, IdentityTransform) {
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::boolean(),
+ .source = Literal::Boolean(true),
+ .expected = Literal::Boolean(true)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(42)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Date(30000)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Long(1234567890)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000000),
+ .expected = Literal::Timestamp(1622547800000000)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000000),
+ .expected = Literal::TimestampTz(1622547800000000)},
+ {.source_type = iceberg::float32(),
+ .source = Literal::Float(3.14),
+ .expected = Literal::Float(3.14)},
+ {.source_type = iceberg::float64(),
+ .source = Literal::Double(1.23e-5),
+ .expected = Literal::Double(1.23e-5)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("Hello, World!"),
+ .expected = Literal::String("Hello, World!")},
+ {.source_type = iceberg::binary(),
+ .source = Literal::Binary({0x01, 0x02, 0x03}),
+ .expected = Literal::Binary({0x01, 0x02, 0x03})},
+ };
+
+ for (const auto& c : cases) {
+ auto transform = Transform::Identity();
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind identity transform";
+
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, BucketTransform) {
+ constexpr int32_t num_buckets = 4;
+ auto transform = Transform::Bucket(num_buckets);
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::date(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Int(2)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("test"),
+ .expected = Literal::Int(3)},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind bucket transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, TruncateTransform) {
+ constexpr int32_t width = 5;
+ auto transform = Transform::Truncate(width);
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(123456),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("Hello, World!"),
+ .expected = Literal::String("Hello")},
+ {.source_type = iceberg::binary(),
+ .source = Literal::Binary({0x01, 0x02, 0x03, 0x04, 0x05, 0x06}),
+ .expected = Literal::Binary({0x01, 0x02, 0x03, 0x04, 0x05})},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind truncate transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformFunctionTransformTest, YearTransform) {
+ auto transform = Transform::Year();
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000000),
Review Comment:
done.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2251855818
##########
src/iceberg/transform_function.cc:
##########
@@ -91,6 +140,46 @@ Result<ArrowArray> TruncateTransform::Transform(const ArrowArray& input) {
return NotImplemented("TruncateTransform::Transform");
}
+Result<std::optional<Literal>> TruncateTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply truncate transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+
+ switch (source_type()->type_id()) {
+ case TypeId::kInt: {
+ auto value = std::get<int32_t>(literal.value());
+ return Literal::Int(value % width_);
+ }
+ case TypeId::kLong: {
+ auto value = std::get<int64_t>(literal.value());
+ return Literal::Long(value % width_);
+ }
+ case TypeId::kDecimal: {
+ // TODO(zhjwpku): Handle decimal truncation logic here
+ return NotImplemented("Truncate for Decimal is not implemented yet");
+ }
+ case TypeId::kString: {
+ auto value = std::get<std::string>(literal.value());
+ if (value.size() > static_cast<size_t>(width_)) {
+ value.resize(width_);
Review Comment:
Good catch, I didn't realize that before.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2262232472
##########
src/iceberg/transform_function.cc:
##########
@@ -148,14 +253,51 @@ Result<std::unique_ptr<TransformFunction>> YearTransform::Make(
MonthTransform::MonthTransform(std::shared_ptr<Type> const& source_type)
: TransformFunction(TransformType::kMonth, source_type) {}
-Result<ArrowArray> MonthTransform::Transform(const ArrowArray& input) {
- return NotImplemented("MonthTransform::Transform");
-}
+Result<Literal> MonthTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply month transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+ if (literal.IsNull()) [[unlikely]] {
+ return Literal::Null(iceberg::int32());
+ }
-Result<std::shared_ptr<Type>> MonthTransform::ResultType() const {
- return iceberg::int32();
+ using namespace std::chrono; // NOLINT
+ switch (source_type()->type_id()) {
+ case TypeId::kDate: {
+ auto value = std::get<int32_t>(literal.value());
Review Comment:
nit: I think we need also move all of these transform functions to a dedicated utility file and add exhaustive test cases. Could you create an issue for this?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2253646204
##########
test/transform_test.cc:
##########
@@ -193,4 +187,316 @@ TEST(TransformResultTypeTest, NegativeCases) {
}
}
+TEST(TransformLiteralTest, IdentityTransform) {
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::boolean(),
+ .source = Literal::Boolean(true),
+ .expected = Literal::Boolean(true)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(42)},
+ {.source_type = iceberg::int32(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Date(30000)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Long(1234567890)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000000),
+ .expected = Literal::Timestamp(1622547800000000)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000000),
+ .expected = Literal::TimestampTz(1622547800000000)},
+ {.source_type = iceberg::float32(),
+ .source = Literal::Float(3.14),
+ .expected = Literal::Float(3.14)},
+ {.source_type = iceberg::float64(),
+ .source = Literal::Double(1.23e-5),
+ .expected = Literal::Double(1.23e-5)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("Hello, World!"),
+ .expected = Literal::String("Hello, World!")},
+ {.source_type = iceberg::binary(),
+ .source = Literal::Binary({0x01, 0x02, 0x03}),
+ .expected = Literal::Binary({0x01, 0x02, 0x03})},
+ };
+
+ for (const auto& c : cases) {
+ auto transform = Transform::Identity();
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind identity transform";
+
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformLiteralTest, BucketTransform) {
+ constexpr int32_t num_buckets = 4;
+ auto transform = Transform::Bucket(num_buckets);
+
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .source = Literal::Int(42),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::date(),
+ .source = Literal::Date(30000),
+ .expected = Literal::Int(2)},
+ {.source_type = iceberg::int64(),
+ .source = Literal::Long(1234567890),
+ .expected = Literal::Int(3)},
+ {.source_type = iceberg::timestamp(),
+ .source = Literal::Timestamp(1622547800000000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::timestamp_tz(),
+ .source = Literal::TimestampTz(1622547800000000),
+ .expected = Literal::Int(1)},
+ {.source_type = iceberg::string(),
+ .source = Literal::String("test"),
+ .expected = Literal::Int(3)},
+ };
+
+ for (const auto& c : cases) {
+ auto transformPtr = transform->Bind(c.source_type);
+ ASSERT_TRUE(transformPtr.has_value()) << "Failed to bind bucket transform";
+ auto result = transformPtr.value()->Transform(c.source);
+ ASSERT_TRUE(result.has_value())
+ << "Failed to transform literal: " << c.source.ToString();
+
+ EXPECT_EQ(result.value(), c.expected)
+ << "Unexpected result for source: " << c.source.ToString();
+ }
+}
+
+TEST(TransformLiteralTest, TruncateTransform) {
+ struct Case {
+ std::shared_ptr<Type> source_type;
+ int32_t width;
+ Literal source;
+ Literal expected;
+ };
+
+ const std::vector<Case> cases = {
+ {.source_type = iceberg::int32(),
+ .width = 5,
+ .source = Literal::Int(123456),
+ .expected = Literal::Int(123455)},
+ {.source_type = iceberg::string(),
+ .width = 5,
+ .source = Literal::String("Hello, World!"),
+ .expected = Literal::String("Hello")},
+ {.source_type = iceberg::string(),
+ .width = 5,
+ .source = Literal::String("😜🧐🤔🤪🥳"),
+ // Truncate to 5 bytes, the safe point should be four bytes which fits the first
Review Comment:
I think this will be annoying to do in C++ without pulling in a library like utf8proc (I suppose assuming UTF-8 you can implement just the bit logic to detect a codepoint)
(FWIW: one emoji can be multiple codepoints, IIRC (due to e.g. flags, skin color selectors) An emoji would be a grapheme cluster?)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2263617095
##########
src/iceberg/transform_function.cc:
##########
@@ -176,11 +318,39 @@ Result<std::unique_ptr<TransformFunction>> MonthTransform::Make(
DayTransform::DayTransform(std::shared_ptr<Type> const& source_type)
: TransformFunction(TransformType::kDay, source_type) {}
-Result<ArrowArray> DayTransform::Transform(const ArrowArray& input) {
- return NotImplemented("DayTransform::Transform");
+Result<Literal> DayTransform::Transform(const Literal& literal) {
+ assert(literal.type() == source_type());
+ if (literal.IsBelowMin() || literal.IsAboveMax()) {
+ return InvalidArgument(
+ "Cannot apply day transform to literal with value {} of type {}",
+ literal.ToString(), source_type()->ToString());
+ }
+ if (literal.IsNull()) [[unlikely]] {
+ return Literal::Null(iceberg::int32());
Review Comment:
What about L345? Is that in-consistent?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: implement Literal Transform [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #156:
URL: https://github.com/apache/iceberg-cpp/pull/156#discussion_r2240139253
##########
src/iceberg/transform.h:
##########
@@ -172,6 +174,8 @@ class ICEBERG_EXPORT TransformFunction {
TransformFunction(TransformType transform_type, std::shared_ptr<Type> source_type);
/// \brief Transform an input array to a new array
virtual Result<ArrowArray> Transform(const ArrowArray& data) = 0;
Review Comment:
Perhaps we can remove this now? cc @gty404
##########
src/iceberg/expression/literal.h:
##########
@@ -63,7 +63,10 @@ class ICEBERG_EXPORT Literal {
/// \brief Factory methods for primitive types
static Literal Boolean(bool value);
static Literal Int(int32_t value);
+ static Literal Date(int32_t value);
static Literal Long(int64_t value);
+ static Literal Timestamp(int64_t value);
Review Comment:
Do we need to add `Time`?
##########
src/iceberg/expression/literal.cc:
##########
@@ -208,12 +216,30 @@ std::partial_ordering Literal::operator<=>(const Literal& other) const {
return this_val <=> other_val;
}
+ case TypeId::kDate: {
Review Comment:
Should we merge it with the above case? We can add [[fallthrough]] to make it explicit.
##########
src/iceberg/util/macros.h:
##########
@@ -36,3 +36,8 @@
#define ICEBERG_ASSIGN_OR_RAISE(lhs, rexpr) \
ICEBERG_ASSIGN_OR_RAISE_IMPL(ICEBERG_ASSIGN_OR_RAISE_NAME(result_, __COUNTER__), lhs, \
rexpr)
+
+#define NANOARROW_RETURN_IF_NOT_OK(status, error) \
+ if (status != NANOARROW_OK) [[unlikely]] { \
Review Comment:
We shouldn't expose nanoarrow publicly.
##########
src/iceberg/expression/literal.h:
##########
@@ -85,6 +88,9 @@ class ICEBERG_EXPORT Literal {
/// \brief Get the literal type.
const std::shared_ptr<PrimitiveType>& type() const;
+ /// \brief Get the literal value.
+ const Value& value() const { return value_; }
Review Comment:
`Value` is still private. We might need to make it public.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org