Skip to content

Conversation

@xx789633
Copy link
Contributor

@xx789633 xx789633 commented Oct 29, 2025

Purpose

Linked issue: close #2103

Brief change log

Tests

API and Format

Documentation

Copy link
Member

@wuchong wuchong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution @xx789633 .

However, could you open an issue before opening pull requests? Because a PR must be linked with an issue, so we can better track them. You can create all the sub-issues related to the FIP-16 under #1886

Comment on lines 66 to 70
private Schema(
List<Column> columns,
@Nullable PrimaryKey primaryKey,
@Nullable String autoIncrementColumn) {
this.columns = normalizeColumns(columns, primaryKey, autoIncrementColumn);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it’s cleaner to store auto-increment column information at the Schema level rather than within individual Column objects. This aligns with how primary keys are currently modeled and makes it straightforward to retrieve the full set of auto-increment columns without iterating over all columns.

Additionally, adding new fields to Column would require multiple constructor overloads, unnecessarily complicating the API.

Given that we may eventually support multiple auto-increment columns, I propose using List<String> (e.g., Schema#getAutoIncrementColumnNames()) instead of a single @Nullable String.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment. It's true that adding extra fields in Column complicates its API. Comments addressed.

@platinumhamburg
Copy link
Contributor

Thank you for the contribution @xx789633 .

However, could you open an issue before opening pull requests? Because a PR must be linked with an issue, so we can better track them. You can create all the sub-issues related to the FIP-16 under #1886

@xx789633 I'v created a subtask issue #2103 and linked to the umbrella issue #1886

@platinumhamburg
Copy link
Contributor

Thank you for the contribution @xx789633 .

However, could you open an issue before opening pull requests? Because a PR must be linked with an issue, so we can better track them. You can create all the sub-issues related to the FIP-16 under #1886

@wuchong Please review again when you have time.

@wuchong
Copy link
Member

wuchong commented Dec 6, 2025

Hi @xx789633 , Please do not use "git merge" to merge branches, otherwise the changes is hard to track and review. Please use "git rebase" to rebase branches instead. IntelliJ IDEA provide an easy tool to do git rebase, you can find the tool via VCS -> Git -> Rebase. Could you rebase the branch and resolve conflicts, so that I can continue reviewing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[kv] Add auto increment column in Fluss schema

3 participants