feat: add partition field/partition spec by gty404 · Pull Request #54 · apache/iceberg-cpp

gty404 · 2025-03-30T15:14:43Z

Implement the API for partition spec/field.
add pure virtual classes for transform

lidavidm · 2025-03-31T03:49:32Z

src/iceberg/partition_spec.h

+  [[nodiscard]] int32_t spec_id() const;
+  /// \brief Get a view of the partition fields.
+  [[nodiscard]] virtual std::span<const PartitionField> fields() const;
+  /// \brief Get a field by field ID.


Should we clarify whether these are source/transform field IDs?

Perhaps we don't need these GetFieldByXXX at this moment?

Perhaps we don't need these GetFieldByXXX at this moment?

+1

Should we clarify whether these are source/transform field IDs?

I will change it to transform_fields, and the PartitionField class will define source_field_id/transform_field_id

Perhaps we don't need these GetFieldByXXX at this moment?

Okay, I'll delete this part of the interface first

From the spec, it seems we should keep the fields.

Yes, fields are required

lidavidm · 2025-03-31T03:50:43Z

src/iceberg/partition_field.h

+  /// \brief Get the transform type.
+  [[nodiscard]] TransformType transform_type() const;


Java stores the full transform, not the type; do we need to do the same?

I think so. Some transform types required additional parameters like bucket number or truncate length.

I will change it to TransformFunction

wgtmac · 2025-03-31T03:59:34Z

src/iceberg/transform.h

+#include <cstdint>
+#include <memory>
+
+#include "iceberg/arrow_c_data_internal.h"


Do not include any internal header in the public header file. To use Arrow C data, we can include arrow_c_data.h.

wgtmac · 2025-03-31T04:09:25Z

src/iceberg/transform.h

+  /// Always produces `null`
+  kVoid,
+  /// Used to represent some customized transform that can't be recognized or supported now.
+  kUnknown,


What about putting kUnknown to the beginning?

src/iceberg/transform.h

wgtmac · 2025-03-31T06:08:27Z

src/iceberg/transform.h

+ public:
+  explicit TransformFunction(TransformType type);
+  /// \brief Transform an input array to a new array
+  [[nodiscard]] virtual expected<std::unique_ptr<ArrowArray>, Error> Transform(const ArrowArray& inputArrowArray) = 0;


Suggested change

[[nodiscard]] virtual expected<std::unique_ptr<ArrowArray>, Error> Transform(const ArrowArray& inputArrowArray) = 0;

virtual expected<ArrowArray, Error> Transform(const ArrowArray& data) = 0;

expected already has nodiscard attribute in its definition, we don't need it here.

For ArrowArray, usually we don't use std::unique_ptr since we have to explicitly call its release function.

wgtmac · 2025-03-31T06:09:01Z

src/iceberg/transform.h

+  [[nodiscard]] virtual TransformType transform_type() const;
+  [[nodiscard]] std::string ToString() const override;


Suggested change

[[nodiscard]] virtual TransformType transform_type() const;

[[nodiscard]] std::string ToString() const override;

virtual TransformType transform_type() const;

std::string ToString() const override;

I don't think we need to add nodiscard to these functions.

wgtmac · 2025-03-31T06:23:01Z

src/iceberg/partition_spec.h

+  [[nodiscard]] int32_t spec_id() const;
+  /// \brief Get a view of the partition fields.
+  [[nodiscard]] virtual std::span<const PartitionField> fields() const;
+  /// \brief Get a field by field ID.


Perhaps we don't need these GetFieldByXXX at this moment?

wgtmac · 2025-03-31T06:25:08Z

src/iceberg/partition_spec.h

