refactor: decouple PartitionSpec from Schema #299

HeartLinked · 2025-11-07T11:36:11Z

Removing schema_ member from PartitionSpec, Making PartitionType() method require a Schema parameter instead of using the stored schema.
Add table_schema_ in ManifestEntryAdapter.
Fix bug in ManifestEntryAdapter::init(v1/v2/v3) to use table_schema in metadata_["schema"].

wgtmac · 2025-11-07T15:39:37Z

src/iceberg/json_internal.cc

    partition_fields.push_back(std::move(*partition_field));
  }
-  return std::make_unique<PartitionSpec>(schema, spec_id, std::move(partition_fields));
+  return std::make_unique<PartitionSpec>(spec_id, std::move(partition_fields));


As discussed offline, we cannot use the unbound form here. Instead, we need to use a new PartitionSpec::Make to find the source field of each partition field from schema and then verify it is acceptable by the transform object.

wgtmac

My main feedback is that we need to keep the schema parameter because we eventually need it to create bound partition spec.

wgtmac · 2025-11-08T03:56:27Z

src/iceberg/json_internal.cc


-Result<std::unique_ptr<PartitionSpec>> PartitionSpecFromJson(
-    const std::shared_ptr<Schema>& schema, const nlohmann::json& json) {
+Result<std::unique_ptr<PartitionSpec>> PartitionSpecFromJson(const nlohmann::json& json) {


Let's keep the function signature to still accept const std::shared_ptr<Schema>& schema and add a TODO comment below for PartitionSpec to validate the partition fields with this schema. Otherwise you need to add this parameter back in the next PR.

wgtmac · 2025-11-08T03:57:15Z

src/iceberg/json_internal.cc

 /// \param[out] default_spec_id The default partition spec ID.
 /// \param[out] partition_specs The list of partition specs.
 Status ParsePartitionSpecs(const nlohmann::json& json, int8_t format_version,
-                           const std::shared_ptr<Schema>& current_schema,


ditto, let's keep it.

wgtmac · 2025-11-08T03:57:25Z

src/iceberg/json_internal.cc

    for (const auto& spec_json : spec_array) {
-      ICEBERG_ASSIGN_OR_RAISE(auto spec,
-                              PartitionSpecFromJson(current_schema, spec_json));
+      ICEBERG_ASSIGN_OR_RAISE(auto spec, PartitionSpecFromJson(spec_json));


wgtmac · 2025-11-08T03:58:00Z

src/iceberg/json_internal.cc


-    auto spec = std::make_unique<PartitionSpec>(
-        current_schema, PartitionSpec::kInitialSpecId, std::move(fields));
+    auto spec =


add a TODO comment here as well.

wgtmac · 2025-11-08T03:58:12Z

src/iceberg/json_internal.cc

-  ICEBERG_RETURN_UNEXPECTED(ParsePartitionSpecs(
-      json, table_metadata->format_version, current_schema,
-      table_metadata->default_spec_id, table_metadata->partition_specs));
+  ICEBERG_RETURN_UNEXPECTED(ParsePartitionSpecs(json, table_metadata->format_version,


wgtmac · 2025-11-08T03:59:36Z

src/iceberg/manifest_adapter.h

+  ManifestEntryAdapter(std::shared_ptr<PartitionSpec> partition_spec,
+                       std::shared_ptr<Schema> table_schema)
+      : partition_spec_(std::move(partition_spec)),
+        table_schema_(std::move(table_schema)) {}


Suggested change

table_schema_(std::move(table_schema)) {}

current_schema_(std::move(current_schema)) {}

Let's avoid calling it table schema because a table can have schemas of different versions.

wgtmac · 2025-11-08T04:00:25Z

src/iceberg/manifest_writer.cc

-  auto adapter =
-      std::make_unique<ManifestEntryAdapterV1>(snapshot_id, std::move(partition_spec));
+    std::shared_ptr<FileIO> file_io, std::shared_ptr<PartitionSpec> partition_spec,
+    std::shared_ptr<Schema> table_schema) {


ditto for the name of table_schema

wgtmac · 2025-11-08T04:01:31Z

src/iceberg/partition_spec.cc


-Result<std::shared_ptr<StructType>> PartitionSpec::PartitionType() {
+Result<std::shared_ptr<StructType>> PartitionSpec::PartitionType(
+    std::shared_ptr<Schema> schema) {


Suggested change

std::shared_ptr<Schema> schema) {

const std::shared_ptr<Schema>& schema) {

Both const std::shared_ptr<Schema>& or const Schema& is OK. Perhaps the latter is better since it accepts wider range of inputs.

wgtmac · 2025-11-08T04:02:03Z

src/iceberg/partition_spec.h


  /// \brief Get the partition type.
-  Result<std::shared_ptr<StructType>> PartitionType();
+  Result<std::shared_ptr<StructType>> PartitionType(std::shared_ptr<Schema> schema);


Suggested change

Result<std::shared_ptr<StructType>> PartitionType(std::shared_ptr<Schema> schema);

Result<std::shared_ptr<StructType>> PartitionType(const Schema& schema);

wgtmac · 2025-11-08T04:02:35Z

src/iceberg/table_scan.cc

+  // Get the table schema and partition type
+  ICEBERG_ASSIGN_OR_RAISE(auto table_schema, context_.table_metadata->Schema());
+  ICEBERG_ASSIGN_OR_RAISE(auto partition_type,
+                          partition_spec->PartitionType(table_schema));


Suggested change

// Get the table schema and partition type

ICEBERG_ASSIGN_OR_RAISE(auto table_schema, context_.table_metadata->Schema());

ICEBERG_ASSIGN_OR_RAISE(auto partition_type,

partition_spec->PartitionType(table_schema));

// Get the current schema and partition type

ICEBERG_ASSIGN_OR_RAISE(auto current_schema, context_.table_metadata->Schema());

ICEBERG_ASSIGN_OR_RAISE(auto partition_type,

partition_spec->PartitionType(current_schema));

lishuxu · 2025-11-08T13:45:20Z

src/iceberg/partition_spec.cc

+    std::shared_ptr<Schema> schema) {
  if (fields_.empty()) {
    return nullptr;
  }


The current implementation allows using partition_type_ cache based on tableschema_, but the given schema parameter may belong to a different schema version, which could lead to correctness issues.
Suggest adding schema_id to the cache key to ensure consistency.

such as:
std::unordered_map<int32_t, std::shared_ptr> partition_type_cache_;

refactor: decouple PartitionSpec from Schema

bb499ad

HeartLinked force-pushed the partition_spec_1 branch from 335a0da to bb499ad Compare November 7, 2025 11:48

wgtmac reviewed Nov 7, 2025

View reviewed changes

wgtmac requested changes Nov 8, 2025

View reviewed changes

lishuxu reviewed Nov 8, 2025

View reviewed changes

	table_schema_(std::move(table_schema)) {}
	current_schema_(std::move(current_schema)) {}

	std::shared_ptr<Schema> schema) {
	const std::shared_ptr<Schema>& schema) {

	Result<std::shared_ptr<StructType>> PartitionType(std::shared_ptr<Schema> schema);
	Result<std::shared_ptr<StructType>> PartitionType(const Schema& schema);

refactor: decouple PartitionSpec from Schema #299

Are you sure you want to change the base?

refactor: decouple PartitionSpec from Schema #299

Conversation

HeartLinked commented Nov 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants