Hi Jark, Thanks for your comments.
For comments: (1) Actually in the beginning I was going to use `oneof` syntax in proto3, but finally found that we are in proto2. Thus I fall back to using `enum` in proto2. Now I removed this `PbTableChange` definition. (2) & (3) It is good for me to introduce `AlterTableConfigsRequest` for specific alter requests. I have updated it in the PR[1]. Please check. And the `FlussTableChange` class is renamed to `TableChange` class. Regards, Yang Guo [1]: https://github.com/apache/fluss/pull/1625/files On Fri, Sep 19, 2025 at 9:51 PM Jark Wu <[email protected]> wrote: > Thanks for the discussion and sorry for jumping in late. > > Hi, Yuxia, > As we introduced in the java-client documentation[1], the Admin API manages > metadata and the cluster, the Table API is used for **reading and writing** > tables. > This will be a clear separation for Admin and Table, so I think it's fine > to put the `alterTable` in Admin. > > Hi Yang, > Here are some my comments: > (1) As we want to upgrade to proto3 syntax some time in the future, so we > try to avoid using `enum` type in the proto which is incompatible. > (2) I suggest to only introduce a `AlterTableConfigsRequest`, this allow us > can easily introduce a independent alter schema RPC in the future. > (3) The alter config message can be resued with FIP-12 > > The RPC can look like this > > message AlterTableConfigsRequest { > required PbTablePath table_path = 1; > required bool ignore_if_not_exists = 2; > repeated PbAlterConfigsRequestInfo config_changes = 3; > } > > // we have introduced this PB message in FIP-12 > message PbAlterConfigsRequestInfo{ > required string config_key = 1; > optional string config_value = 2; > required int32 op_type = 3; // SET=0, DELETE=1, APPEND=2, SUBTRACT=3 > } > > message AlterTablePropertiesResponse { > } > > What do you think? > > Best, > Jark > > [1]: https://fluss.apache.org/docs/apis/java-client/ > [2]: > > https://github.com/apache/fluss/blob/master/fluss-rpc/src/main/proto/FlussApi.proto#L27 > > > On Tue, 16 Sept 2025 at 21:59, Yang Guo <[email protected]> wrote: > > > Hi, > > > > Thanks for feedback from yuxia and liebing. > > > > I compared the implementation on update properties. Here is my > > understanding: > > > > In our design, to alter table, we need to provide two kinds of info: > > `TablePath` and `FlussTabledChanges`. The `TablePath` provides the table > > information and the `FlussTabledChanges` defines the changes. They are > > independent to each other. > > But in iceberg, the `UpdateProperties` defines both the table information > > and table changes. Each `UpdateProperties` object has the whole > > information needed to alter a table. e.g: > > ``` > > class PropertiesUpdate implements UpdateProperties { > > private final TableOperations ops; > > private final Map<String, String> updates = Maps.newHashMap(); > > private final Set<String> removals = Sets.newHashSet(); > > private TableMetadata base; > > } > > ``` > > > > The difference is that, in the iceberg, `UpdateProperties` implements an > > interface called `PendingUpdate`. To put all info together, it allows the > > update not be executed immediately. We can control when to `apply` and > > `commit` those changes. > > ``` > > public interface PendingUpdate<T> { > > > > /** > > * Apply the pending changes and return the uncommitted changes for > > validation. > > */ > > T apply(); > > > > /** > > * Apply the pending changes and commit. > > * > > * <p>Changes are committed by calling the underlying table's commit > > method. > > * > > * <p>Once the commit is successful, the updated table will be > refreshed. > > */ > > void commit(); > > } > > ``` > > > > Back to Fluss, I think it does not have any differences in clients side, > to > > pass `TablePath` and `FlussTableChanges` independently or together. > > But in Fluss server side, if we need those 'PendingUpdate' features, we > may > > have a new class to put them together. e.g: > > ``` > > class TablePathWithChanges { > > TablePath tablePath; > > List<FlussTableChanges> tableChanges; > > } > > ``` > > > > And for the class name, I will change from `FlussTableChange` to > > `TableChange` after we settle down the design. > > > > Regards, > > Yang Guo > > > > > > > > On Mon, Sep 15, 2025 at 2:31 PM Liebing Yu <[email protected]> wrote: > > > > > Hi, Yang Guo! > > > > > > Thanks for moving this forward. For the interfaces, I suggest using > > > TableChange instead of FlussTableChange. Since we are always in Fluss, > > > adding a "Fluss" prefix is a bit redundant. Another reason is that we > > might > > > also use this interface when modifying lake tables, in which case we > are > > > not modifying Fluss tables. > > > > > > Best regards, > > > Liebing Yu > > > > > > > > > On Fri, 12 Sept 2025 at 15:26, yuxia <[email protected]> > > wrote: > > > > > > > Hi, Yang Guo! > > > > Thnaks for driving this FIP. It must be a good improvement. > > > > > > > > About Api design in client side. I want to raise the design of > iceberg > > > for > > > > update properties[1] into discussion. > > > > The difference is how the api is attached to. In iceberg, the api is > > > > attached to `Table` instance instead of something like `Admin` or > > > > `Catalog`. > > > > Here is an example of How iceberg does: > > > > ``` > > > > public interface Table { > > > > > > > > /** > > > > * Create a new {@link UpdateProperties} to update table properties > > and > > > > commit the changes. > > > > * > > > > * @return a new {@link UpdateProperties} > > > > */ > > > > UpdateProperties updateProperties(); > > > > > > > > } > > > > ``` > > > > ``` > > > > public interface UpdateProperties { > > > > /** > > > > * Add a key/value property to the table. > > > > * > > > > * @param key a String key > > > > * @param value a String value > > > > * @return this for method chaining > > > > * @throws NullPointerException If either the key or value is null > > > > */ > > > > UpdateProperties set(String key, String value); > > > > > > > > /** > > > > * Remove the given property key from the table. > > > > * > > > > * @param key a String key > > > > * @return this for method chaining > > > > * @throws NullPointerException If the key is null > > > > */ > > > > UpdateProperties remove(String key); > > > > } > > > > ``` > > > > > > > > I don't have a strong opinion on this, but following Iceberg's design > > > > approach raises another question: how should we define the > > > responsibilities > > > > of `Admin` versus `Table`? > > > > > > > > For example, methods like listOffsets and getLatestKvSnapshots are > > > > currently placed in `Admin`, but they could also be appropriate for > > > > `Table`. This makes the responsibility boundaries less clear. > > > > > > > > cc @Jark What's your opinion on it. > > > > > > > > > > > > [1] > > > > > > > > > > https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/Table.java#L191 > > > > > > > > > > > > > > > > Best regards, > > > > Yuxia > > > > > > > > ----- 原始邮件 ----- > > > > 发件人: "Yang Guo" <[email protected]> > > > > 收件人: "dev" <[email protected]> > > > > 发送时间: 星期四, 2025年 9 月 11日 下午 9:33:38 > > > > 主题: [DISCUSS] FIP-15: Alter Table Interface > > > > > > > > Hi all, > > > > > > > > Fluss server and client do not support alter tables now, but there > are > > > many > > > > features required to alter table properties. After discussion with > > other > > > > fluss developers, I'd like to draft this FIP about the alter table > > > > interface. > > > > > > > > This FIP-15 is to design an alter table interface. Briefly speaking > it > > > adds > > > > an 'alterTable' API in both client and server side. Considering there > > > will > > > > be various different types of changes on a table (such as comments, > > > > properties, buckets, columns, etc), we introduce a 'FlussTableChange' > > > class > > > > to distinguish them. It will also make servers easy to determine how > to > > > > operate on the table based on change types. > > > > > > > > For mode details, you can see the FIP-15 draft: > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/FLUSS/FIP-15%3A+Alter+Table+Interface > > > > > > > > > > > > I also have implemented a PR to alter table properties based on this > > > > design, which could be a sample for this design: > > > > https://github.com/apache/fluss/pull/1625 > > > > > > > > Thanks in advance and looking forward to any feedback from you. > > > > > > > > Regards, > > > > Yang Guo > > > > > > > > > >
