You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "gty404 (via GitHub)" <gi...@apache.org> on 2025/04/08 05:31:08 UTC
[PR] feat: transform function [iceberg-cpp]
gty404 opened a new pull request, #61:
URL: https://github.com/apache/iceberg-cpp/pull/61
(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: transform function [iceberg-cpp]
Posted by "gty404 (via GitHub)" <gi...@apache.org>.
gty404 commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2036516995
##########
src/iceberg/type_fwd.h:
##########
@@ -101,6 +101,7 @@ class StructLike;
class TableMetadata;
enum class TransformType;
class TransformFunction;
+struct TransformSpec;
Review Comment:
It's not needed anymore, I will delete it.
--
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: transform function [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2032469032
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
Hmm, do the parameters need to be Arrow data? I suppose what we need is a standard for scalars or row-wise data...
##########
src/iceberg/transform/transform_factory.h:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_factory.h
+
+#include <memory>
+
+#include "iceberg/error.h"
+#include "iceberg/expected.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Factory class for creating TransformFunction instances from TransformSpec.
Review Comment:
Can we just have `TransformFunction::Make`? Especially because `create` is static anyways so there is no value to having a factory (+factory is very much a Java-ism that doesn't offer anything here)
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
Do we actually need TransformSpec though?
##########
src/iceberg/transform.h:
##########
@@ -56,14 +56,45 @@ enum class TransformType {
kVoid,
};
+/// \brief Get the relative transform name
+constexpr std::string_view ToString(TransformType type) {
+ switch (type) {
+ case TransformType::kUnknown:
+ return "unknown";
+ case TransformType::kIdentity:
+ return "identity";
+ case TransformType::kBucket:
+ return "bucket";
+ case TransformType::kTruncate:
+ return "truncate";
+ case TransformType::kYear:
+ return "year";
+ case TransformType::kMonth:
+ return "month";
+ case TransformType::kDay:
+ return "day";
+ case TransformType::kHour:
+ return "hour";
+ case TransformType::kVoid:
+ return "void";
+ default:
+ return "invalid";
+ }
+}
+
/// \brief A transform function used for partitioning.
class ICEBERG_EXPORT TransformFunction : public util::Formattable {
public:
- explicit TransformFunction(TransformType type);
+ explicit TransformFunction(TransformType transform_type,
+ std::shared_ptr<Type> source_type);
/// \brief Transform an input array to a new array
virtual expected<ArrowArray, Error> Transform(const ArrowArray& data) = 0;
/// \brief Get the transform type
- virtual TransformType transform_type() const;
+ TransformType transform_type() const;
+ /// \brief Get the source type of transform function
+ std::shared_ptr<Type> source_type() const;
Review Comment:
`const&` to avoid forcing a copy
##########
src/iceberg/transform.h:
##########
@@ -56,14 +56,45 @@ enum class TransformType {
kVoid,
};
+/// \brief Get the relative transform name
+constexpr std::string_view ToString(TransformType type) {
+ switch (type) {
+ case TransformType::kUnknown:
+ return "unknown";
+ case TransformType::kIdentity:
+ return "identity";
+ case TransformType::kBucket:
+ return "bucket";
+ case TransformType::kTruncate:
+ return "truncate";
+ case TransformType::kYear:
+ return "year";
+ case TransformType::kMonth:
+ return "month";
+ case TransformType::kDay:
+ return "day";
+ case TransformType::kHour:
+ return "hour";
+ case TransformType::kVoid:
+ return "void";
+ default:
+ return "invalid";
+ }
+}
+
/// \brief A transform function used for partitioning.
class ICEBERG_EXPORT TransformFunction : public util::Formattable {
public:
- explicit TransformFunction(TransformType type);
+ explicit TransformFunction(TransformType transform_type,
+ std::shared_ptr<Type> source_type);
/// \brief Transform an input array to a new array
virtual expected<ArrowArray, Error> Transform(const ArrowArray& data) = 0;
/// \brief Get the transform type
- virtual TransformType transform_type() const;
+ TransformType transform_type() const;
+ /// \brief Get the source type of transform function
+ std::shared_ptr<Type> source_type() const;
+ /// \brief Get the result type of transform function
+ virtual expected<std::shared_ptr<Type>, Error> result_type() const = 0;
Review Comment:
`snake_case` is only for trivial getters. This should either be `shared_ptr<Type> const& result_type() const` or `expected<...> ResultType() const`.
--
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: transform function [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2034375471
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
SGTM!
--
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: transform function [iceberg-cpp]
Posted by "gty404 (via GitHub)" <gi...@apache.org>.
gty404 commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2033070013
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
> > 1. Using ArrowArray is necessary, as different Transforms may use different types and numbers of parameters, which should be needed within the Transform or when creating an instance of the Transform.
>
> Yes, but why not (say) `vector<variant<...>>`?
The parameter list for this variant might be quite long. Additionally, I understand that there is also logic to convert from JSON to ArrowArray in other places, and using ArrowArray would be more natural. However, this is just my personal opinion.
--
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: transform function [iceberg-cpp]
Posted by "gty404 (via GitHub)" <gi...@apache.org>.
gty404 commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2034506563
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,100 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <cstdint>
+#include <memory>
+#include <variant>
+#include <vector>
+
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a int32 value (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be empty.
+ std::vector<std::variant<int32_t>> params;
Review Comment:
> I don't like the name of `TransformSpec` because it does not have a spec_id or something. What about renaming `TransformSpec` to `Transform`? For `TransformFunction`, we can use `Apply` or even `operator()` to resolve the name conflict.
How about UnboundTransform?
--
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: transform function [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2034508109
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,100 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <cstdint>
+#include <memory>
+#include <variant>
+#include <vector>
+
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a int32 value (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be empty.
+ std::vector<std::variant<int32_t>> params;
Review Comment:
I suppose `UnboundTransform` will be used in the expression
--
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: transform function [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2036600412
##########
test/transform_test.cc:
##########
@@ -25,30 +25,48 @@
#include <gmock/gmock.h>
#include <gtest/gtest.h>
+#include "iceberg/transform/transform_function.h"
+#include "iceberg/type.h"
#include "iceberg/util/formatter.h"
namespace iceberg {
TEST(TransformTest, TransformFunction) {
- class TestTransformFunction : public TransformFunction {
- public:
- TestTransformFunction() : TransformFunction(TransformType::kUnknown) {}
- expected<ArrowArray, Error> Transform(const ArrowArray& input) override {
- return unexpected(
- Error{.kind = ErrorKind::kNotSupported, .message = "test transform function"});
- }
- };
-
- TestTransformFunction transform;
- EXPECT_EQ(TransformType::kUnknown, transform.transform_type());
- EXPECT_EQ("unknown", transform.ToString());
- EXPECT_EQ("unknown", std::format("{}", transform));
+ IdentityTransform transform{std::make_shared<StringType>()};
Review Comment:
not for this PR but we should consider singleton type instances for common types (though maybe sharing a single refcount is actually a net negative)
--
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: transform function [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2034363891
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
That sounds good to me. @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: transform function [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2034372823
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,100 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <cstdint>
+#include <memory>
+#include <variant>
+#include <vector>
+
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a int32 value (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be empty.
+ std::vector<std::variant<int32_t>> params;
Review Comment:
The only difference between `TransformFunction` and `TransformSpec` is this addition param. Should we delete `TransformSpec` by adding `param` to `TransformFunction`?
--
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: transform function [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2034371723
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,100 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <cstdint>
+#include <memory>
+#include <variant>
+#include <vector>
+
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a int32 value (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be empty.
+ std::vector<std::variant<int32_t>> params;
Review Comment:
Why not simply `std::variant<std::monostate, int32_t> param`?
--
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: transform function [iceberg-cpp]
Posted by "gty404 (via GitHub)" <gi...@apache.org>.
gty404 commented on PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#issuecomment-2792545038
I made the following modifications based on the previous comment:
1. The Transform object is used for JSON serialization/deserialization.
2. Transform::Bind can get the TransformFunction, which is used for data transformation.
Please review in your spare time. Thanks @lidavidm @wgtmac @zhjwpku
--
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: transform function [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2034179840
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
If the argument is that you can't instantiate the full transform during parsing, you could split up the transform's type (aka the operation + parameters) which you should be able to construct immediately from the fully instantiated transform
--
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: transform function [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2038705942
##########
src/iceberg/transform.h:
##########
@@ -56,16 +57,98 @@ enum class TransformType {
kVoid,
};
+/// \brief Get the relative transform name
+constexpr std::string_view TransformTypeToString(TransformType type) {
+ switch (type) {
+ case TransformType::kUnknown:
+ return "unknown";
+ case TransformType::kIdentity:
+ return "identity";
+ case TransformType::kBucket:
+ return "bucket";
+ case TransformType::kTruncate:
+ return "truncate";
+ case TransformType::kYear:
+ return "year";
+ case TransformType::kMonth:
+ return "month";
+ case TransformType::kDay:
+ return "day";
+ case TransformType::kHour:
+ return "hour";
+ case TransformType::kVoid:
+ return "void";
+ }
+}
+
+/// \brief Represents a transform used in partitioning or sorting in Iceberg.
+///
+/// This class supports binding to a source type and instantiating the corresponding
+/// TransformFunction, as well as serialization-friendly introspection.
+class ICEBERG_EXPORT Transform : public util::Formattable {
+ public:
+ /// \brief Returns a shared singleton instance of the Identity transform.
+ ///
+ /// This transform leaves values unchanged and is commonly used for direct partitioning.
+ /// \return A shared pointer to the Identity transform.
+ static std::shared_ptr<Transform> Identity();
+
+ /// \brief Constructs a Transform of the specified type (for non-parametric types).
+ /// \param transform_type The transform type (e.g., identity, year, day).
+ explicit Transform(TransformType transform_type);
+
+ /// \brief Constructs a parameterized Transform (e.g., bucket(16), truncate(4)).
+ /// \param transform_type The transform type.
+ /// \param param The integer parameter associated with the transform.
+ Transform(TransformType transform_type, int32_t param);
+
+ /// \brief Returns the transform type.
+ TransformType transform_type() const;
+
+ /// \brief Binds this transform to a source type, returning a typed TransformFunction.
+ ///
+ /// This creates a concrete transform implementation based on the transform type and
+ /// parameter.
+ /// \param source_type The source column type to bind to.
+ /// \return A TransformFunction instance wrapped in `expected`, or an error on failure.
+ expected<std::unique_ptr<TransformFunction>, Error> Bind(
+ const std::shared_ptr<Type>& source_type) const;
+
+ /// \brief Returns a string representation of this transform (e.g., "bucket[16]").
+ std::string ToString() const override;
+
+ /// \brief Equality comparison.
+ friend bool operator==(const Transform& lhs, const Transform& rhs) {
+ return lhs.Equals(rhs);
+ }
+
+ /// \brief Inequality comparison.
+ friend bool operator!=(const Transform& lhs, const Transform& rhs) {
+ return !(lhs == rhs);
+ }
+
+ private:
+ /// \brief Checks equality with another Transform instance.
+ [[nodiscard]] virtual bool Equals(const Transform& other) const;
+
+ TransformType transform_type_;
+ ///< Optional parameter (e.g., num_buckets, width)
+ std::variant<std::monostate, int32_t> param_;
+};
+
/// \brief A transform function used for partitioning.
-class ICEBERG_EXPORT TransformFunction : public util::Formattable {
Review Comment:
Sounds good to me.
--
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: transform function [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2039748800
##########
src/iceberg/transform.cc:
##########
@@ -21,65 +21,113 @@
#include <format>
+#include "iceberg/transform_function.h"
+#include "iceberg/type.h"
+
namespace iceberg {
-namespace {
-/// \brief Get the relative transform name
-constexpr std::string_view ToString(TransformType type) {
- switch (type) {
- case TransformType::kUnknown:
- return "unknown";
+std::shared_ptr<Transform> Transform::Identity() {
+ static auto instance = std::make_shared<Transform>(TransformType::kIdentity);
+ return instance;
+}
+
+Transform::Transform(TransformType transform_type) : transform_type_(transform_type) {}
Review Comment:
Throw for parameterized transform_type?
##########
src/iceberg/transform.h:
##########
@@ -56,16 +57,98 @@ enum class TransformType {
kVoid,
};
+/// \brief Get the relative transform name
+constexpr std::string_view TransformTypeToString(TransformType type) {
+ switch (type) {
+ case TransformType::kUnknown:
+ return "unknown";
+ case TransformType::kIdentity:
+ return "identity";
+ case TransformType::kBucket:
+ return "bucket";
+ case TransformType::kTruncate:
+ return "truncate";
+ case TransformType::kYear:
+ return "year";
+ case TransformType::kMonth:
+ return "month";
+ case TransformType::kDay:
+ return "day";
+ case TransformType::kHour:
+ return "hour";
+ case TransformType::kVoid:
+ return "void";
+ }
+}
+
+/// \brief Represents a transform used in partitioning or sorting in Iceberg.
+///
+/// This class supports binding to a source type and instantiating the corresponding
+/// TransformFunction, as well as serialization-friendly introspection.
+class ICEBERG_EXPORT Transform : public util::Formattable {
+ public:
+ /// \brief Returns a shared singleton instance of the Identity transform.
+ ///
+ /// This transform leaves values unchanged and is commonly used for direct partitioning.
+ /// \return A shared pointer to the Identity transform.
+ static std::shared_ptr<Transform> Identity();
+
+ /// \brief Constructs a Transform of the specified type (for non-parametric types).
+ /// \param transform_type The transform type (e.g., identity, year, day).
+ explicit Transform(TransformType transform_type);
+
+ /// \brief Constructs a parameterized Transform (e.g., bucket(16), truncate(4)).
+ /// \param transform_type The transform type.
+ /// \param param The integer parameter associated with the transform.
+ Transform(TransformType transform_type, int32_t param);
+
+ /// \brief Returns the transform type.
+ TransformType transform_type() const;
+
+ /// \brief Binds this transform to a source type, returning a typed TransformFunction.
+ ///
+ /// This creates a concrete transform implementation based on the transform type and
+ /// parameter.
+ /// \param source_type The source column type to bind to.
+ /// \return A TransformFunction instance wrapped in `expected`, or an error on failure.
+ expected<std::unique_ptr<TransformFunction>, Error> Bind(
Review Comment:
Can we use forward declaration here and define TransformFunction in the `transform_function.h`?
##########
src/iceberg/transform.cc:
##########
@@ -21,65 +21,113 @@
#include <format>
+#include "iceberg/transform_function.h"
+#include "iceberg/type.h"
+
namespace iceberg {
-namespace {
-/// \brief Get the relative transform name
-constexpr std::string_view ToString(TransformType type) {
- switch (type) {
- case TransformType::kUnknown:
- return "unknown";
+std::shared_ptr<Transform> Transform::Identity() {
+ static auto instance = std::make_shared<Transform>(TransformType::kIdentity);
+ return instance;
+}
+
+Transform::Transform(TransformType transform_type) : transform_type_(transform_type) {}
+
+Transform::Transform(TransformType transform_type, int32_t param)
Review Comment:
Throw for non-parameterized transform_type?
##########
src/iceberg/transform.cc:
##########
@@ -21,65 +21,113 @@
#include <format>
+#include "iceberg/transform_function.h"
+#include "iceberg/type.h"
+
namespace iceberg {
-namespace {
-/// \brief Get the relative transform name
-constexpr std::string_view ToString(TransformType type) {
- switch (type) {
- case TransformType::kUnknown:
- return "unknown";
+std::shared_ptr<Transform> Transform::Identity() {
+ static auto instance = std::make_shared<Transform>(TransformType::kIdentity);
+ return instance;
+}
+
+Transform::Transform(TransformType transform_type) : transform_type_(transform_type) {}
+
+Transform::Transform(TransformType transform_type, int32_t param)
+ : transform_type_(transform_type), param_(param) {}
+
+TransformType Transform::transform_type() const { return transform_type_; }
+
+expected<std::unique_ptr<TransformFunction>, Error> Transform::Bind(
Review Comment:
It seems that we should merge `transform_function.h/cc` to `transform.h/cc`. They have strong connections.
##########
src/iceberg/transform.cc:
##########
@@ -21,65 +21,113 @@
#include <format>
+#include "iceberg/transform_function.h"
+#include "iceberg/type.h"
+
namespace iceberg {
-namespace {
-/// \brief Get the relative transform name
-constexpr std::string_view ToString(TransformType type) {
- switch (type) {
- case TransformType::kUnknown:
- return "unknown";
+std::shared_ptr<Transform> Transform::Identity() {
+ static auto instance = std::make_shared<Transform>(TransformType::kIdentity);
+ return instance;
+}
+
+Transform::Transform(TransformType transform_type) : transform_type_(transform_type) {}
+
+Transform::Transform(TransformType transform_type, int32_t param)
Review Comment:
If we want to avoid invalid inputs. Perhaps we should define a separate static functions to create each transform type?
##########
src/iceberg/transform.h:
##########
@@ -56,16 +57,98 @@ enum class TransformType {
kVoid,
};
+/// \brief Get the relative transform name
+constexpr std::string_view TransformTypeToString(TransformType type) {
+ switch (type) {
+ case TransformType::kUnknown:
+ return "unknown";
+ case TransformType::kIdentity:
+ return "identity";
+ case TransformType::kBucket:
+ return "bucket";
+ case TransformType::kTruncate:
+ return "truncate";
+ case TransformType::kYear:
+ return "year";
+ case TransformType::kMonth:
+ return "month";
+ case TransformType::kDay:
+ return "day";
+ case TransformType::kHour:
+ return "hour";
+ case TransformType::kVoid:
+ return "void";
+ }
+}
+
+/// \brief Represents a transform used in partitioning or sorting in Iceberg.
+///
+/// This class supports binding to a source type and instantiating the corresponding
+/// TransformFunction, as well as serialization-friendly introspection.
+class ICEBERG_EXPORT Transform : public util::Formattable {
+ public:
+ /// \brief Returns a shared singleton instance of the Identity transform.
+ ///
+ /// This transform leaves values unchanged and is commonly used for direct partitioning.
+ /// \return A shared pointer to the Identity transform.
+ static std::shared_ptr<Transform> Identity();
+
+ /// \brief Constructs a Transform of the specified type (for non-parametric types).
+ /// \param transform_type The transform type (e.g., identity, year, day).
+ explicit Transform(TransformType transform_type);
+
+ /// \brief Constructs a parameterized Transform (e.g., bucket(16), truncate(4)).
+ /// \param transform_type The transform type.
+ /// \param param The integer parameter associated with the transform.
+ Transform(TransformType transform_type, int32_t param);
+
+ /// \brief Returns the transform type.
+ TransformType transform_type() const;
+
+ /// \brief Binds this transform to a source type, returning a typed TransformFunction.
+ ///
+ /// This creates a concrete transform implementation based on the transform type and
+ /// parameter.
+ /// \param source_type The source column type to bind to.
+ /// \return A TransformFunction instance wrapped in `expected`, or an error on failure.
+ expected<std::unique_ptr<TransformFunction>, Error> Bind(
+ const std::shared_ptr<Type>& source_type) const;
+
+ /// \brief Returns a string representation of this transform (e.g., "bucket[16]").
+ std::string ToString() const override;
+
+ /// \brief Equality comparison.
+ friend bool operator==(const Transform& lhs, const Transform& rhs) {
+ return lhs.Equals(rhs);
+ }
+
+ /// \brief Inequality comparison.
+ friend bool operator!=(const Transform& lhs, const Transform& rhs) {
+ return !(lhs == rhs);
+ }
+
+ private:
+ /// \brief Checks equality with another Transform instance.
+ [[nodiscard]] virtual bool Equals(const Transform& other) const;
+
+ TransformType transform_type_;
+ ///< Optional parameter (e.g., num_buckets, width)
+ std::variant<std::monostate, int32_t> param_;
+};
+
/// \brief A transform function used for partitioning.
-class ICEBERG_EXPORT TransformFunction : public util::Formattable {
+class ICEBERG_EXPORT TransformFunction {
public:
- explicit TransformFunction(TransformType type);
+ virtual ~TransformFunction() = default;
+ TransformFunction(TransformType transform_type, std::shared_ptr<Type> source_type);
/// \brief Transform an input array to a new array
virtual expected<ArrowArray, Error> Transform(const ArrowArray& data) = 0;
/// \brief Get the transform type
- virtual TransformType transform_type() const;
-
- std::string ToString() const override;
+ TransformType transform_type() const;
+ /// \brief Get the source type of transform function
+ const std::shared_ptr<Type>& source_type() const;
+ /// \brief Get the result type of transform function
+ virtual expected<std::shared_ptr<Type>, Error> ResultType() const = 0;
Review Comment:
Use `Result`?
--
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: transform function [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2033053211
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
> 1. Using ArrowArray is necessary, as different Transforms may use different types and numbers of parameters, which should be needed within the Transform or when creating an instance of the Transform.
Yes, but why not (say) `vector<variant<...>>`?
--
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: transform function [iceberg-cpp]
Posted by "gty404 (via GitHub)" <gi...@apache.org>.
gty404 commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2032896374
##########
src/iceberg/transform/transform_factory.h:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_factory.h
+
+#include <memory>
+
+#include "iceberg/error.h"
+#include "iceberg/expected.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+
+/// \brief Factory class for creating TransformFunction instances from TransformSpec.
Review Comment:
Reasonable, I will modify it. Thank you.
--
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: transform function [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2034393074
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,100 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <cstdint>
+#include <memory>
+#include <variant>
+#include <vector>
+
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a int32 value (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be empty.
+ std::vector<std::variant<int32_t>> params;
Review Comment:
For each transform function, there is unlikely to have more than one param in the near future. Am I correct, @Fokko?
--
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: transform function [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2036584089
##########
src/iceberg/transform.cc:
##########
@@ -21,55 +21,26 @@
#include <format>
-namespace iceberg {
+#include "iceberg/type.h"
-namespace {
-/// \brief Get the relative transform name
-constexpr std::string_view ToString(TransformType type) {
- switch (type) {
- case TransformType::kUnknown:
- return "unknown";
- case TransformType::kIdentity:
- return "identity";
- case TransformType::kBucket:
- return "bucket";
- case TransformType::kTruncate:
- return "truncate";
- case TransformType::kYear:
- return "year";
- case TransformType::kMonth:
- return "month";
- case TransformType::kDay:
- return "day";
- case TransformType::kHour:
- return "hour";
- case TransformType::kVoid:
- return "void";
- default:
- return "invalid";
- }
-}
-} // namespace
+namespace iceberg {
-TransformFunction::TransformFunction(TransformType type) : transform_type_(type) {}
+TransformFunction::TransformFunction(TransformType transform_type,
+ std::shared_ptr<Type> source_type)
+ : transform_type_(transform_type), source_type_(std::move(source_type)) {}
TransformType TransformFunction::transform_type() const { return transform_type_; }
-std::string TransformFunction::ToString() const {
- return std::format("{}", iceberg::ToString(transform_type_));
+std::shared_ptr<Type> const& TransformFunction::source_type() const {
Review Comment:
Ideally the formatter would just pick one style and stick to it...I think `T const&` is cleaner but `const T&` is far more common
--
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: transform function [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2036585617
##########
src/iceberg/transform.cc:
##########
@@ -21,55 +21,26 @@
#include <format>
-namespace iceberg {
+#include "iceberg/type.h"
-namespace {
-/// \brief Get the relative transform name
-constexpr std::string_view ToString(TransformType type) {
- switch (type) {
- case TransformType::kUnknown:
- return "unknown";
- case TransformType::kIdentity:
- return "identity";
- case TransformType::kBucket:
- return "bucket";
- case TransformType::kTruncate:
- return "truncate";
- case TransformType::kYear:
- return "year";
- case TransformType::kMonth:
- return "month";
- case TransformType::kDay:
- return "day";
- case TransformType::kHour:
- return "hour";
- case TransformType::kVoid:
- return "void";
- default:
- return "invalid";
- }
-}
-} // namespace
+namespace iceberg {
-TransformFunction::TransformFunction(TransformType type) : transform_type_(type) {}
+TransformFunction::TransformFunction(TransformType transform_type,
+ std::shared_ptr<Type> source_type)
+ : transform_type_(transform_type), source_type_(std::move(source_type)) {}
TransformType TransformFunction::transform_type() const { return transform_type_; }
-std::string TransformFunction::ToString() const {
- return std::format("{}", iceberg::ToString(transform_type_));
+std::shared_ptr<Type> const& TransformFunction::source_type() const {
Review Comment:
I suppose there's https://clang.llvm.org/docs/ClangFormatStyleOptions.html#qualifieralignment but with some caveats
--
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: transform function [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2040668421
##########
src/iceberg/transform.h:
##########
@@ -56,16 +57,133 @@ enum class TransformType {
kVoid,
};
+/// \brief Get the relative transform name
+ICEBERG_EXPORT constexpr std::string_view TransformTypeToString(TransformType type);
+
+/// \brief Represents a transform used in partitioning or sorting in Iceberg.
+///
+/// This class supports binding to a source type and instantiating the corresponding
+/// TransformFunction, as well as serialization-friendly introspection.
+class ICEBERG_EXPORT Transform : public util::Formattable {
+ public:
+ /// \brief Returns a shared singleton instance of the Identity transform.
+ ///
+ /// This transform leaves values unchanged and is commonly used for direct partitioning.
+ /// \return A shared pointer to the Identity transform.
+ static std::shared_ptr<Transform> Identity();
+
+ /// \brief Creates a shared instance of the Bucket transform.
+ ///
+ /// Buckets values using a hash modulo operation. Commonly used for distributing data.
+ /// \param num_buckets The number of buckets.
+ /// \return A shared pointer to the Bucket transform.
+ static std::shared_ptr<Transform> Bucket(int32_t num_buckets);
+
+ /// \brief Creates a shared instance of the Truncate transform.
+ ///
+ /// Truncates values to a fixed width (e.g., for strings or binary data).
+ /// \param width The width to truncate to.
+ /// \return A shared pointer to the Truncate transform.
+ static std::shared_ptr<Transform> Truncate(int32_t width);
+
+ /// \brief Creates a shared singleton instance of the Year transform.
+ ///
+ /// Extracts the year portion from a date or timestamp.
+ /// \return A shared pointer to the Year transform.
+ static std::shared_ptr<Transform> Year();
+
+ /// \brief Creates a shared singleton instance of the Month transform.
+ ///
+ /// Extracts the month portion from a date or timestamp.
+ /// \return A shared pointer to the Month transform.
+ static std::shared_ptr<Transform> Month();
+
+ /// \brief Creates a shared singleton instance of the Day transform.
+ ///
+ /// Extracts the day portion from a date or timestamp.
+ /// \return A shared pointer to the Day transform.
+ static std::shared_ptr<Transform> Day();
+
+ /// \brief Creates a shared singleton instance of the Hour transform.
+ ///
+ /// Extracts the hour portion from a timestamp.
+ /// \return A shared pointer to the Hour transform.
+ static std::shared_ptr<Transform> Hour();
+
+ /// \brief Creates a shared singleton instance of the Void transform.
+ ///
+ /// Ignores values and always returns null. Useful for testing or special cases.
+ /// \return A shared pointer to the Void transform.
+ static std::shared_ptr<Transform> Void();
+
+ /// \brief Returns the transform type.
+ TransformType transform_type() const;
+
+ /// \brief Binds this transform to a source type, returning a typed TransformFunction.
+ ///
+ /// This creates a concrete transform implementation based on the transform type and
+ /// parameter.
+ /// \param source_type The source column type to bind to.
+ /// \return A TransformFunction instance wrapped in `expected`, or an error on failure.
+ expected<std::unique_ptr<TransformFunction>, Error> Bind(
Review Comment:
```suggestion
Result<std::unique_ptr<TransformFunction>> Bind(
```
nit: we can leave these as-is and fix all together in a separate PR.
--
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: transform function [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2034178885
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
That's why I was questioning if we need this class in the first place vs. directly constructing the transforms which would sidestep this complexity
You _could_ use ArrowArray as a scalar but it is an extremely expensive scalar all things considered. At the very least you should pull in or write a RAII helper to manage its lifetime IMO if you are going down that path.
--
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: transform function [iceberg-cpp]
Posted by "gty404 (via GitHub)" <gi...@apache.org>.
gty404 commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2034281506
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
> If the argument is that you can't instantiate the full transform during parsing, you could split up the transform's type (aka the operation + parameters) which you should be able to construct immediately from the fully instantiated transform
When creating a TransformFunction, you need to specify the transform type, optional parameters, and the types of one or more source columns. These types need to be retrievable from the context to get the table's schema. My intention was to use TransformSpec as a parsed data block that can be easily passed around and delay the creation of the TransformFunction until the context meets the requirements. This might just be a hypothetical scenario, and currently, there might not be a need to provide an entry to create all TransformFunctions?
--
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: transform function [iceberg-cpp]
Posted by "gty404 (via GitHub)" <gi...@apache.org>.
gty404 commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2034383670
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,100 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <cstdint>
+#include <memory>
+#include <variant>
+#include <vector>
+
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a int32 value (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be empty.
+ std::vector<std::variant<int32_t>> params;
Review Comment:
1. Could there be multiple param?
2. TransformSpec is just the result after json parsing, and TransformFunction is the specific implementation for the transformation.
--
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: transform function [iceberg-cpp]
Posted by "gty404 (via GitHub)" <gi...@apache.org>.
gty404 commented on PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#issuecomment-2800283294
@Fokko @Xuanwo Could you help review and merge 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: transform function [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2033329587
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
> I suppose what we need is a standard for scalars or row-wise data...
I feel concerned of the complexity to introduce another row-wise data. Can't we reuse `ArrowArray` and pretend it as a scalar when its size is 1? @lidavidm
--
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: transform function [iceberg-cpp]
Posted by "gty404 (via GitHub)" <gi...@apache.org>.
gty404 commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2033327821
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
I have changed the type to vector<variant<int32_t>>, please review it in your free time. @lidavidm @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: transform function [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2032544448
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
I was thinking about the following approach:
```cpp
struct TransformUtil {
// TODO: source and result types may be required here
static expected<ArrowArray, Error> Copy(const ArrowArray& input);
static expected<ArrowArray, Error> Bucket(const ArrowArray& input, int32_t num_buckets);
static expected<ArrowArray, Error> Truncate(const ArrowArray& input, int32_t width);
};
class Transform {
public:
virtual ~Transform() = default;
expected<ArrowArray, Error> Apply(const ArrowArray& input) const {
return transform_function_(input);
}
protected:
Transform(
TransformType transform_type, std::shared_ptr<Type> source_type,
std::shared_ptr<Type> result_type,
std::function<expected<ArrowArray, Error>(const ArrowArray&)> transform_function)
: Transform(transform_type,
std::vector<std::shared_ptr<Type>>{std::move(source_type)},
std::move(result_type), std::move(transform_function)) {}
Transform(
TransformType transform_type, std::vector<std::shared_ptr<Type>> source_types,
std::shared_ptr<Type> result_type,
std::function<expected<ArrowArray, Error>(const ArrowArray&)> transform_function)
: transform_type_(transform_type),
source_types_(std::move(source_types)),
result_type_(std::move(result_type)),
transform_function_(std::move(transform_function)) {}
private:
TransformType transform_type_;
std::vector<std::shared_ptr<Type>> source_types_;
std::shared_ptr<Type> result_type_;
std::function<expected<ArrowArray, Error>(const ArrowArray&)> transform_function_;
};
class IdentityTransform : public Transform {
public:
explicit IdentityTransform(std::shared_ptr<Type> source_type)
: Transform(TransformType::kIdentity, source_type, source_type,
[](const ArrowArray& input) -> expected<ArrowArray, Error> {
return TransformUtil::Copy(input);
}) {}
};
class BucketTransform : public Transform {
public:
BucketTransform(std::shared_ptr<Type> source_type, int32_t num_buckets)
: BucketTransform(std::vector<std::shared_ptr<Type>>{std::move(source_type)},
num_buckets) {}
BucketTransform(std::vector<std::shared_ptr<Type>> source_types, int32_t num_buckets)
: Transform(TransformType::kBucket, std::move(source_types),
std::make_shared<IntType>(),
[num_buckets](const ArrowArray& input) -> expected<ArrowArray, Error> {
return TransformUtil::Bucket(input, num_buckets);
}),
num_buckets_{num_buckets} {}
int32_t num_buckets() const { return num_buckets_; }
private:
int32_t num_buckets_;
};
```
We can even overload `expected<ArrowArray, Error> operator()(const ArrowArray& input) const` if you don't like the name of `Apply`.
WDYT? @gty404 @lidavidm @yingcai-cy
--
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: transform function [iceberg-cpp]
Posted by "gty404 (via GitHub)" <gi...@apache.org>.
gty404 commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2034361080
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
> Right, which is why I said
>
> > you could split up the transform's type (aka the operation + parameters)[...]from the fully instantiated transform
>
> You could even still have something like TransformSpec, I'm just arguing that it might make more sense to directly instantiate a transform-like object (with parameters, without types), just like how a Field separates the (parameterized) Type out.
I see. I will provide UnboundTransform, which includes the transform type and parameters. At the same time, TransformFunction will provide a Bind interface to receive UnboundTransform and source type, generating a specific TransformFunction. What do you think?
--
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: transform function [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2038699818
##########
src/iceberg/transform.h:
##########
@@ -56,16 +57,98 @@ enum class TransformType {
kVoid,
};
+/// \brief Get the relative transform name
+constexpr std::string_view TransformTypeToString(TransformType type) {
+ switch (type) {
+ case TransformType::kUnknown:
+ return "unknown";
+ case TransformType::kIdentity:
+ return "identity";
+ case TransformType::kBucket:
+ return "bucket";
+ case TransformType::kTruncate:
+ return "truncate";
+ case TransformType::kYear:
+ return "year";
+ case TransformType::kMonth:
+ return "month";
+ case TransformType::kDay:
+ return "day";
+ case TransformType::kHour:
+ return "hour";
+ case TransformType::kVoid:
+ return "void";
+ }
+}
+
+/// \brief Represents a transform used in partitioning or sorting in Iceberg.
+///
+/// This class supports binding to a source type and instantiating the corresponding
+/// TransformFunction, as well as serialization-friendly introspection.
+class ICEBERG_EXPORT Transform : public util::Formattable {
+ public:
+ /// \brief Returns a shared singleton instance of the Identity transform.
+ ///
+ /// This transform leaves values unchanged and is commonly used for direct partitioning.
+ /// \return A shared pointer to the Identity transform.
+ static std::shared_ptr<Transform> Identity();
+
+ /// \brief Constructs a Transform of the specified type (for non-parametric types).
+ /// \param transform_type The transform type (e.g., identity, year, day).
+ explicit Transform(TransformType transform_type);
+
+ /// \brief Constructs a parameterized Transform (e.g., bucket(16), truncate(4)).
+ /// \param transform_type The transform type.
+ /// \param param The integer parameter associated with the transform.
+ Transform(TransformType transform_type, int32_t param);
+
+ /// \brief Returns the transform type.
+ TransformType transform_type() const;
+
+ /// \brief Binds this transform to a source type, returning a typed TransformFunction.
+ ///
+ /// This creates a concrete transform implementation based on the transform type and
+ /// parameter.
+ /// \param source_type The source column type to bind to.
+ /// \return A TransformFunction instance wrapped in `expected`, or an error on failure.
+ expected<std::unique_ptr<TransformFunction>, Error> Bind(
+ const std::shared_ptr<Type>& source_type) const;
+
+ /// \brief Returns a string representation of this transform (e.g., "bucket[16]").
+ std::string ToString() const override;
+
+ /// \brief Equality comparison.
+ friend bool operator==(const Transform& lhs, const Transform& rhs) {
+ return lhs.Equals(rhs);
+ }
+
+ /// \brief Inequality comparison.
+ friend bool operator!=(const Transform& lhs, const Transform& rhs) {
+ return !(lhs == rhs);
+ }
+
+ private:
+ /// \brief Checks equality with another Transform instance.
+ [[nodiscard]] virtual bool Equals(const Transform& other) const;
+
+ TransformType transform_type_;
+ ///< Optional parameter (e.g., num_buckets, width)
+ std::variant<std::monostate, int32_t> param_;
+};
+
/// \brief A transform function used for partitioning.
-class ICEBERG_EXPORT TransformFunction : public util::Formattable {
Review Comment:
nit: why not formattable anymore?
--
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: transform function [iceberg-cpp]
Posted by "gty404 (via GitHub)" <gi...@apache.org>.
gty404 commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2038705365
##########
src/iceberg/transform.h:
##########
@@ -56,16 +57,98 @@ enum class TransformType {
kVoid,
};
+/// \brief Get the relative transform name
+constexpr std::string_view TransformTypeToString(TransformType type) {
+ switch (type) {
+ case TransformType::kUnknown:
+ return "unknown";
+ case TransformType::kIdentity:
+ return "identity";
+ case TransformType::kBucket:
+ return "bucket";
+ case TransformType::kTruncate:
+ return "truncate";
+ case TransformType::kYear:
+ return "year";
+ case TransformType::kMonth:
+ return "month";
+ case TransformType::kDay:
+ return "day";
+ case TransformType::kHour:
+ return "hour";
+ case TransformType::kVoid:
+ return "void";
+ }
+}
+
+/// \brief Represents a transform used in partitioning or sorting in Iceberg.
+///
+/// This class supports binding to a source type and instantiating the corresponding
+/// TransformFunction, as well as serialization-friendly introspection.
+class ICEBERG_EXPORT Transform : public util::Formattable {
+ public:
+ /// \brief Returns a shared singleton instance of the Identity transform.
+ ///
+ /// This transform leaves values unchanged and is commonly used for direct partitioning.
+ /// \return A shared pointer to the Identity transform.
+ static std::shared_ptr<Transform> Identity();
+
+ /// \brief Constructs a Transform of the specified type (for non-parametric types).
+ /// \param transform_type The transform type (e.g., identity, year, day).
+ explicit Transform(TransformType transform_type);
+
+ /// \brief Constructs a parameterized Transform (e.g., bucket(16), truncate(4)).
+ /// \param transform_type The transform type.
+ /// \param param The integer parameter associated with the transform.
+ Transform(TransformType transform_type, int32_t param);
+
+ /// \brief Returns the transform type.
+ TransformType transform_type() const;
+
+ /// \brief Binds this transform to a source type, returning a typed TransformFunction.
+ ///
+ /// This creates a concrete transform implementation based on the transform type and
+ /// parameter.
+ /// \param source_type The source column type to bind to.
+ /// \return A TransformFunction instance wrapped in `expected`, or an error on failure.
+ expected<std::unique_ptr<TransformFunction>, Error> Bind(
+ const std::shared_ptr<Type>& source_type) const;
+
+ /// \brief Returns a string representation of this transform (e.g., "bucket[16]").
+ std::string ToString() const override;
+
+ /// \brief Equality comparison.
+ friend bool operator==(const Transform& lhs, const Transform& rhs) {
+ return lhs.Equals(rhs);
+ }
+
+ /// \brief Inequality comparison.
+ friend bool operator!=(const Transform& lhs, const Transform& rhs) {
+ return !(lhs == rhs);
+ }
+
+ private:
+ /// \brief Checks equality with another Transform instance.
+ [[nodiscard]] virtual bool Equals(const Transform& other) const;
+
+ TransformType transform_type_;
+ ///< Optional parameter (e.g., num_buckets, width)
+ std::variant<std::monostate, int32_t> param_;
+};
+
/// \brief A transform function used for partitioning.
-class ICEBERG_EXPORT TransformFunction : public util::Formattable {
Review Comment:
Transform is already formattable, I haven't thought of the need for TransformFunction to be printable yet, possibly outputting the source type/result type is a requirement. In the next PR, I will support this interface.
--
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: transform function [iceberg-cpp]
Posted by "gty404 (via GitHub)" <gi...@apache.org>.
gty404 commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2040480756
##########
src/iceberg/transform.cc:
##########
@@ -21,65 +21,113 @@
#include <format>
+#include "iceberg/transform_function.h"
+#include "iceberg/type.h"
+
namespace iceberg {
-namespace {
-/// \brief Get the relative transform name
-constexpr std::string_view ToString(TransformType type) {
- switch (type) {
- case TransformType::kUnknown:
- return "unknown";
+std::shared_ptr<Transform> Transform::Identity() {
+ static auto instance = std::make_shared<Transform>(TransformType::kIdentity);
+ return instance;
+}
+
+Transform::Transform(TransformType transform_type) : transform_type_(transform_type) {}
+
+Transform::Transform(TransformType transform_type, int32_t param)
Review Comment:
Yes, currently I only added Transform::Identity, I will add the others as well.
##########
src/iceberg/transform.cc:
##########
@@ -21,65 +21,113 @@
#include <format>
+#include "iceberg/transform_function.h"
+#include "iceberg/type.h"
+
namespace iceberg {
-namespace {
-/// \brief Get the relative transform name
-constexpr std::string_view ToString(TransformType type) {
- switch (type) {
- case TransformType::kUnknown:
- return "unknown";
+std::shared_ptr<Transform> Transform::Identity() {
+ static auto instance = std::make_shared<Transform>(TransformType::kIdentity);
+ return instance;
+}
+
+Transform::Transform(TransformType transform_type) : transform_type_(transform_type) {}
+
+Transform::Transform(TransformType transform_type, int32_t param)
+ : transform_type_(transform_type), param_(param) {}
+
+TransformType Transform::transform_type() const { return transform_type_; }
+
+expected<std::unique_ptr<TransformFunction>, Error> Transform::Bind(
Review Comment:
I suggest it is separate, with users of Transform only needing to depend on transform.h and not needing to concern themselves with the implementation details of the transform function.
##########
src/iceberg/transform.h:
##########
@@ -56,16 +57,98 @@ enum class TransformType {
kVoid,
};
+/// \brief Get the relative transform name
+constexpr std::string_view TransformTypeToString(TransformType type) {
+ switch (type) {
+ case TransformType::kUnknown:
+ return "unknown";
+ case TransformType::kIdentity:
+ return "identity";
+ case TransformType::kBucket:
+ return "bucket";
+ case TransformType::kTruncate:
+ return "truncate";
+ case TransformType::kYear:
+ return "year";
+ case TransformType::kMonth:
+ return "month";
+ case TransformType::kDay:
+ return "day";
+ case TransformType::kHour:
+ return "hour";
+ case TransformType::kVoid:
+ return "void";
+ }
+}
+
+/// \brief Represents a transform used in partitioning or sorting in Iceberg.
+///
+/// This class supports binding to a source type and instantiating the corresponding
+/// TransformFunction, as well as serialization-friendly introspection.
+class ICEBERG_EXPORT Transform : public util::Formattable {
+ public:
+ /// \brief Returns a shared singleton instance of the Identity transform.
+ ///
+ /// This transform leaves values unchanged and is commonly used for direct partitioning.
+ /// \return A shared pointer to the Identity transform.
+ static std::shared_ptr<Transform> Identity();
+
+ /// \brief Constructs a Transform of the specified type (for non-parametric types).
+ /// \param transform_type The transform type (e.g., identity, year, day).
+ explicit Transform(TransformType transform_type);
+
+ /// \brief Constructs a parameterized Transform (e.g., bucket(16), truncate(4)).
+ /// \param transform_type The transform type.
+ /// \param param The integer parameter associated with the transform.
+ Transform(TransformType transform_type, int32_t param);
+
+ /// \brief Returns the transform type.
+ TransformType transform_type() const;
+
+ /// \brief Binds this transform to a source type, returning a typed TransformFunction.
+ ///
+ /// This creates a concrete transform implementation based on the transform type and
+ /// parameter.
+ /// \param source_type The source column type to bind to.
+ /// \return A TransformFunction instance wrapped in `expected`, or an error on failure.
+ expected<std::unique_ptr<TransformFunction>, Error> Bind(
Review Comment:
My idea is that transform.h contains all the interfaces that transform users depend on, without exposing the implementation details of the transform functions.
##########
src/iceberg/transform.h:
##########
@@ -56,16 +57,98 @@ enum class TransformType {
kVoid,
};
+/// \brief Get the relative transform name
+constexpr std::string_view TransformTypeToString(TransformType type) {
+ switch (type) {
+ case TransformType::kUnknown:
+ return "unknown";
+ case TransformType::kIdentity:
+ return "identity";
+ case TransformType::kBucket:
+ return "bucket";
+ case TransformType::kTruncate:
+ return "truncate";
+ case TransformType::kYear:
+ return "year";
+ case TransformType::kMonth:
+ return "month";
+ case TransformType::kDay:
+ return "day";
+ case TransformType::kHour:
+ return "hour";
+ case TransformType::kVoid:
+ return "void";
+ }
+}
+
+/// \brief Represents a transform used in partitioning or sorting in Iceberg.
+///
+/// This class supports binding to a source type and instantiating the corresponding
+/// TransformFunction, as well as serialization-friendly introspection.
+class ICEBERG_EXPORT Transform : public util::Formattable {
+ public:
+ /// \brief Returns a shared singleton instance of the Identity transform.
+ ///
+ /// This transform leaves values unchanged and is commonly used for direct partitioning.
+ /// \return A shared pointer to the Identity transform.
+ static std::shared_ptr<Transform> Identity();
+
+ /// \brief Constructs a Transform of the specified type (for non-parametric types).
+ /// \param transform_type The transform type (e.g., identity, year, day).
+ explicit Transform(TransformType transform_type);
+
+ /// \brief Constructs a parameterized Transform (e.g., bucket(16), truncate(4)).
+ /// \param transform_type The transform type.
+ /// \param param The integer parameter associated with the transform.
+ Transform(TransformType transform_type, int32_t param);
+
+ /// \brief Returns the transform type.
+ TransformType transform_type() const;
+
+ /// \brief Binds this transform to a source type, returning a typed TransformFunction.
+ ///
+ /// This creates a concrete transform implementation based on the transform type and
+ /// parameter.
+ /// \param source_type The source column type to bind to.
+ /// \return A TransformFunction instance wrapped in `expected`, or an error on failure.
+ expected<std::unique_ptr<TransformFunction>, Error> Bind(
+ const std::shared_ptr<Type>& source_type) const;
+
+ /// \brief Returns a string representation of this transform (e.g., "bucket[16]").
+ std::string ToString() const override;
+
+ /// \brief Equality comparison.
+ friend bool operator==(const Transform& lhs, const Transform& rhs) {
+ return lhs.Equals(rhs);
+ }
+
+ /// \brief Inequality comparison.
+ friend bool operator!=(const Transform& lhs, const Transform& rhs) {
+ return !(lhs == rhs);
+ }
+
+ private:
+ /// \brief Checks equality with another Transform instance.
+ [[nodiscard]] virtual bool Equals(const Transform& other) const;
+
+ TransformType transform_type_;
+ ///< Optional parameter (e.g., num_buckets, width)
+ std::variant<std::monostate, int32_t> param_;
+};
+
/// \brief A transform function used for partitioning.
-class ICEBERG_EXPORT TransformFunction : public util::Formattable {
+class ICEBERG_EXPORT TransformFunction {
public:
- explicit TransformFunction(TransformType type);
+ virtual ~TransformFunction() = default;
+ TransformFunction(TransformType transform_type, std::shared_ptr<Type> source_type);
/// \brief Transform an input array to a new array
virtual expected<ArrowArray, Error> Transform(const ArrowArray& data) = 0;
/// \brief Get the transform type
- virtual TransformType transform_type() const;
-
- std::string ToString() const override;
+ TransformType transform_type() const;
+ /// \brief Get the source type of transform function
+ const std::shared_ptr<Type>& source_type() const;
+ /// \brief Get the result type of transform function
+ virtual expected<std::shared_ptr<Type>, Error> ResultType() const = 0;
Review Comment:
Okay, I will merge the latest code.
--
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: transform function [iceberg-cpp]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61
--
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: transform function [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2034394476
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,100 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <cstdint>
+#include <memory>
+#include <variant>
+#include <vector>
+
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a int32 value (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be empty.
+ std::vector<std::variant<int32_t>> params;
Review Comment:
I don't like the name of `TransformSpec` because it does not have a spec_id or something. What about renaming `TransformSpec` to `Transform`? For `TransformFunction`, we can use `Apply` or even `operator()` to resolve the name conflict.
--
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: transform function [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2036495200
##########
src/iceberg/transform.cc:
##########
@@ -21,55 +21,26 @@
#include <format>
-namespace iceberg {
+#include "iceberg/type.h"
-namespace {
-/// \brief Get the relative transform name
-constexpr std::string_view ToString(TransformType type) {
- switch (type) {
- case TransformType::kUnknown:
- return "unknown";
- case TransformType::kIdentity:
- return "identity";
- case TransformType::kBucket:
- return "bucket";
- case TransformType::kTruncate:
- return "truncate";
- case TransformType::kYear:
- return "year";
- case TransformType::kMonth:
- return "month";
- case TransformType::kDay:
- return "day";
- case TransformType::kHour:
- return "hour";
- case TransformType::kVoid:
- return "void";
- default:
- return "invalid";
- }
-}
-} // namespace
+namespace iceberg {
-TransformFunction::TransformFunction(TransformType type) : transform_type_(type) {}
+TransformFunction::TransformFunction(TransformType transform_type,
+ std::shared_ptr<Type> source_type)
+ : transform_type_(transform_type), source_type_(std::move(source_type)) {}
TransformType TransformFunction::transform_type() const { return transform_type_; }
-std::string TransformFunction::ToString() const {
- return std::format("{}", iceberg::ToString(transform_type_));
+std::shared_ptr<Type> const& TransformFunction::source_type() const {
Review Comment:
It seems we have two different styles for const placement: `const std::shared_ptr<Type>&` vs `std::shared_ptr<Type> const&`. Do we want to stick to one style?
You don’t need to make changes in this PR; I’m just curious if this is something we should pay attention to. @wgtmac @lidavidm WDYT
##########
src/iceberg/type_fwd.h:
##########
@@ -101,6 +101,7 @@ class StructLike;
class TableMetadata;
enum class TransformType;
class TransformFunction;
+struct TransformSpec;
Review Comment:
I don't see this struct elsewhere, do we need 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: transform function [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2034342585
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
Right, which is why I said
> you could split up the transform's type (aka the operation + parameters)[...]from the fully instantiated transform
You could even still have something like TransformSpec, I'm just arguing that it might make more sense to directly instantiate a transform-like object (with parameters, without types), just like how a Field separates the (parameterized) Type out.
--
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: transform function [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2032494097
##########
src/iceberg/transform.h:
##########
@@ -56,14 +56,45 @@ enum class TransformType {
kVoid,
};
+/// \brief Get the relative transform name
+constexpr std::string_view ToString(TransformType type) {
+ switch (type) {
+ case TransformType::kUnknown:
+ return "unknown";
+ case TransformType::kIdentity:
+ return "identity";
+ case TransformType::kBucket:
+ return "bucket";
+ case TransformType::kTruncate:
+ return "truncate";
+ case TransformType::kYear:
+ return "year";
+ case TransformType::kMonth:
+ return "month";
+ case TransformType::kDay:
+ return "day";
+ case TransformType::kHour:
+ return "hour";
+ case TransformType::kVoid:
+ return "void";
+ default:
+ return "invalid";
+ }
+}
+
/// \brief A transform function used for partitioning.
class ICEBERG_EXPORT TransformFunction : public util::Formattable {
public:
- explicit TransformFunction(TransformType type);
+ explicit TransformFunction(TransformType transform_type,
Review Comment:
We can remove `explicit` now
##########
src/iceberg/transform.h:
##########
@@ -56,14 +56,45 @@ enum class TransformType {
kVoid,
};
+/// \brief Get the relative transform name
+constexpr std::string_view ToString(TransformType type) {
+ switch (type) {
+ case TransformType::kUnknown:
+ return "unknown";
+ case TransformType::kIdentity:
+ return "identity";
+ case TransformType::kBucket:
+ return "bucket";
+ case TransformType::kTruncate:
+ return "truncate";
+ case TransformType::kYear:
+ return "year";
+ case TransformType::kMonth:
+ return "month";
+ case TransformType::kDay:
+ return "day";
+ case TransformType::kHour:
+ return "hour";
+ case TransformType::kVoid:
+ return "void";
+ default:
Review Comment:
It seems that we can remove the `default` branch
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
I was thinking about the following approach:
```cpp
struct TransformUtil {
// TODO: source and result types may be required here
static expected<ArrowArray, Error> Copy(const ArrowArray& input);
static expected<ArrowArray, Error> Bucket(const ArrowArray& input, int32_t num_buckets);
static expected<ArrowArray, Error> Truncate(const ArrowArray& input, int32_t width);
};
class Transform {
public:
virtual ~Transform() = default;
expected<ArrowArray, Error> Apply(const ArrowArray& input) const {
return transform_function_(input);
}
protected:
Transform(
TransformType transform_type, std::shared_ptr<Type> source_type,
std::shared_ptr<Type> result_type,
std::function<expected<ArrowArray, Error>(const ArrowArray&)> transform_function)
: Transform(transform_type,
std::vector<std::shared_ptr<Type>>{std::move(source_type)},
std::move(result_type), std::move(transform_function)) {}
Transform(
TransformType transform_type, std::vector<std::shared_ptr<Type>> source_types,
std::shared_ptr<Type> result_type,
std::function<expected<ArrowArray, Error>(const ArrowArray&)> transform_function)
: transform_type_(transform_type),
source_types_(std::move(source_types)),
result_type_(std::move(result_type)),
transform_function_(std::move(transform_function)) {}
private:
TransformType transform_type_;
std::vector<std::shared_ptr<Type>> source_types_;
std::shared_ptr<Type> result_type_;
std::function<expected<ArrowArray, Error>(const ArrowArray&)> transform_function_;
};
class IdentityTransform : public Transform {
public:
explicit IdentityTransform(std::shared_ptr<Type> source_type)
: Transform(TransformType::kIdentity, source_type, source_type,
[](const ArrowArray& input) -> expected<ArrowArray, Error> {
return TransformUtil::Copy(input);
}) {}
};
class BucketTransform : public Transform {
public:
BucketTransform(std::shared_ptr<Type> source_type, int32_t num_buckets)
: BucketTransform(std::vector<std::shared_ptr<Type>>{std::move(source_type)},
num_buckets) {}
BucketTransform(std::vector<std::shared_ptr<Type>> source_types, int32_t num_buckets)
: Transform(TransformType::kBucket, std::move(source_types),
std::make_shared<IntType>(),
[num_buckets](const ArrowArray& input) -> expected<ArrowArray, Error> {
return TransformUtil::Bucket(input, num_buckets);
}),
num_buckets_{num_buckets} {}
int32_t num_buckets() const { return num_buckets_; }
private:
int32_t num_buckets_;
};
```
WDYT? @gty404 @lidavidm @yingcai-cy
##########
src/iceberg/arrow_c_data.h:
##########
@@ -73,4 +73,44 @@ struct ArrowArray {
#endif // ARROW_C_DATA_INTERFACE
+#ifndef ARROW_C_STREAM_INTERFACE
+# define ARROW_C_STREAM_INTERFACE
+
+struct ArrowArrayStream {
Review Comment:
Why do we need 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: transform function [iceberg-cpp]
Posted by "gty404 (via GitHub)" <gi...@apache.org>.
gty404 commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2033080461
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
> It somewhat feels like overengineering to me; there are only two transforms listed with parameters, and in both it's a single integer. Allocating an entire Arrow table to represent this seems like extreme overkill and will make the code much more complicated for that use case, and as noted here it causes lifetime issues too.
From the perspective of complexity and meeting current needs, it is a bit over-designed. No problem, I will revise this part again.
--
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: transform function [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2033074455
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
It somewhat feels like overengineering to me; there are only two transforms listed with parameters, and in both it's a single integer. Allocating an entire Arrow table to represent this seems like extreme overkill and will make the code much more complicated for that use case, and as noted here it causes lifetime issues too.
--
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: transform function [iceberg-cpp]
Posted by "gty404 (via GitHub)" <gi...@apache.org>.
gty404 commented on code in PR #61:
URL: https://github.com/apache/iceberg-cpp/pull/61#discussion_r2032908900
##########
src/iceberg/transform.h:
##########
@@ -56,14 +56,45 @@ enum class TransformType {
kVoid,
};
+/// \brief Get the relative transform name
+constexpr std::string_view ToString(TransformType type) {
+ switch (type) {
+ case TransformType::kUnknown:
+ return "unknown";
+ case TransformType::kIdentity:
+ return "identity";
+ case TransformType::kBucket:
+ return "bucket";
+ case TransformType::kTruncate:
+ return "truncate";
+ case TransformType::kYear:
+ return "year";
+ case TransformType::kMonth:
+ return "month";
+ case TransformType::kDay:
+ return "day";
+ case TransformType::kHour:
+ return "hour";
+ case TransformType::kVoid:
+ return "void";
+ default:
+ return "invalid";
+ }
+}
+
/// \brief A transform function used for partitioning.
class ICEBERG_EXPORT TransformFunction : public util::Formattable {
public:
- explicit TransformFunction(TransformType type);
+ explicit TransformFunction(TransformType transform_type,
+ std::shared_ptr<Type> source_type);
/// \brief Transform an input array to a new array
virtual expected<ArrowArray, Error> Transform(const ArrowArray& data) = 0;
/// \brief Get the transform type
- virtual TransformType transform_type() const;
+ TransformType transform_type() const;
+ /// \brief Get the source type of transform function
+ std::shared_ptr<Type> source_type() const;
+ /// \brief Get the result type of transform function
+ virtual expected<std::shared_ptr<Type>, Error> result_type() const = 0;
Review Comment:
Reasonable, I will modify it. Thank you.
##########
src/iceberg/transform.h:
##########
@@ -56,14 +56,45 @@ enum class TransformType {
kVoid,
};
+/// \brief Get the relative transform name
+constexpr std::string_view ToString(TransformType type) {
+ switch (type) {
+ case TransformType::kUnknown:
+ return "unknown";
+ case TransformType::kIdentity:
+ return "identity";
+ case TransformType::kBucket:
+ return "bucket";
+ case TransformType::kTruncate:
+ return "truncate";
+ case TransformType::kYear:
+ return "year";
+ case TransformType::kMonth:
+ return "month";
+ case TransformType::kDay:
+ return "day";
+ case TransformType::kHour:
+ return "hour";
+ case TransformType::kVoid:
+ return "void";
+ default:
+ return "invalid";
+ }
+}
+
/// \brief A transform function used for partitioning.
class ICEBERG_EXPORT TransformFunction : public util::Formattable {
public:
- explicit TransformFunction(TransformType type);
+ explicit TransformFunction(TransformType transform_type,
+ std::shared_ptr<Type> source_type);
/// \brief Transform an input array to a new array
virtual expected<ArrowArray, Error> Transform(const ArrowArray& data) = 0;
/// \brief Get the transform type
- virtual TransformType transform_type() const;
+ TransformType transform_type() const;
+ /// \brief Get the source type of transform function
+ std::shared_ptr<Type> source_type() const;
Review Comment:
Reasonable, I will modify it. Thank you.
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
1. Using ArrowArray is necessary, as different Transforms may use different types and numbers of parameters, which should be needed within the Transform or when creating an instance of the Transform.
2. TransformSpec is used to describe a transform, which can be derived from JSON and is also convenient for future extensions.
@lidavidm
##########
src/iceberg/arrow_c_data.h:
##########
@@ -73,4 +73,44 @@ struct ArrowArray {
#endif // ARROW_C_DATA_INTERFACE
+#ifndef ARROW_C_STREAM_INTERFACE
+# define ARROW_C_STREAM_INTERFACE
+
+struct ArrowArrayStream {
Review Comment:
I have the impression that I relied on it when testing and copying ArrowArray, and I would delete it
##########
src/iceberg/transform.h:
##########
@@ -56,14 +56,45 @@ enum class TransformType {
kVoid,
};
+/// \brief Get the relative transform name
+constexpr std::string_view ToString(TransformType type) {
+ switch (type) {
+ case TransformType::kUnknown:
+ return "unknown";
+ case TransformType::kIdentity:
+ return "identity";
+ case TransformType::kBucket:
+ return "bucket";
+ case TransformType::kTruncate:
+ return "truncate";
+ case TransformType::kYear:
+ return "year";
+ case TransformType::kMonth:
+ return "month";
+ case TransformType::kDay:
+ return "day";
+ case TransformType::kHour:
+ return "hour";
+ case TransformType::kVoid:
+ return "void";
+ default:
+ return "invalid";
+ }
+}
+
/// \brief A transform function used for partitioning.
class ICEBERG_EXPORT TransformFunction : public util::Formattable {
public:
- explicit TransformFunction(TransformType type);
+ explicit TransformFunction(TransformType transform_type,
Review Comment:
Reasonable, I will modify it. Thank you.
##########
src/iceberg/transform/transform_spec.h:
##########
@@ -0,0 +1,117 @@
+/*
+ * 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
+
+/// \file iceberg/transform/transform_spec.h
+
+#include <memory>
+#include <optional>
+
+#include "iceberg/arrow_c_data.h"
+#include "iceberg/type_fwd.h"
+
+namespace iceberg {
+/// \brief A specification for creating a TransformFunction instance.
+///
+/// This struct encapsulates all necessary information to describe a transform,
+/// including the type of the transform and any optional parameters it may require.
+/// It is used by the TransformFactory to instantiate the corresponding transform
+/// implementation.
+///
+/// For example:
+/// - A Bucket transform with 10 buckets: TransformType::kBucket, params = [10]
+/// - An Identity transform: TransformType::kIdentity, no parameters
+struct TransformSpec {
+ /// \brief The type of the transform (e.g., kBucket, kTruncate, kIdentity, etc.)
+ TransformType transform_type;
+
+ /// \brief Optional parameter values passed to the transform, represented as an
+ /// ArrowArray.
+ ///
+ /// For transforms that require parameters (e.g., Bucket(N)), this holds the arguments
+ /// as a primitive ArrowArray (e.g., INT32 for num_buckets or width).
+ /// If the transform does not require parameters, this will be std::nullopt.
+ std::optional<ArrowArray> params_opt;
Review Comment:
The main difference lies in the fact that your approach sub-classes uniformly provide a lambda function. I am concerned that the function might become too complex, as the same Transform type may need to implement corresponding logic based on different source types. However, iceberg-java has a similar approach; it generates specific function objects by binding the source type. @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