You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fluss.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2025/11/27 16:54:36 UTC
[PR] [iceberg] support altering table properties [fluss]
nastra opened a new pull request, #2043:
URL: https://github.com/apache/fluss/pull/2043
<!--
*Thank you very much for contributing to Fluss - we are happy that you want to help us improve Fluss. To help the community review your contribution in the best possible way, please go through the checklist below, which will get the contribution into a shape in which it can be best reviewed.*
## Contribution Checklist
- Make sure that the pull request corresponds to a [GitHub issue](https://github.com/apache/fluss/issues). Exceptions are made for typos in JavaDoc or documentation files, which need no issue.
- Name the pull request in the format "[component] Title of the pull request", where *[component]* should be replaced by the name of the component being changed. Typically, this corresponds to the component label assigned to the issue (e.g., [kv], [log], [client], [flink]). Skip *[component]* if you are unsure about which is the best component.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Make sure that the change passes the automated tests, i.e., `mvn clean verify` passes.
- Each pull request should address only one issue, not mix up code from multiple issues.
**(The sections below can be removed for hotfixes or typos)**
-->
### Purpose
This adds support for altering Iceberg table properties
<!-- What is the purpose of the change -->
### Brief change log
<!-- Please describe the changes made in this pull request and explain how they address the issue -->
### Tests
added a unit test that sets/removes table properties
### API and Format
<!-- Does this change affect API or storage format -->
### Documentation
<!-- Does this change introduce a new feature -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [iceberg] support altering table properties [fluss]
Posted by "MehulBatra (via GitHub)" <gi...@apache.org>.
MehulBatra commented on PR #2043:
URL: https://github.com/apache/fluss/pull/2043#issuecomment-3638162027
Overall LGTM!
Left small comments, @luoyuxia it would be great if you could take a look too!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [iceberg] support altering table properties [fluss]
Posted by "MehulBatra (via GitHub)" <gi...@apache.org>.
MehulBatra commented on code in PR #2043:
URL: https://github.com/apache/fluss/pull/2043#discussion_r2607528900
##########
fluss-lake/fluss-lake-iceberg/src/main/java/org/apache/fluss/lake/iceberg/IcebergLakeCatalog.java:
##########
@@ -249,6 +271,14 @@ private void setFlussPropertyToIceberg(
}
}
+ private static String convertFlussPropertyKeyToIceberg(String key) {
Review Comment:
If a user passes "iceberg." or "fluss." (just the prefix with no actual property name)
This will attempt to set/remove a property with an empty key in Iceberg, which could create invalid table metadata
wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [iceberg] support altering table properties [fluss]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2043:
URL: https://github.com/apache/fluss/pull/2043#discussion_r2569724326
##########
fluss-lake/fluss-lake-iceberg/src/main/java/org/apache/fluss/lake/iceberg/IcebergLakeCatalog.java:
##########
@@ -119,8 +121,27 @@ public void createTable(TablePath tablePath, TableDescriptor tableDescriptor, Co
@Override
public void alterTable(TablePath tablePath, List<TableChange> tableChanges, Context context)
throws TableNotExistException {
- throw new UnsupportedOperationException(
- "Alter table is not supported for Iceberg at the moment");
+ try {
+ Table table = icebergCatalog.loadTable(toIcebergTableIdentifier(tablePath));
+ UpdateProperties updateProperties = table.updateProperties();
+ for (TableChange tableChange : tableChanges) {
+ if (tableChange instanceof TableChange.SetOption) {
+ TableChange.SetOption option = (TableChange.SetOption) tableChange;
+ updateProperties.set(
+ convertFlussPropertyKeyToIceberg(option.getKey()), option.getValue());
+ } else if (tableChange instanceof TableChange.ResetOption) {
+ TableChange.ResetOption option = (TableChange.ResetOption) tableChange;
+ updateProperties.remove(convertFlussPropertyKeyToIceberg(option.getKey()));
+ } else {
+ throw new UnsupportedOperationException(
+ "Unsupported table change: " + tableChange.getClass());
+ }
+ }
+
+ updateProperties.commit();
+ } catch (TableNotExistException e) {
+ throw new TableNotExistException("Table " + tablePath + " not exists.");
Review Comment:
The error message contains a grammatical error. It should be 'does not exist' instead of 'not exists'.
```suggestion
throw new TableNotExistException("Table " + tablePath + " does not exist.");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [iceberg] support altering table properties [fluss]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #2043:
URL: https://github.com/apache/fluss/pull/2043#discussion_r2609940108
##########
fluss-lake/fluss-lake-iceberg/src/main/java/org/apache/fluss/lake/iceberg/IcebergLakeCatalog.java:
##########
@@ -249,6 +271,14 @@ private void setFlussPropertyToIceberg(
}
}
+ private static String convertFlussPropertyKeyToIceberg(String key) {
Review Comment:
You would just end up with an empty table property that wouldn't do anything, so I don't think we need to do any special casing here and one wouldn't have invalid table metadata. Iceberg also doesn't prevent against this case as can be seen in the impl: https://github.com/apache/iceberg/blob/fe23584fc3af9f0ea1371989030b0a99affb233f/core/src/main/java/org/apache/iceberg/PropertiesUpdate.java#L50-L70
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [iceberg] support altering table properties [fluss]
Posted by "MehulBatra (via GitHub)" <gi...@apache.org>.
MehulBatra merged PR #2043:
URL: https://github.com/apache/fluss/pull/2043
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [iceberg] support altering table properties [fluss]
Posted by "MehulBatra (via GitHub)" <gi...@apache.org>.
MehulBatra commented on code in PR #2043:
URL: https://github.com/apache/fluss/pull/2043#discussion_r2607555541
##########
fluss-lake/fluss-lake-iceberg/src/main/java/org/apache/fluss/lake/iceberg/IcebergLakeCatalog.java:
##########
@@ -119,8 +122,27 @@ public void createTable(TablePath tablePath, TableDescriptor tableDescriptor, Co
@Override
public void alterTable(TablePath tablePath, List<TableChange> tableChanges, Context context)
Review Comment:
For primary key tables, Fluss automatically sets merge-on-read properties for Iceberg table
eg: icebergProperties.put("write.delete.mode", "merge-on-read)...etc
We should not allow user to change that for Primary key table I feel it was set purposely how to merge data and delete files for reading, we can put a check on that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [iceberg] support altering table properties [fluss]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #2043:
URL: https://github.com/apache/fluss/pull/2043#discussion_r2610142508
##########
fluss-lake/fluss-lake-iceberg/src/main/java/org/apache/fluss/lake/iceberg/IcebergLakeCatalog.java:
##########
@@ -119,8 +122,27 @@ public void createTable(TablePath tablePath, TableDescriptor tableDescriptor, Co
@Override
public void alterTable(TablePath tablePath, List<TableChange> tableChanges, Context context)
Review Comment:
that makes sense. I've added some checks and a test to make sure that those reserved properties can't be altered
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [iceberg] support altering table properties [fluss]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #2043:
URL: https://github.com/apache/fluss/pull/2043#discussion_r2609940108
##########
fluss-lake/fluss-lake-iceberg/src/main/java/org/apache/fluss/lake/iceberg/IcebergLakeCatalog.java:
##########
@@ -249,6 +271,14 @@ private void setFlussPropertyToIceberg(
}
}
+ private static String convertFlussPropertyKeyToIceberg(String key) {
Review Comment:
I don't think table metadata would be invalid and Iceberg also doesn't prevent against this case as can be seen in the impl: https://github.com/apache/iceberg/blob/fe23584fc3af9f0ea1371989030b0a99affb233f/core/src/main/java/org/apache/iceberg/PropertiesUpdate.java#L50-L70
You would just end up with an empty table property that wouldn't do anything, so I don't think we need to do any special casing here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [iceberg] support altering table properties [fluss]
Posted by "MehulBatra (via GitHub)" <gi...@apache.org>.
MehulBatra commented on code in PR #2043:
URL: https://github.com/apache/fluss/pull/2043#discussion_r2614586971
##########
fluss-lake/fluss-lake-iceberg/src/main/java/org/apache/fluss/lake/iceberg/IcebergLakeCatalog.java:
##########
@@ -249,6 +271,14 @@ private void setFlussPropertyToIceberg(
}
}
+ private static String convertFlussPropertyKeyToIceberg(String key) {
Review Comment:
Makes sense, thanks for the reference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [iceberg] support altering table properties [fluss]
Posted by "MehulBatra (via GitHub)" <gi...@apache.org>.
MehulBatra commented on code in PR #2043:
URL: https://github.com/apache/fluss/pull/2043#discussion_r2607555541
##########
fluss-lake/fluss-lake-iceberg/src/main/java/org/apache/fluss/lake/iceberg/IcebergLakeCatalog.java:
##########
@@ -119,8 +122,27 @@ public void createTable(TablePath tablePath, TableDescriptor tableDescriptor, Co
@Override
public void alterTable(TablePath tablePath, List<TableChange> tableChanges, Context context)
Review Comment:
For primary key tables, Fluss automatically sets merge-on-read properties for Iceberg table
eg: icebergProperties.put("write.delete.mode", "merge-on-read)
We should not allow user to change that for Primary key table I feel it was set purposely how to merge data and delete files for reading, we can put a check on that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [iceberg] support altering table properties [fluss]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #2043:
URL: https://github.com/apache/fluss/pull/2043#discussion_r2610148541
##########
fluss-lake/fluss-lake-iceberg/src/main/java/org/apache/fluss/lake/iceberg/IcebergLakeCatalog.java:
##########
@@ -119,8 +131,35 @@ public void createTable(TablePath tablePath, TableDescriptor tableDescriptor, Co
@Override
public void alterTable(TablePath tablePath, List<TableChange> tableChanges, Context context)
throws TableNotExistException {
- throw new UnsupportedOperationException(
- "Alter table is not supported for Iceberg at the moment");
+ try {
+ Table table = icebergCatalog.loadTable(toIcebergTableIdentifier(tablePath));
+ UpdateProperties updateProperties = table.updateProperties();
+ for (TableChange tableChange : tableChanges) {
+ if (tableChange instanceof TableChange.SetOption) {
+ TableChange.SetOption option = (TableChange.SetOption) tableChange;
+ checkArgument(
Review Comment:
@MehulBatra does it make sense to throw IAE exception here, since the alterTable API doesn't specify what to throw in such a case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [iceberg] support altering table properties [fluss]
Posted by "MehulBatra (via GitHub)" <gi...@apache.org>.
MehulBatra commented on code in PR #2043:
URL: https://github.com/apache/fluss/pull/2043#discussion_r2614573340
##########
fluss-lake/fluss-lake-iceberg/src/main/java/org/apache/fluss/lake/iceberg/IcebergLakeCatalog.java:
##########
@@ -119,8 +131,35 @@ public void createTable(TablePath tablePath, TableDescriptor tableDescriptor, Co
@Override
public void alterTable(TablePath tablePath, List<TableChange> tableChanges, Context context)
throws TableNotExistException {
- throw new UnsupportedOperationException(
- "Alter table is not supported for Iceberg at the moment");
+ try {
+ Table table = icebergCatalog.loadTable(toIcebergTableIdentifier(tablePath));
+ UpdateProperties updateProperties = table.updateProperties();
+ for (TableChange tableChange : tableChanges) {
+ if (tableChange instanceof TableChange.SetOption) {
+ TableChange.SetOption option = (TableChange.SetOption) tableChange;
+ checkArgument(
Review Comment:
I believe that should be good, I could see it being used in as standard utility across
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org