Partition Evolution

[amogh-jahagirdar]

Fixes #193

[amogh-jahagirdar]

There's still a lot of cleanup required, and need to add unit tests, also some bugs I'm encountering. But I'm putting up this draft since the core pieces are here.

[Fokko]

Nice to have: Do we also want to have a single dispatch to map these types?

[Fokko]

Can you create an issue for this?

[amogh-jahagirdar]

@Fokko @HonahX I added this API to the Table since we'll need it in the implementation and we don't want to directly access TableMetadata let me know what you think

[HonahX]

I think we probably should update

iceberg-python/pyiceberg/partitioning.py

Lines 148 to 152 in 7fbcc22

	@property
	def last_assigned_field_id(self) -> int:
	if self.fields:
	return max(pf.field_id for pf in self.fields)
	return PARTITION_FIELD_ID_START

to return PARTITION_FIELD_ID_START - 1 for unpartitioned spec. Then we can return the last_partition_id from metadata directly because the metadata should have last_partition_id=999 for unpartitioned table.

Java implementation uses PARTITION_FIELD_ID_START - 1 for unpartitioned spec:
https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/PartitionSpec.java#L344-L345
https://github.com/apache/iceberg/blob/9921937d8285dec9a19fd16b0cd82d451a8aca9e/api/src/main/java/org/apache/iceberg/PartitionSpec.java#L319-L321

I checked locally that unpartitioned tables created by spark-iceberg-runtime have last_partition_id=999, while those created by pyiceberg have last_partition_id=1000:

[amogh-jahagirdar]

I was thinking about that but wasn't sure about breaking API behavior which is why I added a new API. If we have the flexibility here to change the API we should do that. I think we can because arguably it's incorrect to return 1000 for an unpartitioned table so it's really a fix.

[amogh-jahagirdar]

This test shows why assigning new field IDs based on the last field ID across all specs is important to avoid collisions. In this case if we just used the last spec, after the remove_field is done for the original partitions, the latest spec would just be the unpartitioned spec. Then when we go and add the new truncated_str partitioned field, we would create a field ID of 1000 which is not what we want (it'll collide with the original 1000 field ID of the bcuket transform on ID)

[HonahX]

I think we probably should update

iceberg-python/pyiceberg/partitioning.py

Lines 148 to 152 in 7fbcc22

	@property
	def last_assigned_field_id(self) -> int:
	if self.fields:
	return max(pf.field_id for pf in self.fields)
	return PARTITION_FIELD_ID_START

to return PARTITION_FIELD_ID_START - 1 for unpartitioned spec. Then we can return the last_partition_id from metadata directly because the metadata should have last_partition_id=999 for unpartitioned table.

Java implementation uses PARTITION_FIELD_ID_START - 1 for unpartitioned spec:
https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/PartitionSpec.java#L344-L345
https://github.com/apache/iceberg/blob/9921937d8285dec9a19fd16b0cd82d451a8aca9e/api/src/main/java/org/apache/iceberg/PartitionSpec.java#L319-L321

I checked locally that unpartitioned tables created by spark-iceberg-runtime have last_partition_id=999, while those created by pyiceberg have last_partition_id=1000:

[amogh-jahagirdar]

This needs to be updated so that in case there are partition specs, we return 999. It's insufficient to just update the PartitionSpec#last_assigned_field_id method. I do believe this is spec compliant since the spec doesn't explicitly say what values these IDs should be. This is also what Spark does when one creates an unpartitioned table. The spec does say in v1, ids were assigned starting at 1000, which is still followed. So I think we're covered. @Fokko @HonahX

[Fokko]

Ah, I see. So the next one will be 1000. It feels a bit like a workaround, let me check

[amogh-jahagirdar]

See https://github.com/apache/iceberg-python/pull/245/files#r1479265846

[Fokko]

I think this one is a bit odd since it is V2 table. I had to dig into the code myself a bit as well. I noticed that the last_partition_id is optional in the metadata. What do you think of the following solution: amogh-jahagirdar#1

[HonahX]

I checked V2 unpartitioned table created by spark-iceberg-runtime and the last_partition_id stored in the metadata is 999.

Therefore I suggested to update the last_partition_id() in pyiceberg to align with the java implementation.

In general, I think 999 is spec compliant since it is for UnpartitionedSpec, where there is no existing partition field. It implies that 1000 will be the id for the first valid partition field and thus align with the spec. Do these sound reasonable? Appreciate your thoughts on this!

[amogh-jahagirdar]

@Fokko I took a look and integrated the changes, and after going back/forth I think I'd like to keep the changes as is.
My rationale is that even after those changes there's some more workarounds that need to be done to make sure new IDs start at 1000 (after taking the changes directly, field IDs for non-REST will start at 1001 without any more changes).

Now, technically it does not seem to be a hard requirement that ids need to start at 1000 for v2 tables. Even for v1 starting at 1000 does not seem to be a requirement, rather just how the Java lib was implemented.

In v1, partition field IDs were not tracked, but were assigned sequentially starting at 1000 in the reference implementation.

I read this as "this was how we originally implemented in Java but not really required so long as it's unique and increasing for new fields"

All that said, I'd advocate for just following the practice of starting at 1000 for both v1 and v2 because it's just the established model and avoids any confusion.

On returning 999 for unpartitioned spec:

As @HonahX alluded to I think that makes for a logical API, considering we want to start a field at 1000, the unpartitioned spec (no fields) last assigned field ID should be one less than that. I don't think we want to return 1000 in that case. This is also what Spark sets for the unpartitioned spec, and is spec compliant (since the spec doesn't mandate any particular IDs)