+class ICEBERG_EXPORT PartitionSpec {
+ public:
+  virtual ~PartitionSpec() = default;
+  PartitionSpec(int32_t spec_id, std::vector<PartitionField> fields);


IMO, we need an Schema in the constructor to denote to the partition schema.

wgtmac · 2025-03-31T06:26:11Z

test/transform_test.cc

+  {
+    EXPECT_EQ("Identity", iceberg::ToString(iceberg::TransformType::kIdentity));
+    EXPECT_EQ("Bucket", iceberg::ToString(iceberg::TransformType::kBucket));
+    EXPECT_EQ("Truncate", iceberg::ToString(iceberg::TransformType::kTruncate));
+    EXPECT_EQ("Year", iceberg::ToString(iceberg::TransformType::kYear));
+    EXPECT_EQ("Month", iceberg::ToString(iceberg::TransformType::kMonth));
+    EXPECT_EQ("Day", iceberg::ToString(iceberg::TransformType::kDay));
+    EXPECT_EQ("Hour", iceberg::ToString(iceberg::TransformType::kHour));
+    EXPECT_EQ("Void", iceberg::ToString(iceberg::TransformType::kVoid));
+    EXPECT_EQ("Unknown", iceberg::ToString(iceberg::TransformType::kUnknown));
+  }


Suggested change

{

EXPECT_EQ("Identity", iceberg::ToString(iceberg::TransformType::kIdentity));

EXPECT_EQ("Bucket", iceberg::ToString(iceberg::TransformType::kBucket));

EXPECT_EQ("Truncate", iceberg::ToString(iceberg::TransformType::kTruncate));

EXPECT_EQ("Year", iceberg::ToString(iceberg::TransformType::kYear));

EXPECT_EQ("Month", iceberg::ToString(iceberg::TransformType::kMonth));

EXPECT_EQ("Day", iceberg::ToString(iceberg::TransformType::kDay));

EXPECT_EQ("Hour", iceberg::ToString(iceberg::TransformType::kHour));

EXPECT_EQ("Void", iceberg::ToString(iceberg::TransformType::kVoid));

EXPECT_EQ("Unknown", iceberg::ToString(iceberg::TransformType::kUnknown));

}

EXPECT_EQ("Identity", iceberg::ToString(iceberg::TransformType::kIdentity));

EXPECT_EQ("Bucket", iceberg::ToString(iceberg::TransformType::kBucket));

EXPECT_EQ("Truncate", iceberg::ToString(iceberg::TransformType::kTruncate));

EXPECT_EQ("Year", iceberg::ToString(iceberg::TransformType::kYear));

EXPECT_EQ("Month", iceberg::ToString(iceberg::TransformType::kMonth));

EXPECT_EQ("Day", iceberg::ToString(iceberg::TransformType::kDay));

EXPECT_EQ("Hour", iceberg::ToString(iceberg::TransformType::kHour));

EXPECT_EQ("Void", iceberg::ToString(iceberg::TransformType::kVoid));

EXPECT_EQ("Unknown", iceberg::ToString(iceberg::TransformType::kUnknown));

Add the iceberg namespace so we don't have to add iceberg:: everywhere.

wgtmac · 2025-03-31T06:27:48Z

test/transform_test.cc

+  EXPECT_EQ("Unknown", std::format("{}", transform));
+
+  auto [_, arrow_array] =
+      iceberg::internal::CreateExampleArrowSchemaAndArrayByNanoarrow();


I will remove CreateExampleArrowSchemaAndArrayByNanoarrow some day. Perhaps add a TODO here?

Okay, I'll build a simple arrow array to test

wgtmac · 2025-03-31T06:30:25Z

src/iceberg/exception.h

  explicit IcebergError(const std::string& what) : std::runtime_error(what) {}
 };

+#define ICEBERG_CHECK_FMT(condition, ...)                    \


Suggested change

#define ICEBERG_CHECK_FMT(condition, ...) \

#define ICEBERG_CHECK(condition, ...) \

yingcai-cy · 2025-03-31T09:08:22Z

src/iceberg/partition_field.h

+  /// \param[in] field_id The partition field ID.
+  /// \param[in] name The partition field name.
+  /// \param[in] transformType The transform type.
+  PartitionField(int32_t source_id, int32_t field_id, std::string name, TransformType transformType);


source_id->source_field_id? keep consistent with impl

I think it is ok to be consistent with the spec: https://iceberg.apache.org/spec/#partition-specs

