feat: transform function #61

gty404 · 2025-04-08T05:31:05Z

No description provided.

src/iceberg/transform/transform_factory.h

src/iceberg/transform.h

lidavidm · 2025-04-08T06:22:31Z

src/iceberg/transform/transform_spec.h

+  /// 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;


Hmm, do the parameters need to be Arrow data? I suppose what we need is a standard for scalars or row-wise data...

Do we actually need TransformSpec though?

I was thinking about the following approach:

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

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.

TransformSpec is used to describe a transform, which can be derived from JSON and is also convenient for future extensions.

@lidavidm

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

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?

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.

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?

That sounds good to me. @wgtmac?

src/iceberg/transform.h

src/iceberg/arrow_c_data.h

src/iceberg/transform.h

wgtmac · 2025-04-08T07:15:50Z

src/iceberg/transform/transform_spec.h

+  /// 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;


I was thinking about the following approach:

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

wgtmac · 2025-04-09T03:15:29Z

src/iceberg/transform/transform_spec.h

+  /// 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;


Why not simply std::variant<std::monostate, int32_t> param?

The only difference between TransformFunction and TransformSpec is this addition param. Should we delete TransformSpec by adding param to TransformFunction?

Could there be multiple param?

TransformSpec is just the result after json parsing, and TransformFunction is the specific implementation for the transformation.

For each transform function, there is unlikely to have more than one param in the near future. Am I correct, @Fokko?

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.

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?

I suppose UnboundTransform will be used in the expression

src/iceberg/transform.cc

zhjwpku · 2025-04-10T05:10:41Z

src/iceberg/type_fwd.h

 class TableMetadata;
 enum class TransformType;
 class TransformFunction;
+struct TransformSpec;


I don't see this struct elsewhere, do we need this?

It's not needed anymore, I will delete it.

lidavidm · 2025-04-10T06:30:19Z

test/transform_test.cc

-  EXPECT_EQ(TransformType::kUnknown, transform.transform_type());
-  EXPECT_EQ("unknown", transform.ToString());
-  EXPECT_EQ("unknown", std::format("{}", transform));
+  IdentityTransform transform{std::make_shared<StringType>()};


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)

gty404 · 2025-04-10T12:15:37Z

I made the following modifications based on the previous comment:

The Transform object is used for JSON serialization/deserialization.
Transform::Bind can get the TransformFunction, which is used for data transformation.

Please review in your spare time. Thanks @lidavidm @wgtmac @zhjwpku

lidavidm · 2025-04-11T02:17:18Z

src/iceberg/transform.h

+};
+
 /// \brief A transform function used for partitioning.
-class ICEBERG_EXPORT TransformFunction : public util::Formattable {


nit: why not formattable anymore?

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.

Sounds good to me.

wgtmac · 2025-04-11T15:15:33Z

src/iceberg/transform.h

+  /// 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(


Can we use forward declaration here and define TransformFunction in the transform_function.h?

My idea is that transform.h contains all the interfaces that transform users depend on, without exposing the implementation details of the transform functions.

wgtmac · 2025-04-11T15:16:59Z

src/iceberg/transform.cc

+  return instance;
+}
+
+Transform::Transform(TransformType transform_type) : transform_type_(transform_type) {}


Throw for parameterized transform_type?

wgtmac · 2025-04-11T15:17:19Z

src/iceberg/transform.cc

+
+Transform::Transform(TransformType transform_type) : transform_type_(transform_type) {}
+
+Transform::Transform(TransformType transform_type, int32_t param)


Throw for non-parameterized transform_type?

If we want to avoid invalid inputs. Perhaps we should define a separate static functions to create each transform type?

Yes, currently I only added Transform::Identity, I will add the others as well.

wgtmac · 2025-04-11T15:21:01Z

src/iceberg/transform.cc

+
+TransformType Transform::transform_type() const { return transform_type_; }
+
+expected<std::unique_ptr<TransformFunction>, Error> Transform::Bind(


It seems that we should merge transform_function.h/cc to transform.h/cc. They have strong connections.

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

wgtmac · 2025-04-12T14:33:17Z

src/iceberg/transform.h

+  /// 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(


Suggested change

expected<std::unique_ptr<TransformFunction>, Error> Bind(

Result<std::unique_ptr<TransformFunction>> Bind(

nit: we can leave these as-is and fix all together in a separate PR.

gty404 · 2025-04-14T01:53:56Z

@Fokko @Xuanwo Could you help review and merge this? Thanks!

Xuanwo

The community has reached a consensus. Let's go!

gty404 force-pushed the identity-transform branch from 67a266d to bb94c3f Compare April 8, 2025 06:14

lidavidm reviewed Apr 8, 2025

View reviewed changes

wgtmac reviewed Apr 8, 2025

View reviewed changes

gty404 force-pushed the identity-transform branch 3 times, most recently from b45e466 to 328880b Compare April 8, 2025 14:28

wgtmac reviewed Apr 9, 2025

View reviewed changes

gty404 force-pushed the identity-transform branch 2 times, most recently from c3ac6c7 to df894f2 Compare April 9, 2025 13:27

zhjwpku reviewed Apr 10, 2025

View reviewed changes

lidavidm reviewed Apr 10, 2025

View reviewed changes

gty404 force-pushed the identity-transform branch 2 times, most recently from 7ad56db to cb153d4 Compare April 10, 2025 11:51

gty404 force-pushed the identity-transform branch from cb153d4 to e6b2fc9 Compare April 10, 2025 15:21

lidavidm approved these changes Apr 11, 2025

View reviewed changes

wgtmac reviewed Apr 11, 2025

View reviewed changes

gty404 force-pushed the identity-transform branch from e6b2fc9 to 39148d0 Compare April 12, 2025 02:11

wgtmac approved these changes Apr 12, 2025

View reviewed changes

gty404 added 5 commits April 13, 2025 09:28

feat: transform function

795c1d3

fix clint

392a1d1

fix clint

fed1d56

fix: windows build failure

350e5eb

fix: merge latest code from main

5a46de2

gty404 force-pushed the identity-transform branch from 3e539be to 5a46de2 Compare April 13, 2025 01:30

fix: Use Result/Status as the return value type

9f21cb4

Xuanwo approved these changes Apr 14, 2025

View reviewed changes

Xuanwo merged commit 185515a into apache:main Apr 14, 2025
6 checks passed

gty404 deleted the identity-transform branch April 14, 2025 11:31


		Transform::Transform(TransformType transform_type) : transform_type_(transform_type) {}

		Transform::Transform(TransformType transform_type, int32_t param)


		TransformType Transform::transform_type() const { return transform_type_; }

		expected<std::unique_ptr<TransformFunction>, Error> Transform::Bind(

	expected<std::unique_ptr<TransformFunction>, Error> Bind(
	Result<std::unique_ptr<TransformFunction>> Bind(

feat: transform function #61

feat: transform function #61

Conversation

gty404 commented Apr 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wgtmac Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gty404 commented Apr 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac Apr 8, 2025 •

edited

Loading

wgtmac Apr 8, 2025 •

edited

Loading