Update table metadata

[HonahX]

Fixes: #22

This PR:

• Implements update_table_metadata which applies a list of TableUpdates on the current metadata
• Adds unit tests in test_init.py

In the first version, update_table_metadata will support the following types of table updates:

• UpgradeFormatVersionUpdate Java ref
• AddSchemaUpdate Java ref
• SetCurrentSchemaUpdate Java ref
• AddSnapshotUpdate Java ref
• SetSnapshotRefUpdate Java ref

This PR is one of the parts required to support_commit_table of catalogs except rest. A WIP PR containing a complete implementation of glue table commit can be found here: #140

[Fokko]

@HonahX This is high on my list. I'm OOO the rest of the week, I'll review this early next week since this is quite an important PR that needs some focus.

[Fokko]

Looking good @HonahX, left some comments.

[Fokko]

It looks like we don't use this one?

Suggested change

last_added_schema_id: Optional[int]

[HonahX]

There is one case that we need this:
For SetCurrentSchemaUpdate, if the given schema_id is -1, we will set the current schema to last added schema by the stored last_added_schema_id.

I think later when we add support to SetDefaultSpecUpdate and SetDefaultSortOrderUpdate we can add last_added_spec_id and last_added_sort_order_id here too. WDYT?

[Fokko]

I prefer to keep them separate. I think we might need to have some additional checks here such as, what happens if you add a column, and then revoke the column again. It will first create a new schema, with a new ID, and then it will reuse the old schema again.

• 1: Schema(a: int), current_schema_id=1
  Add column b:
• 1: Schema(a: int), 2: Schema(a: int, b: int), current_schema_id=2
  Drop column b:
• 1: Schema(a: int), 2: Schema(a: int, b: int), current_schema_id=1

[HonahX]

Thanks for sharing the example! I'd like to clarify some points to ensure I fully understand your perspective. According to Java ref, It appears that lastAddedSchemaId refers to the most recently added schema in the current transaction.

The purpose of lastAddedSchemaId in Java is to support multiple AddSchema updates within a single transaction, allowing for efficient tracking of the latest schema addition. This is particularly beneficial if a schema added later in the transaction is the same as one added earlier. However, in Python, where we limit to one AddSchema per transaction, lastAddedSchemaId should either be the ID of the added schema or None, specifically when the added schema is identical to existing ones in the metadata.

The example you provided brings up an important consideration for our Python implementation: if the result schema of UpdateSchema is identical to a previously added schema in base_metadata, we should not add this new schema to the metadata. Instead, we only need to set the current schema ID to that of the previously existing one.
In Java, metadata builder will suppress the AddSchema update if the new schema is a duplicate of any previous schemas. Inspired by this, I think we can implement a similar mechanism in UpdateSchema.commit(). If the new schema is identical to an existing one, only a single setCurrentSchema should be added.

In conclusion, to enhance our approach to schema updates, I propose the following:

1. In UpdateSchema.commit(): We should check if new_schema is identical to any existing schemas before adding an AddSchema and a SetCurrentSchema. If they are identical, only a SetCurrentSchema(existing_schema_id) is necessary.
2. Regarding lastAddedSchemaId: It’s crucial to ensure that if it's not None, it points to a schema added earlier in the current transaction. Since Python transactions contain only one AddSchema, this should be straightforward. We could also consider adding a uniqueness check in update_table_metadata to confirm that the updates are from a single transaction. Also, instead of having a lastAddedSchemaId, we may add a method called last_added_schema() to extract the last added schema from the update context.

What do you think – do the above reasoning and proposed changes seem good to you?

Thanks!

[Fokko]

Thanks for the thorough explanation.

In UpdateSchema.commit(): We should check if new_schema is identical to any existing schemas before adding an AddSchema and a SetCurrentSchema. If they are identical, only a SetCurrentSchema(existing_schema_id) is necessary.

Yes, this makes a lot of sense to me.

Regarding lastAddedSchemaId: It’s crucial to ensure that if it's not None, it points to a schema added earlier in the current transaction. Since Python transactions contain only one AddSchema, this should be straightforward. We could also consider adding a uniqueness check in update_table_metadata to confirm that the updates are from a single transaction. Also, instead of having a lastAddedSchemaId, we may add a method called last_added_schema() to extract the last added schema from the update context.

This also makes sense to me. However one thing to note. The lastAddedSchemaId sounds to me like an implementation detail from Java. In PyIceberg the current situation is simpler as you explained, so we could just do max(schema.id from tbl.schemas) + 1.

You sparked my interest here, let me double check this on the REST side as well 👍

[Fokko]

Thanks for raising this, this is currently not handled correctly on the rest side either.

I've created a PR here as a suggestion, but also feel free to fix it in your PR.

[Fokko]

@HonahX can you rebase? I think this one is ready to go!

[HonahX]

@Fokko Thanks for reviewing! I've pushed my updates (mostly on SetCurrentSchemaUpdate) and rebased the PR.

[Fokko]

While this is a very safe way of doing the copy, it is also rather expensive since we convert everything to a Python dict, and then create a new object again. Pydantic has the model_copy argument that seems to do what we're looking for:

Suggested change

	updated_metadata_data=copy(base_metadata.model_dump())
	updated_metadata_data["format-version"] =update.format_version
	updated_metadata_data=base_metadata.model_copy(**{"format-version": update.format_version})

We could construct a dict where we add all the changes (for the more complicated updated below), and then call model_copy for each update.

This will make a shallow copy by default (which I think is okay, since the model is immutable).

[HonahX]

Thanks for the explanation! I am a little worried about the immutability of table metadata. I think Pydantic's frozen config does not prevent updates to list, dict, etc. If we make shallow copy of list fields in metadata and later some code mistakenly alter the list (e.g. append something) in the updated metadata, the effect will be populated to the base_metadata too and it may be hard to detect. Do you think this might be a problem?

[Fokko]

In that case, we can be cautious and set deep=true. I would love to see some tests that validate the behavior. Those should be easy to add.

[HonahX]

Thanks for the suggestion! I've just implemented the suggested change on my end, but I'm still in the process of building the tests for shallow vs deep copy. Given that the current PR already contains lots of change, do you think it might be a good idea to make the model_copy transfer in a separate, follow-up PR?

[Fokko]

Sounds good @HonahX. I've created a new issue here: #179

[Fokko]

@HonahX Almost there, looking good 👍

[Fokko]

Great work @HonahX Thanks for picking this up!