I could see the argument where we just want to return None for a last_assigned_partition_id for this API but that just shifts the responsibility to other locations to make sure Ids are set correctly according to our scheme which ends up being more messy imo compared to just defining the API to return 999 if the only spec is the unpartitioned spec (which is the value which would be set anyways).

I also think it makes sense to set last-assigned-partition-id for both V1 and V2. Even though it's optional for V1 we set the other optional fields for v1 metadata so it seems a bit odd to make this particular case an exception.

[Fokko]

Sounds reasonable to me, and as long as it is in line with the spec, I'm okay with it 👍

[amogh-jahagirdar]

@Fokko I took a look and integrated the changes, and after going back/forth I think I'd like to keep the changes as is.
My rationale is that even after those changes there's some more workarounds that need to be done to make sure new IDs start at 1000 (after taking the changes directly, field IDs for non-REST will start at 1001 without any more changes).

Now, technically it does not seem to be a hard requirement that ids need to start at 1000 for v2 tables. Even for v1 starting at 1000 does not seem to be a requirement, rather just how the Java lib was implemented.

In v1, partition field IDs were not tracked, but were assigned sequentially starting at 1000 in the reference implementation.

I read this as "this was how we originally implemented in Java but not really required so long as it's unique and increasing for new fields"

All that said, I'd advocate for just following the practice of starting at 1000 for both v1 and v2 because it's just the established model and avoids any confusion.

On returning 999 for unpartitioned spec:

As @HonahX alluded to I think that makes for a logical API, considering we want to start a field at 1000, the unpartitioned spec (no fields) last assigned field ID should be one less than that. I don't think we want to return 1000 in that case. This is also what Spark sets for the unpartitioned spec, and is spec compliant (since the spec doesn't mandate any particular IDs)

I could see the argument where we just want to return None for a last_assigned_partition_id for this API but that just shifts the responsibility to other locations to make sure Ids are set correctly according to our scheme which ends up being more messy imo compared to just defining the API to return 999 if the only spec is the unpartitioned spec (which is the value which would be set anyways).

I also think it makes sense to set last-assigned-partition-id for both V1 and V2. Even though it's optional for V1 we set the other optional fields for v1 metadata so it seems a bit odd to make this particular case an exception.

[amogh-jahagirdar]

I've parameterized all the tests for testing both hive/rest.

[Fokko]

Thanks @amogh-jahagirdar for working on this, and sorry for the long wait for the review. Thanks @HonahX for the review 🙌