[kv] Add an implementation of AutoIncIDBuffer #2161

beryllw · 2025-12-12T08:14:09Z

Purpose

Linked issue: close #2111

Brief change log

Tests

API and Format

Documentation

fluss-common/src/main/java/org/apache/fluss/metadata/Schema.java

fluss-common/src/test/java/org/apache/fluss/metadata/TableDescriptorTest.java

fluss-server/src/main/java/org/apache/fluss/server/kv/autoinc/SegmentIDGenerator.java

vamossagar12 · 2025-12-17T03:49:51Z

hi, I had created this Pr: #2119 for this issue. Do we want to prefer this one over that?

xx789633 · 2025-12-17T14:46:11Z

Hi @vamossagar12 , we prefer this one to work on since it is more comprehensive.

fluss-server/src/main/java/org/apache/fluss/server/utils/TableDescriptorValidation.java

fluss-common/src/main/java/org/apache/fluss/utils/json/ColumnJsonSerde.java

fluss-common/src/main/java/org/apache/fluss/config/ConfigOptions.java

beryllw · 2025-12-25T02:56:05Z

fluss-common/src/main/java/org/apache/fluss/metadata/Schema.java

                        new Column(
                                column.getName(),
-                                column.getDataType().copy(false),
+                                column.getDataType().copy(true),


Partial update is required, so the data type here needs to be temporarily set to nullable.

fluss-server/src/main/java/org/apache/fluss/server/kv/autoinc/AutoIncProcessor.java

fluss-server/src/main/java/org/apache/fluss/server/zk/data/ZkData.java

platinumhamburg

@beryllw Thanks for contribution, I left some comments.

fluss-server/src/main/java/org/apache/fluss/server/zk/data/ZkData.java

fluss-server/src/main/java/org/apache/fluss/server/zk/ZkSequenceIDCounter.java

fluss-server/src/main/java/org/apache/fluss/server/kv/autoinc/AutoIncProcessor.java

fluss-server/src/main/java/org/apache/fluss/server/kv/autoinc/IncIDGenerator.java

fluss-server/src/main/java/org/apache/fluss/server/kv/autoinc/SegmentSequenceGenerator.java

fluss-client/src/test/java/org/apache/fluss/client/table/FlussTableITCase.java

fluss-common/src/main/java/org/apache/fluss/config/ConfigOptions.java

platinumhamburg · 2025-12-29T06:20:25Z

fluss-server/src/main/java/org/apache/fluss/server/kv/KvTablet.java

            throws Exception {
-        walBuilder.append(ChangeType.INSERT, latestSchemaRow.replaceRow(currentValue.row));
-        kvPreWriteBuffer.put(key, currentValue.encodeValue(), logOffset);
+        BinaryValue newValue = autoIncProcessor.processAutoInc(currentValue);


I've been wondering about the relationship between auto-increment column handling and the Merge Engine. I think they should probably be integrated in the design to avoid redundant encoding of row data, which would otherwise lead to significant performance overhead.

I agree. We had better handle the partial updater and auto increment column at the same time.

Currently, auto-increment is only utilized for INSERT operations, which does not overlap with the functionalities of RowMerger and PartialUpdateRowMerger. I agree integrating auto-increment into RowMerger would result in a more streamlined design, and I will try to optimize this part.

I agree with this perspective. One point worth noting is that when changelogImage == ChangelogImage.WAL, the applyInsert method might not be invoked. It would be helpful to clarify the intended design and expected behavior in this case.

Additionally, since PartialUpdater is currently used only by DefaultRowMerger and isn’t a general-purpose class, we might defer considering its consolidation for now.

One point worth noting is that when changelogImage == ChangelogImage.WAL, the applyInsert method might not be invoked. It would be helpful to clarify the intended design and expected behavior in this case.

Agree. When changelogImage == ChangelogImage.WAL, auto-Increment column aren't supported. We could clarify this in the docs.

Here, I believe we should support this. ChangelogImage.WAL is an optimization mode whose core use case is to skip generating UPDATE_BEFORE records when a user's table does not require UPDATE_BEFORE information. This significantly reduces the storage, replication (leader-follower), and read overhead associated with the changelog. Building upon this foundation, skipping the lookup of the old value to further reduce CPU and I/O overhead is merely an incremental optimization. Therefore, under the condition changelogImage == ChangelogImage.WAL, it's not that we cannot support the auto-increment columns; rather, we simply cannot benefit from this incremental optimization and would still need to look up the old value.

The ChangelogImage.WAL mode is already supported. My concern is that when changelogImage == ChangelogImage.WAL, the ChangelogNormalize operator will be inserted, which will introduce additional computational overhead.

It's okay; this is Flink's behavior and can be optimized on the Flink side.

platinumhamburg · 2025-12-29T06:24:49Z

In addition to integration tests, I think we should also add several unit tests in KvTabletTest to cover behavior assertions after a TabletServer restart and after a single segment overflows.

beryllw · 2025-12-31T02:15:12Z

In addition to integration tests, I think we should also add several unit tests in KvTabletTest to cover behavior assertions after a TabletServer restart and after a single segment overflows.

We could create a new issue to address the failover problem, including the design and implementation of its corresponding recovery mechanisms.

fluss-server/src/main/java/org/apache/fluss/server/kv/autoinc/IncIDGenerator.java

fluss-server/src/main/java/org/apache/fluss/server/kv/autoinc/SegmentIncIDGenerator.java

fluss-server/src/main/java/org/apache/fluss/server/kv/autoinc/AutoIncProcessor.java

platinumhamburg · 2025-12-31T03:34:52Z

fluss-server/src/main/java/org/apache/fluss/server/kv/KvTablet.java

            throws Exception {
-        walBuilder.append(ChangeType.INSERT, latestSchemaRow.replaceRow(currentValue.row));
-        kvPreWriteBuffer.put(key, currentValue.encodeValue(), logOffset);
+        BinaryValue newValue = autoIncProcessor.processAutoInc(currentValue);


I agree with this perspective. One point worth noting is that when changelogImage == ChangelogImage.WAL, the applyInsert method might not be invoked. It would be helpful to clarify the intended design and expected behavior in this case.

Additionally, since PartialUpdater is currently used only by DefaultRowMerger and isn’t a general-purpose class, we might defer considering its consolidation for now.

fluss-server/src/main/java/org/apache/fluss/server/kv/autoinc/AutoIncColumnProcessor.java

platinumhamburg · 2026-01-07T07:34:26Z

fluss-server/src/main/java/org/apache/fluss/server/kv/KvTablet.java

        // Optimization: when using WAL mode and merger is DefaultRowMerger (full update, not
        // partial update), we can skip fetching old value for better performance since it
        // always returns new value. In this case, both INSERT and UPDATE will produce
        // UPDATE_AFTER.


Comment above should be updated. The description of TABLE_CHANGELOG_IMAGE in both ConfigOptions.java and options.md should also be updated.

platinumhamburg

I've left some additional comments.

platinumhamburg · 2026-01-07T08:04:22Z

fluss-client/src/test/java/org/apache/fluss/client/table/FlussFailServerTableITCase.java

+            upsertWriter.upsert(row("e", null, "batch2")).get();
+            // The auto-increment column should start from a new segment for now, and local cached
+            // IDs have been discarded.
+            assertThatRow(lookupRow(lookuper, keyRow.replaceRow(row("e", null, null))))


This is a good test addition; however, there's no need to create a new test class. The scenario can be simply incorporated into the existing tests in FlussTableITCase.java.

platinumhamburg · 2026-01-07T08:07:49Z

fluss-client/src/test/java/org/apache/fluss/client/table/FlussTableITCase.java

+            {"b", 1L, "batch1"},
+            {"c", 2L, "batch1"},
+            {"d", 3L, "batch1"},
+            {"e", 4L, "batch1"}


The tests are generally good, but they can be further improved by abstracting the logic for writing partial updates to a specified schema and performing query assertions into a common utility method to enhance readability—this would significantly simplify the tests. Additionally, the test data should include records with duplicate primary keys to strengthen the effectiveness of behavioral validation.

platinumhamburg · 2026-01-07T08:08:50Z

fluss-client/src/test/java/org/apache/fluss/client/table/FlussTableITCase.java

+    }
+
+    @Test
+    void testPutAutoIncColumnAndLookup() throws Exception {


This test doesn't appear to include any additional behavioral verification; it could potentially be merged with the previous test （testSingleBucketPutAutoIncColumnAndLookup）.

platinumhamburg · 2026-01-07T08:13:49Z

fluss-server/src/main/java/org/apache/fluss/server/kv/autoinc/AutoIncProcessor.java

+    public void configureSchema(int latestSchemaId) {
+        if (latestSchemaId != this.schemaId) {
+            Schema schema = schemaGetter.getSchema(latestSchemaId);
+            int[] autoIncColumnIds = schema.getAutoIncColumnIds();


From the schema implementation perspective, this is a high-overhead operation and should not be repeatedly computed unnecessarily; at the very least, some form of caching mechanism should be in place.

platinumhamburg · 2026-01-07T08:22:39Z

fluss-server/src/main/java/org/apache/fluss/server/kv/autoinc/AutoIncProcessor.java

+                                    sequenceGeneratorMap.get(autoIncColumnId));
+                } else {
+                    throw new IllegalStateException(
+                            "Not supported add auto increment column for a table.");


I believe intercepting here is unreasonable—the DDL has already succeeded, and the client has even attempted to write data. If this needs to be prohibited, it should be blocked on the DDL side.

Alternatively, the validation/assertion here may be acceptable, but the error message is inappropriate—please verify whether the DDL-side interception is comprehensive.

platinumhamburg · 2026-01-07T08:24:05Z

fluss-server/src/main/java/org/apache/fluss/server/kv/autoinc/AutoIncProcessor.java

+    // Supports removing or reordering columns; does NOT support adding an auto-increment column to
+    // an existing table.
+    public void configureSchema(int latestSchemaId) {
+        if (latestSchemaId != this.schemaId) {


The condition here should be inverted with an early return to avoid deep indentation of a large code block, which would otherwise degrade readability.

platinumhamburg · 2026-01-07T08:26:58Z

fluss-server/src/main/java/org/apache/fluss/server/kv/autoinc/AutoIncProcessor.java

+                int autoIncColumnId = autoIncColumnIds[0];
+                if (sequenceGeneratorMap.containsKey(autoIncColumnId)) {
+                    this.autoIncUpdater =
+                            new DefaultAutoIncUpdater(


It seems that different versions of DefaultAutoIncUpdater can be managed using a cache.

platinumhamburg · 2026-01-07T08:31:57Z

fluss-server/src/main/java/org/apache/fluss/server/kv/autoinc/AutoIncProcessor.java

+                    } else {
+                        rowEncoder.encodeField(
+                                i, flussFieldGetters[i].getFieldOrNull(oldValue.row));
+                    }


Could oldValue here be null? I don't think that's possible—this doesn't appear to be a scenario that RowMerger would encounter.

Here, oldValue actually corresponds to newValue in RowMerger.

beryllw marked this pull request as draft December 12, 2025 08:14

beryllw force-pushed the auto-inc-impl branch 2 times, most recently from 45dc38a to 4d674d7 Compare December 12, 2025 15:04

xx789633 reviewed Dec 12, 2025

View reviewed changes

fluss-common/src/main/java/org/apache/fluss/metadata/Schema.java Outdated Show resolved Hide resolved

fluss-common/src/test/java/org/apache/fluss/metadata/TableDescriptorTest.java Show resolved Hide resolved

xx789633 reviewed Dec 12, 2025

View reviewed changes

fluss-server/src/main/java/org/apache/fluss/server/kv/autoinc/SegmentIDGenerator.java Outdated Show resolved Hide resolved

beryllw force-pushed the auto-inc-impl branch 2 times, most recently from 19d0475 to 697dbcf Compare December 24, 2025 04:33

beryllw marked this pull request as ready for review December 24, 2025 06:03

beryllw requested a review from xx789633 December 24, 2025 06:10

xx789633 reviewed Dec 24, 2025

View reviewed changes

fluss-server/src/main/java/org/apache/fluss/server/utils/TableDescriptorValidation.java Outdated Show resolved Hide resolved

xx789633 reviewed Dec 24, 2025

View reviewed changes

fluss-common/src/main/java/org/apache/fluss/utils/json/ColumnJsonSerde.java Outdated Show resolved Hide resolved

xx789633 reviewed Dec 24, 2025

View reviewed changes

fluss-common/src/main/java/org/apache/fluss/config/ConfigOptions.java Outdated Show resolved Hide resolved

beryllw commented Dec 25, 2025

View reviewed changes

beryllw changed the title ~~[kv]: Add an implementation of AutoIncIDBuffer~~ [kv] Add an implementation of AutoIncIDBuffer Dec 25, 2025

beryllw requested a review from xx789633 December 25, 2025 03:16

xx789633 reviewed Dec 25, 2025

View reviewed changes

fluss-server/src/main/java/org/apache/fluss/server/kv/autoinc/AutoIncProcessor.java Outdated Show resolved Hide resolved

xx789633 reviewed Dec 25, 2025

View reviewed changes

fluss-server/src/main/java/org/apache/fluss/server/zk/data/ZkData.java Outdated Show resolved Hide resolved

beryllw requested a review from xx789633 December 26, 2025 01:13

platinumhamburg suggested changes Dec 29, 2025

View reviewed changes

platinumhamburg suggested changes Dec 31, 2025

View reviewed changes

beryllw force-pushed the auto-inc-impl branch from 6120fbc to 0da32af Compare January 3, 2026 14:26

beryllw marked this pull request as draft January 3, 2026 14:27

beryllw force-pushed the auto-inc-impl branch from 0da32af to cdc73b2 Compare January 5, 2026 14:18

beryllw added 2 commits January 6, 2026 14:55

[kv] Add an implementation of AutoIncIDBuffer

12bba24

Add TabletServer fail over test

0f145e5

beryllw force-pushed the auto-inc-impl branch from cdc73b2 to 0f145e5 Compare January 6, 2026 11:14

beryllw marked this pull request as ready for review January 6, 2026 11:16

beryllw added 2 commits January 6, 2026 19:35

clarify behavior under schema evolution

728c19d

fix ci

e137d85

beryllw requested a review from platinumhamburg January 7, 2026 02:17

xx789633 requested review from wuchong and removed request for platinumhamburg January 7, 2026 02:22

refactor AutoIncProcessor with safer and cleaner design

c1c09b6

beryllw requested a review from platinumhamburg January 7, 2026 03:10

support WAL mode for tables with auto-increment

04f5ccd

platinumhamburg suggested changes Jan 7, 2026

View reviewed changes

[kv] Add an implementation of AutoIncIDBuffer #2161

Are you sure you want to change the base?

[kv] Add an implementation of AutoIncIDBuffer #2161

Conversation

beryllw commented Dec 12, 2025

Purpose

Brief change log

Tests

API and Format

Documentation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vamossagar12 commented Dec 17, 2025

Uh oh!

xx789633 commented Dec 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

platinumhamburg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beryllw Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

platinumhamburg commented Dec 29, 2025

Uh oh!

beryllw commented Dec 31, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

platinumhamburg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

beryllw Jan 5, 2026 •

edited

Loading