[HonahX]

Fixes: #22

This PR:

• Implements update_table_metadata which applies a list of TableUpdates on the current metadata
• Adds unit tests in test_init.py

In the first version, update_table_metadata will support the following types of table updates:

• UpgradeFormatVersionUpdate Java ref
• AddSchemaUpdate Java ref
• SetCurrentSchemaUpdate Java ref
• AddSnapshotUpdate Java ref
• SetSnapshotRefUpdate Java ref

This PR is one of the parts required to support_commit_table of catalogs except rest. A WIP PR containing a complete implementation of glue table commit can be found here: #140

[Fokko]

@HonahX This is high on my list. I'm OOO the rest of the week, I'll review this early next week since this is quite an important PR that needs some focus.

[Fokko]

Looking good @HonahX, left some comments.

[Fokko]

It looks like we don't use this one?

Suggested change

last_added_schema_id: Optional[int]

[HonahX]

There is one case that we need this:
For SetCurrentSchemaUpdate, if the given schema_id is -1, we will set the current schema to last added schema by the stored last_added_schema_id.

I think later when we add support to SetDefaultSpecUpdate and SetDefaultSortOrderUpdate we can add last_added_spec_id and last_added_sort_order_id here too. WDYT?

[Fokko]

I prefer to keep them separate. I think we might need to have some additional checks here such as, what happens if you add a column, and then revoke the column again. It will first create a new schema, with a new ID, and then it will reuse the old schema again.

• 1: Schema(a: int), current_schema_id=1
  Add column b:
• 1: Schema(a: int), 2: Schema(a: int, b: int), current_schema_id=2
  Drop column b:
• 1: Schema(a: int), 2: Schema(a: int, b: int), current_schema_id=1

[HonahX]

Thanks for sharing the example! I'd like to clarify some points to ensure I fully understand your perspective. According to Java ref, It appears that lastAddedSchemaId refers to the most recently added schema in the current transaction.

The purpose of lastAddedSchemaId in Java is to support multiple AddSchema updates within a single transaction, allowing for efficient tracking of the latest schema addition. This is particularly beneficial if a schema added later in the transaction is the same as one added earlier. However, in Python, where we limit to one AddSchema per transaction, lastAddedSchemaId should either be the ID of the added schema or None, specifically when the added schema is identical to existing ones in the metadata.

The example you provided brings up an important consideration for our Python implementation: if the result schema of UpdateSchema is identical to a previously added schema in base_metadata, we should not add this new schema to the metadata. Instead, we only need to set the current schema ID to that of the previously existing one.
In Java, metadata builder will suppress the AddSchema update if the new schema is a duplicate of any previous schemas. Inspired by this, I think we can implement a similar mechanism in UpdateSchema.commit(). If the new schema is identical to an existing one, only a single setCurrentSchema should be added.

In conclusion, to enhance our approach to schema updates, I propose the following:

1. In UpdateSchema.commit(): We should check if new_schema is identical to any existing schemas before adding an AddSchema and a SetCurrentSchema. If they are identical, only a SetCurrentSchema(existing_schema_id) is necessary.
2. Regarding lastAddedSchemaId: It’s crucial to ensure that if it's not None, it points to a schema added earlier in the current transaction. Since Python transactions contain only one AddSchema, this should be straightforward. We could also consider adding a uniqueness check in update_table_metadata to confirm that the updates are from a single transaction. Also, instead of having a lastAddedSchemaId, we may add a method called last_added_schema() to extract the last added schema from the update context.

What do you think – do the above reasoning and proposed changes seem good to you?

Thanks!

[Fokko]

Thanks for the thorough explanation.

In UpdateSchema.commit(): We should check if new_schema is identical to any existing schemas before adding an AddSchema and a SetCurrentSchema. If they are identical, only a SetCurrentSchema(existing_schema_id) is necessary.

Yes, this makes a lot of sense to me.

Regarding lastAddedSchemaId: It’s crucial to ensure that if it's not None, it points to a schema added earlier in the current transaction. Since Python transactions contain only one AddSchema, this should be straightforward. We could also consider adding a uniqueness check in update_table_metadata to confirm that the updates are from a single transaction. Also, instead of having a lastAddedSchemaId, we may add a method called last_added_schema() to extract the last added schema from the update context.

This also makes sense to me. However one thing to note. The lastAddedSchemaId sounds to me like an implementation detail from Java. In PyIceberg the current situation is simpler as you explained, so we could just do max(schema.id from tbl.schemas) + 1.

You sparked my interest here, let me double check this on the REST side as well 👍

[Fokko]

Thanks for raising this, this is currently not handled correctly on the rest side either.

I've created a PR here as a suggestion, but also feel free to fix it in your PR.

[Fokko]

@HonahX can you rebase? I think this one is ready to go!

[HonahX]

@Fokko Thanks for reviewing! I've pushed my updates (mostly on SetCurrentSchemaUpdate) and rebased the PR.

[Fokko]

While this is a very safe way of doing the copy, it is also rather expensive since we convert everything to a Python dict, and then create a new object again. Pydantic has the model_copy argument that seems to do what we're looking for:

Suggested change

	updated_metadata_data=copy(base_metadata.model_dump())
	updated_metadata_data["format-version"] =update.format_version
	updated_metadata_data=base_metadata.model_copy(**{"format-version": update.format_version})

We could construct a dict where we add all the changes (for the more complicated updated below), and then call model_copy for each update.

This will make a shallow copy by default (which I think is okay, since the model is immutable).

[HonahX]

Thanks for the explanation! I am a little worried about the immutability of table metadata. I think Pydantic's frozen config does not prevent updates to list, dict, etc. If we make shallow copy of list fields in metadata and later some code mistakenly alter the list (e.g. append something) in the updated metadata, the effect will be populated to the base_metadata too and it may be hard to detect. Do you think this might be a problem?

[Fokko]

In that case, we can be cautious and set deep=true. I would love to see some tests that validate the behavior. Those should be easy to add.

[HonahX]

Thanks for the suggestion! I've just implemented the suggested change on my end, but I'm still in the process of building the tests for shallow vs deep copy. Given that the current PR already contains lots of change, do you think it might be a good idea to make the model_copy transfer in a separate, follow-up PR?

[Fokko]

Sounds good @HonahX. I've created a new issue here: #179

[Fokko]

@HonahX Almost there, looking good 👍

[Fokko]

Great work @HonahX Thanks for picking this up!

[HonahX]

Fixes: #22

This PR:

• Implements update_table_metadata which applies a list of TableUpdates on the current metadata
• Adds unit tests in test_init.py

In the first version, update_table_metadata will support the following types of table updates:

• UpgradeFormatVersionUpdate Java ref
• AddSchemaUpdate Java ref
• SetCurrentSchemaUpdate Java ref
• AddSnapshotUpdate Java ref
• SetSnapshotRefUpdate Java ref

This PR is one of the parts required to support_commit_table of catalogs except rest. A WIP PR containing a complete implementation of glue table commit can be found here: #140

[Fokko]

@HonahX This is high on my list. I'm OOO the rest of the week, I'll review this early next week since this is quite an important PR that needs some focus.

[Fokko]

Looking good @HonahX, left some comments.

[Fokko]

It looks like we don't use this one?

Suggested change

last_added_schema_id: Optional[int]

[HonahX]

There is one case that we need this:
For SetCurrentSchemaUpdate, if the given schema_id is -1, we will set the current schema to last added schema by the stored last_added_schema_id.

I think later when we add support to SetDefaultSpecUpdate and SetDefaultSortOrderUpdate we can add last_added_spec_id and last_added_sort_order_id here too. WDYT?

[Fokko]

I prefer to keep them separate. I think we might need to have some additional checks here such as, what happens if you add a column, and then revoke the column again. It will first create a new schema, with a new ID, and then it will reuse the old schema again.

• 1: Schema(a: int), current_schema_id=1
  Add column b:
• 1: Schema(a: int), 2: Schema(a: int, b: int), current_schema_id=2
  Drop column b:
• 1: Schema(a: int), 2: Schema(a: int, b: int), current_schema_id=1

[HonahX]

Thanks for sharing the example! I'd like to clarify some points to ensure I fully understand your perspective. According to Java ref, It appears that lastAddedSchemaId refers to the most recently added schema in the current transaction.

The purpose of lastAddedSchemaId in Java is to support multiple AddSchema updates within a single transaction, allowing for efficient tracking of the latest schema addition. This is particularly beneficial if a schema added later in the transaction is the same as one added earlier. However, in Python, where we limit to one AddSchema per transaction, lastAddedSchemaId should either be the ID of the added schema or None, specifically when the added schema is identical to existing ones in the metadata.

The example you provided brings up an important consideration for our Python implementation: if the result schema of UpdateSchema is identical to a previously added schema in base_metadata, we should not add this new schema to the metadata. Instead, we only need to set the current schema ID to that of the previously existing one.
In Java, metadata builder will suppress the AddSchema update if the new schema is a duplicate of any previous schemas. Inspired by this, I think we can implement a similar mechanism in UpdateSchema.commit(). If the new schema is identical to an existing one, only a single setCurrentSchema should be added.

In conclusion, to enhance our approach to schema updates, I propose the following:

1. In UpdateSchema.commit(): We should check if new_schema is identical to any existing schemas before adding an AddSchema and a SetCurrentSchema. If they are identical, only a SetCurrentSchema(existing_schema_id) is necessary.
2. Regarding lastAddedSchemaId: It’s crucial to ensure that if it's not None, it points to a schema added earlier in the current transaction. Since Python transactions contain only one AddSchema, this should be straightforward. We could also consider adding a uniqueness check in update_table_metadata to confirm that the updates are from a single transaction. Also, instead of having a lastAddedSchemaId, we may add a method called last_added_schema() to extract the last added schema from the update context.

What do you think – do the above reasoning and proposed changes seem good to you?

Thanks!

[Fokko]

Thanks for the thorough explanation.

In UpdateSchema.commit(): We should check if new_schema is identical to any existing schemas before adding an AddSchema and a SetCurrentSchema. If they are identical, only a SetCurrentSchema(existing_schema_id) is necessary.

Yes, this makes a lot of sense to me.

Regarding lastAddedSchemaId: It’s crucial to ensure that if it's not None, it points to a schema added earlier in the current transaction. Since Python transactions contain only one AddSchema, this should be straightforward. We could also consider adding a uniqueness check in update_table_metadata to confirm that the updates are from a single transaction. Also, instead of having a lastAddedSchemaId, we may add a method called last_added_schema() to extract the last added schema from the update context.

This also makes sense to me. However one thing to note. The lastAddedSchemaId sounds to me like an implementation detail from Java. In PyIceberg the current situation is simpler as you explained, so we could just do max(schema.id from tbl.schemas) + 1.

You sparked my interest here, let me double check this on the REST side as well 👍

[Fokko]

Thanks for raising this, this is currently not handled correctly on the rest side either.

I've created a PR here as a suggestion, but also feel free to fix it in your PR.

[Fokko]

@HonahX can you rebase? I think this one is ready to go!

[HonahX]

@Fokko Thanks for reviewing! I've pushed my updates (mostly on SetCurrentSchemaUpdate) and rebased the PR.

[Fokko]

While this is a very safe way of doing the copy, it is also rather expensive since we convert everything to a Python dict, and then create a new object again. Pydantic has the model_copy argument that seems to do what we're looking for:

Suggested change

	updated_metadata_data=copy(base_metadata.model_dump())
	updated_metadata_data["format-version"] =update.format_version
	updated_metadata_data=base_metadata.model_copy(**{"format-version": update.format_version})

We could construct a dict where we add all the changes (for the more complicated updated below), and then call model_copy for each update.

This will make a shallow copy by default (which I think is okay, since the model is immutable).

[HonahX]

Thanks for the explanation! I am a little worried about the immutability of table metadata. I think Pydantic's frozen config does not prevent updates to list, dict, etc. If we make shallow copy of list fields in metadata and later some code mistakenly alter the list (e.g. append something) in the updated metadata, the effect will be populated to the base_metadata too and it may be hard to detect. Do you think this might be a problem?

[Fokko]

In that case, we can be cautious and set deep=true. I would love to see some tests that validate the behavior. Those should be easy to add.

[HonahX]

Thanks for the suggestion! I've just implemented the suggested change on my end, but I'm still in the process of building the tests for shallow vs deep copy. Given that the current PR already contains lots of change, do you think it might be a good idea to make the model_copy transfer in a separate, follow-up PR?

[Fokko]

Sounds good @HonahX. I've created a new issue here: #179

[Fokko]

@HonahX Almost there, looking good 👍

[Fokko]

Great work @HonahX Thanks for picking this up!

[HonahX]

Fixes: #22

This PR:

• Implements update_table_metadata which applies a list of TableUpdates on the current metadata
• Adds unit tests in test_init.py

In the first version, update_table_metadata will support the following types of table updates:

• UpgradeFormatVersionUpdate Java ref
• AddSchemaUpdate Java ref
• SetCurrentSchemaUpdate Java ref
• AddSnapshotUpdate Java ref
• SetSnapshotRefUpdate Java ref

This PR is one of the parts required to support_commit_table of catalogs except rest. A WIP PR containing a complete implementation of glue table commit can be found here: #140

[Fokko]

@HonahX This is high on my list. I'm OOO the rest of the week, I'll review this early next week since this is quite an important PR that needs some focus.

[Fokko]

Looking good @HonahX, left some comments.

[Fokko]

It looks like we don't use this one?

Suggested change

last_added_schema_id: Optional[int]

[HonahX]

There is one case that we need this:
For SetCurrentSchemaUpdate, if the given schema_id is -1, we will set the current schema to last added schema by the stored last_added_schema_id.

I think later when we add support to SetDefaultSpecUpdate and SetDefaultSortOrderUpdate we can add last_added_spec_id and last_added_sort_order_id here too. WDYT?

[Fokko]

I prefer to keep them separate. I think we might need to have some additional checks here such as, what happens if you add a column, and then revoke the column again. It will first create a new schema, with a new ID, and then it will reuse the old schema again.

• 1: Schema(a: int), current_schema_id=1
  Add column b:
• 1: Schema(a: int), 2: Schema(a: int, b: int), current_schema_id=2
  Drop column b:
• 1: Schema(a: int), 2: Schema(a: int, b: int), current_schema_id=1

[HonahX]

Thanks for sharing the example! I'd like to clarify some points to ensure I fully understand your perspective. According to Java ref, It appears that lastAddedSchemaId refers to the most recently added schema in the current transaction.

The purpose of lastAddedSchemaId in Java is to support multiple AddSchema updates within a single transaction, allowing for efficient tracking of the latest schema addition. This is particularly beneficial if a schema added later in the transaction is the same as one added earlier. However, in Python, where we limit to one AddSchema per transaction, lastAddedSchemaId should either be the ID of the added schema or None, specifically when the added schema is identical to existing ones in the metadata.

The example you provided brings up an important consideration for our Python implementation: if the result schema of UpdateSchema is identical to a previously added schema in base_metadata, we should not add this new schema to the metadata. Instead, we only need to set the current schema ID to that of the previously existing one.
In Java, metadata builder will suppress the AddSchema update if the new schema is a duplicate of any previous schemas. Inspired by this, I think we can implement a similar mechanism in UpdateSchema.commit(). If the new schema is identical to an existing one, only a single setCurrentSchema should be added.

In conclusion, to enhance our approach to schema updates, I propose the following:

1. In UpdateSchema.commit(): We should check if new_schema is identical to any existing schemas before adding an AddSchema and a SetCurrentSchema. If they are identical, only a SetCurrentSchema(existing_schema_id) is necessary.
2. Regarding lastAddedSchemaId: It’s crucial to ensure that if it's not None, it points to a schema added earlier in the current transaction. Since Python transactions contain only one AddSchema, this should be straightforward. We could also consider adding a uniqueness check in update_table_metadata to confirm that the updates are from a single transaction. Also, instead of having a lastAddedSchemaId, we may add a method called last_added_schema() to extract the last added schema from the update context.

What do you think – do the above reasoning and proposed changes seem good to you?

Thanks!

[Fokko]

Thanks for the thorough explanation.

In UpdateSchema.commit(): We should check if new_schema is identical to any existing schemas before adding an AddSchema and a SetCurrentSchema. If they are identical, only a SetCurrentSchema(existing_schema_id) is necessary.

Yes, this makes a lot of sense to me.

Regarding lastAddedSchemaId: It’s crucial to ensure that if it's not None, it points to a schema added earlier in the current transaction. Since Python transactions contain only one AddSchema, this should be straightforward. We could also consider adding a uniqueness check in update_table_metadata to confirm that the updates are from a single transaction. Also, instead of having a lastAddedSchemaId, we may add a method called last_added_schema() to extract the last added schema from the update context.

This also makes sense to me. However one thing to note. The lastAddedSchemaId sounds to me like an implementation detail from Java. In PyIceberg the current situation is simpler as you explained, so we could just do max(schema.id from tbl.schemas) + 1.

You sparked my interest here, let me double check this on the REST side as well 👍

[Fokko]

Thanks for raising this, this is currently not handled correctly on the rest side either.

I've created a PR here as a suggestion, but also feel free to fix it in your PR.

[Fokko]

@HonahX can you rebase? I think this one is ready to go!

[HonahX]

@Fokko Thanks for reviewing! I've pushed my updates (mostly on SetCurrentSchemaUpdate) and rebased the PR.

[Fokko]

While this is a very safe way of doing the copy, it is also rather expensive since we convert everything to a Python dict, and then create a new object again. Pydantic has the model_copy argument that seems to do what we're looking for:

Suggested change

	updated_metadata_data=copy(base_metadata.model_dump())
	updated_metadata_data["format-version"] =update.format_version
	updated_metadata_data=base_metadata.model_copy(**{"format-version": update.format_version})

We could construct a dict where we add all the changes (for the more complicated updated below), and then call model_copy for each update.

This will make a shallow copy by default (which I think is okay, since the model is immutable).

[HonahX]

Thanks for the explanation! I am a little worried about the immutability of table metadata. I think Pydantic's frozen config does not prevent updates to list, dict, etc. If we make shallow copy of list fields in metadata and later some code mistakenly alter the list (e.g. append something) in the updated metadata, the effect will be populated to the base_metadata too and it may be hard to detect. Do you think this might be a problem?

[Fokko]

In that case, we can be cautious and set deep=true. I would love to see some tests that validate the behavior. Those should be easy to add.

[HonahX]

Thanks for the suggestion! I've just implemented the suggested change on my end, but I'm still in the process of building the tests for shallow vs deep copy. Given that the current PR already contains lots of change, do you think it might be a good idea to make the model_copy transfer in a separate, follow-up PR?

[Fokko]

Sounds good @HonahX. I've created a new issue here: #179

[Fokko]

@HonahX Almost there, looking good 👍

[Fokko]

Great work @HonahX Thanks for picking this up!