| V1 | V2 | V3 | Field | JSON representation | Example | |----------|----------|----------|------------------|---------------------|--------------| | required | required | omitted | **`source-id`** | `JSON int` | 1 | | | | required | **`source-ids`** | `JSON list of ints` | `[1,2]` | | | required | required | **`field-id`** | `JSON int` | 1000 | | required | required | required | **`name`** | `JSON string` | `id_bucket` | | required | required | required | **`transform`** | `JSON string` | `bucket[16]` |

iceberg-java used Transform, iceberg-rust used Transform, which is an enum type, and TransformFunction serves as the implementation interface for Transform. Which one does iceberg-cpp tend to use? @wgtmac @yingcai-cy

src/iceberg/CMakeLists.txt

src/iceberg/transform.h

src/iceberg/partition_spec.h

src/iceberg/partition_spec.cc

src/iceberg/partition_field.h

src/iceberg/transform.h

test/partition_field_test.cc

wgtmac

+1

wgtmac · 2025-04-01T06:59:00Z

@lidavidm @zhjwpku @yingcai-cy Do you want to take a look?

src/iceberg/transform.h

zhjwpku

Looking good, thanks.

wgtmac · 2025-04-01T09:01:04Z

@Fokko @Xuanwo Could you help review and merge it?

src/iceberg/transform.h

lidavidm reviewed Mar 31, 2025

View reviewed changes

wgtmac reviewed Mar 31, 2025

View reviewed changes

yingcai-cy reviewed Mar 31, 2025

View reviewed changes

gty404 force-pushed the table-metadata branch from bfe3152 to 720f908 Compare March 31, 2025 16:22

wgtmac reviewed Apr 1, 2025

View reviewed changes

gty404 force-pushed the table-metadata branch 4 times, most recently from 58e0b5e to be03f0a Compare April 1, 2025 06:42

wgtmac approved these changes Apr 1, 2025

View reviewed changes

gty404 changed the title ~~Add partition field/partition spec~~ feat: add partition field/partition spec Apr 1, 2025

lidavidm reviewed Apr 1, 2025

View reviewed changes

src/iceberg/transform.h Outdated Show resolved Hide resolved

gty404 force-pushed the table-metadata branch from be03f0a to ccbd930 Compare April 1, 2025 07:44

lidavidm approved these changes Apr 1, 2025

View reviewed changes

zhjwpku approved these changes Apr 1, 2025

View reviewed changes

Fokko reviewed Apr 1, 2025

View reviewed changes

src/iceberg/transform.h Outdated Show resolved Hide resolved

Fokko reviewed Apr 1, 2025

View reviewed changes

src/iceberg/transform.h Outdated Show resolved Hide resolved

gty404 force-pushed the table-metadata branch from ccbd930 to 01f53b8 Compare April 1, 2025 09:53

Add partition field/partition spec/transform

a6c3f17

gty404 force-pushed the table-metadata branch from 01f53b8 to a6c3f17 Compare April 1, 2025 10:14

Fokko approved these changes Apr 1, 2025

View reviewed changes

Fokko merged commit 2912352 into apache:main Apr 1, 2025
6 checks passed

gty404 deleted the table-metadata branch April 1, 2025 15:14

		/// \brief Get the transform type.
		[[nodiscard]] TransformType transform_type() const;

	[[nodiscard]] virtual expected<std::unique_ptr<ArrowArray>, Error> Transform(const ArrowArray& inputArrowArray) = 0;
	virtual expected<ArrowArray, Error> Transform(const ArrowArray& data) = 0;

		[[nodiscard]] virtual TransformType transform_type() const;
		[[nodiscard]] std::string ToString() const override;

	#define ICEBERG_CHECK_FMT(condition, ...) \
	#define ICEBERG_CHECK(condition, ...) \

Conversation

gty404 commented Mar 30, 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!

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!

wgtmac Mar 31, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Apr 1, 2025

Uh oh!

Uh oh!

zhjwpku left a comment

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Apr 1, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

wgtmac Mar 31, 2025 •

edited

Loading