[HonahX]

Fixes: #22

This PR:

• Implements update_table_metadata which applies a list of TableUpdates on the current metadata
• Adds unit tests in test_init.py

In the first version, update_table_metadata will support the following types of table updates:

• UpgradeFormatVersionUpdate Java ref
• AddSchemaUpdate Java ref
• SetCurrentSchemaUpdate Java ref
• AddSnapshotUpdate Java ref
• SetSnapshotRefUpdate Java ref

This PR is one of the parts required to support_commit_table of catalogs except rest. A WIP PR containing a complete implementation of glue table commit can be found here: #140

[Fokko]

@HonahX This is high on my list. I'm OOO the rest of the week, I'll review this early next week since this is quite an important PR that needs some focus.

[Fokko]

Looking good @HonahX, left some comments.

[Fokko]

It looks like we don't use this one?

Suggested change

last_added_schema_id: Optional[int]

[HonahX]

There is one case that we need this:
For SetCurrentSchemaUpdate, if the given schema_id is -1, we will set the current schema to last added schema by the stored last_added_schema_id.

I think later when we add support to SetDefaultSpecUpdate and SetDefaultSortOrderUpdate we can add last_added_spec_id and last_added_sort_order_id here too. WDYT?

[Fokko]

I prefer to keep them separate. I think we might need to have some additional checks here such as, what happens if you add a column, and then revoke the column again. It will first create a new schema, with a new ID, and then it will reuse the old schema again.

• 1: Schema(a: int), current_schema_id=1
  Add column b:
• 1: Schema(a: int), 2: Schema(a: int, b: int), current_schema_id=2
  Drop column b:
• 1: Schema(a: int), 2: Schema(a: int, b: int), current_schema_id=1

[HonahX]

Thanks for sharing the example! I'd like to clarify some points to ensure I fully understand your perspective. According to Java ref, It appears that lastAddedSchemaId refers to the most recently added schema in the current transaction.

The purpose of lastAddedSchemaId in Java is to support multiple AddSchema updates within a single transaction, allowing for efficient tracking of the latest schema addition. This is particularly beneficial if a schema added later in the transaction is the same as one added earlier. However, in Python, where we limit to one AddSchema per transaction, lastAddedSchemaId should either be the ID of the added schema or None, specifically when the added schema is identical to existing ones in the metadata.

The example you provided brings up an important consideration for our Python implementation: if the result schema of UpdateSchema is identical to a previously added schema in base_metadata, we should not add this new schema to the metadata. Instead, we only need to set the current schema ID to that of the previously existing one.
In Java, metadata builder will suppress the AddSchema update if the new schema is a duplicate of any previous schemas. Inspired by this, I think we can implement a similar mechanism in UpdateSchema.commit(). If the new schema is identical to an existing one, only a single setCurrentSchema should be added.

In conclusion, to enhance our approach to schema updates, I propose the following:

1. In UpdateSchema.commit(): We should check if new_schema is identical to any existing schemas before adding an AddSchema and a SetCurrentSchema. If they are identical, only a SetCurrentSchema(existing_schema_id) is necessary.
2. Regarding lastAddedSchemaId: It’s crucial to ensure that if it's not None, it points to a schema added earlier in the current transaction. Since Python transactions contain only one AddSchema, this should be straightforward. We could also consider adding a uniqueness check in update_table_metadata to confirm that the updates are from a single transaction. Also, instead of having a lastAddedSchemaId, we may add a method called last_added_schema() to extract the last added schema from the update context.

What do you think – do the above reasoning and proposed changes seem good to you?

Thanks!

[Fokko]

Thanks for the thorough explanation.

In UpdateSchema.commit(): We should check if new_schema is identical to any existing schemas before adding an AddSchema and a SetCurrentSchema. If they are identical, only a SetCurrentSchema(existing_schema_id) is necessary.

Yes, this makes a lot of sense to me.

Regarding lastAddedSchemaId: It’s crucial to ensure that if it's not None, it points to a schema added earlier in the current transaction. Since Python transactions contain only one AddSchema, this should be straightforward. We could also consider adding a uniqueness check in update_table_metadata to confirm that the updates are from a single transaction. Also, instead of having a lastAddedSchemaId, we may add a method called last_added_schema() to extract the last added schema from the update context.

This also makes sense to me. However one thing to note. The lastAddedSchemaId sounds to me like an implementation detail from Java. In PyIceberg the current situation is simpler as you explained, so we could just do max(schema.id from tbl.schemas) + 1.

You sparked my interest here, let me double check this on the REST side as well 👍

[Fokko]

Thanks for raising this, this is currently not handled correctly on the rest side either.

I've created a PR here as a suggestion, but also feel free to fix it in your PR.

[Fokko]

@HonahX can you rebase? I think this one is ready to go!

[HonahX]

@Fokko Thanks for reviewing! I've pushed my updates (mostly on SetCurrentSchemaUpdate) and rebased the PR.

[Fokko]

While this is a very safe way of doing the copy, it is also rather expensive since we convert everything to a Python dict, and then create a new object again. Pydantic has the model_copy argument that seems to do what we're looking for:

Suggested change

	updated_metadata_data=copy(base_metadata.model_dump())
	updated_metadata_data["format-version"] =update.format_version
	updated_metadata_data=base_metadata.model_copy(**{"format-version": update.format_version})

We could construct a dict where we add all the changes (for the more complicated updated below), and then call model_copy for each update.

This will make a shallow copy by default (which I think is okay, since the model is immutable).

[HonahX]

Thanks for the explanation! I am a little worried about the immutability of table metadata. I think Pydantic's frozen config does not prevent updates to list, dict, etc. If we make shallow copy of list fields in metadata and later some code mistakenly alter the list (e.g. append something) in the updated metadata, the effect will be populated to the base_metadata too and it may be hard to detect. Do you think this might be a problem?

[Fokko]

In that case, we can be cautious and set deep=true. I would love to see some tests that validate the behavior. Those should be easy to add.

[HonahX]

Thanks for the suggestion! I've just implemented the suggested change on my end, but I'm still in the process of building the tests for shallow vs deep copy. Given that the current PR already contains lots of change, do you think it might be a good idea to make the model_copy transfer in a separate, follow-up PR?

[Fokko]

Sounds good @HonahX. I've created a new issue here: #179

[Fokko]

@HonahX Almost there, looking good 👍

[Fokko]

Great work @HonahX Thanks for picking this up!

[HonahX]

Fixes: #22

This PR:

• Implements update_table_metadata which applies a list of TableUpdates on the current metadata
• Adds unit tests in test_init.py

In the first version, update_table_metadata will support the following types of table updates:

• UpgradeFormatVersionUpdate Java ref
• AddSchemaUpdate Java ref
• SetCurrentSchemaUpdate Java ref
• AddSnapshotUpdate Java ref
• SetSnapshotRefUpdate Java ref

This PR is one of the parts required to support_commit_table of catalogs except rest. A WIP PR containing a complete implementation of glue table commit can be found here: #140

[Fokko]

@HonahX This is high on my list. I'm OOO the rest of the week, I'll review this early next week since this is quite an important PR that needs some focus.

[Fokko]

Looking good @HonahX, left some comments.

[Fokko]

It looks like we don't use this one?

Suggested change

last_added_schema_id: Optional[int]

[HonahX]

There is one case that we need this:
For SetCurrentSchemaUpdate, if the given schema_id is -1, we will set the current schema to last added schema by the stored last_added_schema_id.

I think later when we add support to SetDefaultSpecUpdate and SetDefaultSortOrderUpdate we can add last_added_spec_id and last_added_sort_order_id here too. WDYT?

[Fokko]

I prefer to keep them separate. I think we might need to have some additional checks here such as, what happens if you add a column, and then revoke the column again. It will first create a new schema, with a new ID, and then it will reuse the old schema again.

• 1: Schema(a: int), current_schema_id=1
  Add column b:
• 1: Schema(a: int), 2: Schema(a: int, b: int), current_schema_id=2
  Drop column b:
• 1: Schema(a: int), 2: Schema(a: int, b: int), current_schema_id=1

[HonahX]

Thanks for sharing the example! I'd like to clarify some points to ensure I fully understand your perspective. According to Java ref, It appears that lastAddedSchemaId refers to the most recently added schema in the current transaction.

The purpose of lastAddedSchemaId in Java is to support multiple AddSchema updates within a single transaction, allowing for efficient tracking of the latest schema addition. This is particularly beneficial if a schema added later in the transaction is the same as one added earlier. However, in Python, where we limit to one AddSchema per transaction, lastAddedSchemaId should either be the ID of the added schema or None, specifically when the added schema is identical to existing ones in the metadata.

The example you provided brings up an important consideration for our Python implementation: if the result schema of UpdateSchema is identical to a previously added schema in base_metadata, we should not add this new schema to the metadata. Instead, we only need to set the current schema ID to that of the previously existing one.
In Java, metadata builder will suppress the AddSchema update if the new schema is a duplicate of any previous schemas. Inspired by this, I think we can implement a similar mechanism in UpdateSchema.commit(). If the new schema is identical to an existing one, only a single setCurrentSchema should be added.

In conclusion, to enhance our approach to schema updates, I propose the following:

1. In UpdateSchema.commit(): We should check if new_schema is identical to any existing schemas before adding an AddSchema and a SetCurrentSchema. If they are identical, only a SetCurrentSchema(existing_schema_id) is necessary.
2. Regarding lastAddedSchemaId: It’s crucial to ensure that if it's not None, it points to a schema added earlier in the current transaction. Since Python transactions contain only one AddSchema, this should be straightforward. We could also consider adding a uniqueness check in update_table_metadata to confirm that the updates are from a single transaction. Also, instead of having a lastAddedSchemaId, we may add a method called last_added_schema() to extract the last added schema from the update context.

What do you think – do the above reasoning and proposed changes seem good to you?

Thanks!

[Fokko]

Thanks for the thorough explanation.

In UpdateSchema.commit(): We should check if new_schema is identical to any existing schemas before adding an AddSchema and a SetCurrentSchema. If they are identical, only a SetCurrentSchema(existing_schema_id) is necessary.

Yes, this makes a lot of sense to me.

Regarding lastAddedSchemaId: It’s crucial to ensure that if it's not None, it points to a schema added earlier in the current transaction. Since Python transactions contain only one AddSchema, this should be straightforward. We could also consider adding a uniqueness check in update_table_metadata to confirm that the updates are from a single transaction. Also, instead of having a lastAddedSchemaId, we may add a method called last_added_schema() to extract the last added schema from the update context.

This also makes sense to me. However one thing to note. The lastAddedSchemaId sounds to me like an implementation detail from Java. In PyIceberg the current situation is simpler as you explained, so we could just do max(schema.id from tbl.schemas) + 1.

You sparked my interest here, let me double check this on the REST side as well 👍

[Fokko]

Thanks for raising this, this is currently not handled correctly on the rest side either.

I've created a PR here as a suggestion, but also feel free to fix it in your PR.

[Fokko]

@HonahX can you rebase? I think this one is ready to go!

[HonahX]

@Fokko Thanks for reviewing! I've pushed my updates (mostly on SetCurrentSchemaUpdate) and rebased the PR.

[Fokko]

While this is a very safe way of doing the copy, it is also rather expensive since we convert everything to a Python dict, and then create a new object again. Pydantic has the model_copy argument that seems to do what we're looking for:

Suggested change

	updated_metadata_data=copy(base_metadata.model_dump())
	updated_metadata_data["format-version"] =update.format_version
	updated_metadata_data=base_metadata.model_copy(**{"format-version": update.format_version})

We could construct a dict where we add all the changes (for the more complicated updated below), and then call model_copy for each update.

This will make a shallow copy by default (which I think is okay, since the model is immutable).

[HonahX]

Thanks for the explanation! I am a little worried about the immutability of table metadata. I think Pydantic's frozen config does not prevent updates to list, dict, etc. If we make shallow copy of list fields in metadata and later some code mistakenly alter the list (e.g. append something) in the updated metadata, the effect will be populated to the base_metadata too and it may be hard to detect. Do you think this might be a problem?

[Fokko]

In that case, we can be cautious and set deep=true. I would love to see some tests that validate the behavior. Those should be easy to add.

[HonahX]

Thanks for the suggestion! I've just implemented the suggested change on my end, but I'm still in the process of building the tests for shallow vs deep copy. Given that the current PR already contains lots of change, do you think it might be a good idea to make the model_copy transfer in a separate, follow-up PR?

[Fokko]

Sounds good @HonahX. I've created a new issue here: #179

[Fokko]

@HonahX Almost there, looking good 👍

[Fokko]

Great work @HonahX Thanks for picking this up!

[HonahX]

Fixes: #22

This PR:

• Implements update_table_metadata which applies a list of TableUpdates on the current metadata
• Adds unit tests in test_init.py

In the first version, update_table_metadata will support the following types of table updates:

• UpgradeFormatVersionUpdate Java ref
• AddSchemaUpdate Java ref
• SetCurrentSchemaUpdate Java ref
• AddSnapshotUpdate Java ref
• SetSnapshotRefUpdate Java ref

This PR is one of the parts required to support_commit_table of catalogs except rest. A WIP PR containing a complete implementation of glue table commit can be found here: #140

[Fokko]

@HonahX This is high on my list. I'm OOO the rest of the week, I'll review this early next week since this is quite an important PR that needs some focus.

[Fokko]

Looking good @HonahX, left some comments.

[Fokko]

It looks like we don't use this one?

Suggested change

last_added_schema_id: Optional[int]

[HonahX]

There is one case that we need this:
For SetCurrentSchemaUpdate, if the given schema_id is -1, we will set the current schema to last added schema by the stored last_added_schema_id.

I think later when we add support to SetDefaultSpecUpdate and SetDefaultSortOrderUpdate we can add last_added_spec_id and last_added_sort_order_id here too. WDYT?

[Fokko]

I prefer to keep them separate. I think we might need to have some additional checks here such as, what happens if you add a column, and then revoke the column again. It will first create a new schema, with a new ID, and then it will reuse the old schema again.

• 1: Schema(a: int), current_schema_id=1
  Add column b:
• 1: Schema(a: int), 2: Schema(a: int, b: int), current_schema_id=2
  Drop column b:
• 1: Schema(a: int), 2: Schema(a: int, b: int), current_schema_id=1

[HonahX]

Thanks for sharing the example! I'd like to clarify some points to ensure I fully understand your perspective. According to Java ref, It appears that lastAddedSchemaId refers to the most recently added schema in the current transaction.

The purpose of lastAddedSchemaId in Java is to support multiple AddSchema updates within a single transaction, allowing for efficient tracking of the latest schema addition. This is particularly beneficial if a schema added later in the transaction is the same as one added earlier. However, in Python, where we limit to one AddSchema per transaction, lastAddedSchemaId should either be the ID of the added schema or None, specifically when the added schema is identical to existing ones in the metadata.

The example you provided brings up an important consideration for our Python implementation: if the result schema of UpdateSchema is identical to a previously added schema in base_metadata, we should not add this new schema to the metadata. Instead, we only need to set the current schema ID to that of the previously existing one.
In Java, metadata builder will suppress the AddSchema update if the new schema is a duplicate of any previous schemas. Inspired by this, I think we can implement a similar mechanism in UpdateSchema.commit(). If the new schema is identical to an existing one, only a single setCurrentSchema should be added.

In conclusion, to enhance our approach to schema updates, I propose the following:

1. In UpdateSchema.commit(): We should check if new_schema is identical to any existing schemas before adding an AddSchema and a SetCurrentSchema. If they are identical, only a SetCurrentSchema(existing_schema_id) is necessary.
2. Regarding lastAddedSchemaId: It’s crucial to ensure that if it's not None, it points to a schema added earlier in the current transaction. Since Python transactions contain only one AddSchema, this should be straightforward. We could also consider adding a uniqueness check in update_table_metadata to confirm that the updates are from a single transaction. Also, instead of having a lastAddedSchemaId, we may add a method called last_added_schema() to extract the last added schema from the update context.

What do you think – do the above reasoning and proposed changes seem good to you?

Thanks!

[Fokko]

Thanks for the thorough explanation.

In UpdateSchema.commit(): We should check if new_schema is identical to any existing schemas before adding an AddSchema and a SetCurrentSchema. If they are identical, only a SetCurrentSchema(existing_schema_id) is necessary.

Yes, this makes a lot of sense to me.

Regarding lastAddedSchemaId: It’s crucial to ensure that if it's not None, it points to a schema added earlier in the current transaction. Since Python transactions contain only one AddSchema, this should be straightforward. We could also consider adding a uniqueness check in update_table_metadata to confirm that the updates are from a single transaction. Also, instead of having a lastAddedSchemaId, we may add a method called last_added_schema() to extract the last added schema from the update context.

This also makes sense to me. However one thing to note. The lastAddedSchemaId sounds to me like an implementation detail from Java. In PyIceberg the current situation is simpler as you explained, so we could just do max(schema.id from tbl.schemas) + 1.

You sparked my interest here, let me double check this on the REST side as well 👍

[Fokko]

Thanks for raising this, this is currently not handled correctly on the rest side either.

I've created a PR here as a suggestion, but also feel free to fix it in your PR.

[Fokko]

@HonahX can you rebase? I think this one is ready to go!

[HonahX]

@Fokko Thanks for reviewing! I've pushed my updates (mostly on SetCurrentSchemaUpdate) and rebased the PR.

[Fokko]

While this is a very safe way of doing the copy, it is also rather expensive since we convert everything to a Python dict, and then create a new object again. Pydantic has the model_copy argument that seems to do what we're looking for:

Suggested change

	updated_metadata_data=copy(base_metadata.model_dump())
	updated_metadata_data["format-version"] =update.format_version
	updated_metadata_data=base_metadata.model_copy(**{"format-version": update.format_version})

We could construct a dict where we add all the changes (for the more complicated updated below), and then call model_copy for each update.

This will make a shallow copy by default (which I think is okay, since the model is immutable).

[HonahX]

Thanks for the explanation! I am a little worried about the immutability of table metadata. I think Pydantic's frozen config does not prevent updates to list, dict, etc. If we make shallow copy of list fields in metadata and later some code mistakenly alter the list (e.g. append something) in the updated metadata, the effect will be populated to the base_metadata too and it may be hard to detect. Do you think this might be a problem?

[Fokko]

In that case, we can be cautious and set deep=true. I would love to see some tests that validate the behavior. Those should be easy to add.

[HonahX]

Thanks for the suggestion! I've just implemented the suggested change on my end, but I'm still in the process of building the tests for shallow vs deep copy. Given that the current PR already contains lots of change, do you think it might be a good idea to make the model_copy transfer in a separate, follow-up PR?

[Fokko]

Sounds good @HonahX. I've created a new issue here: #179

[Fokko]

@HonahX Almost there, looking good 👍

[Fokko]

Great work @HonahX Thanks for picking this up!

[HonahX]

Fixes: #22

This PR:

• Implements update_table_metadata which applies a list of TableUpdates on the current metadata
• Adds unit tests in test_init.py

In the first version, update_table_metadata will support the following types of table updates:

• UpgradeFormatVersionUpdate Java ref
• AddSchemaUpdate Java ref
• SetCurrentSchemaUpdate Java ref
• AddSnapshotUpdate Java ref
• SetSnapshotRefUpdate Java ref

This PR is one of the parts required to support_commit_table of catalogs except rest. A WIP PR containing a complete implementation of glue table commit can be found here: #140

[Fokko]

@HonahX This is high on my list. I'm OOO the rest of the week, I'll review this early next week since this is quite an important PR that needs some focus.

[Fokko]

Looking good @HonahX, left some comments.

[Fokko]

It looks like we don't use this one?

Suggested change

last_added_schema_id: Optional[int]

[HonahX]

There is one case that we need this:
For SetCurrentSchemaUpdate, if the given schema_id is -1, we will set the current schema to last added schema by the stored last_added_schema_id.

I think later when we add support to SetDefaultSpecUpdate and SetDefaultSortOrderUpdate we can add last_added_spec_id and last_added_sort_order_id here too. WDYT?

[Fokko]

I prefer to keep them separate. I think we might need to have some additional checks here such as, what happens if you add a column, and then revoke the column again. It will first create a new schema, with a new ID, and then it will reuse the old schema again.

• 1: Schema(a: int), current_schema_id=1
  Add column b:
• 1: Schema(a: int), 2: Schema(a: int, b: int), current_schema_id=2
  Drop column b:
• 1: Schema(a: int), 2: Schema(a: int, b: int), current_schema_id=1

[HonahX]

Thanks for sharing the example! I'd like to clarify some points to ensure I fully understand your perspective. According to Java ref, It appears that lastAddedSchemaId refers to the most recently added schema in the current transaction.

The purpose of lastAddedSchemaId in Java is to support multiple AddSchema updates within a single transaction, allowing for efficient tracking of the latest schema addition. This is particularly beneficial if a schema added later in the transaction is the same as one added earlier. However, in Python, where we limit to one AddSchema per transaction, lastAddedSchemaId should either be the ID of the added schema or None, specifically when the added schema is identical to existing ones in the metadata.

The example you provided brings up an important consideration for our Python implementation: if the result schema of UpdateSchema is identical to a previously added schema in base_metadata, we should not add this new schema to the metadata. Instead, we only need to set the current schema ID to that of the previously existing one.
In Java, metadata builder will suppress the AddSchema update if the new schema is a duplicate of any previous schemas. Inspired by this, I think we can implement a similar mechanism in UpdateSchema.commit(). If the new schema is identical to an existing one, only a single setCurrentSchema should be added.

In conclusion, to enhance our approach to schema updates, I propose the following:

1. In UpdateSchema.commit(): We should check if new_schema is identical to any existing schemas before adding an AddSchema and a SetCurrentSchema. If they are identical, only a SetCurrentSchema(existing_schema_id) is necessary.
2. Regarding lastAddedSchemaId: It’s crucial to ensure that if it's not None, it points to a schema added earlier in the current transaction. Since Python transactions contain only one AddSchema, this should be straightforward. We could also consider adding a uniqueness check in update_table_metadata to confirm that the updates are from a single transaction. Also, instead of having a lastAddedSchemaId, we may add a method called last_added_schema() to extract the last added schema from the update context.

What do you think – do the above reasoning and proposed changes seem good to you?

Thanks!

[Fokko]

Thanks for the thorough explanation.

In UpdateSchema.commit(): We should check if new_schema is identical to any existing schemas before adding an AddSchema and a SetCurrentSchema. If they are identical, only a SetCurrentSchema(existing_schema_id) is necessary.

Yes, this makes a lot of sense to me.

Regarding lastAddedSchemaId: It’s crucial to ensure that if it's not None, it points to a schema added earlier in the current transaction. Since Python transactions contain only one AddSchema, this should be straightforward. We could also consider adding a uniqueness check in update_table_metadata to confirm that the updates are from a single transaction. Also, instead of having a lastAddedSchemaId, we may add a method called last_added_schema() to extract the last added schema from the update context.

This also makes sense to me. However one thing to note. The lastAddedSchemaId sounds to me like an implementation detail from Java. In PyIceberg the current situation is simpler as you explained, so we could just do max(schema.id from tbl.schemas) + 1.

You sparked my interest here, let me double check this on the REST side as well 👍

[Fokko]

Thanks for raising this, this is currently not handled correctly on the rest side either.

I've created a PR here as a suggestion, but also feel free to fix it in your PR.

[Fokko]

@HonahX can you rebase? I think this one is ready to go!

[HonahX]

@Fokko Thanks for reviewing! I've pushed my updates (mostly on SetCurrentSchemaUpdate) and rebased the PR.

[Fokko]

While this is a very safe way of doing the copy, it is also rather expensive since we convert everything to a Python dict, and then create a new object again. Pydantic has the model_copy argument that seems to do what we're looking for:

Suggested change

	updated_metadata_data=copy(base_metadata.model_dump())
	updated_metadata_data["format-version"] =update.format_version
	updated_metadata_data=base_metadata.model_copy(**{"format-version": update.format_version})

We could construct a dict where we add all the changes (for the more complicated updated below), and then call model_copy for each update.

This will make a shallow copy by default (which I think is okay, since the model is immutable).

[HonahX]

Thanks for the explanation! I am a little worried about the immutability of table metadata. I think Pydantic's frozen config does not prevent updates to list, dict, etc. If we make shallow copy of list fields in metadata and later some code mistakenly alter the list (e.g. append something) in the updated metadata, the effect will be populated to the base_metadata too and it may be hard to detect. Do you think this might be a problem?

[Fokko]

In that case, we can be cautious and set deep=true. I would love to see some tests that validate the behavior. Those should be easy to add.

[HonahX]

Thanks for the suggestion! I've just implemented the suggested change on my end, but I'm still in the process of building the tests for shallow vs deep copy. Given that the current PR already contains lots of change, do you think it might be a good idea to make the model_copy transfer in a separate, follow-up PR?

[Fokko]

Sounds good @HonahX. I've created a new issue here: #179

[Fokko]

@HonahX Almost there, looking good 👍

[Fokko]

Great work @HonahX Thanks for picking this up!