[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.

platinumhamburg · 2025-12-29T05:47:50Z

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

+     *
+     * <p>/metadata/databases/[databaseName]/tables/[tableName]/auto_inc/col_[columnIdx]
+     */
+    public static final class AutoIncrementColumnZNode {


Schema evolution (adding a column) is already supported; It would be better to use column IDs instead of column indices here.

platinumhamburg · 2025-12-29T05:50:14Z

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

+        if (incrementValue.succeeded()) {
+            return incrementValue.preValue();
+        } else {
+            throw new Exception("Failed to add sequence id counter.");


Perhaps we could make the exception information clearer—for example, by including the sequenceIDPath.

platinumhamburg · 2025-12-29T05:53:16Z

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

+            int schemaId,
+            Configuration properties,
+            TableConfig tableConf,
+            Schema schema,


Use SchemaGetter instead of Schema.

platinumhamburg · 2025-12-29T05:54:02Z

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

+            TableConfig tableConf,
+            Schema schema,
+            ZooKeeperClient zkClient) {
+        int[] autoIncColumnIndexes = schema.getAutoIncColumnIndexes();


Use column IDs instead of column indices.

platinumhamburg · 2025-12-29T06:01:03Z

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


    /**
-     * Return row value.
+     * Returns the next auto increment column ID.


Consider renaming this method, as "column ID" is misleading and conflicts with the actual ID of a Column object, which can cause confusion. Moreover, while UID is one use case for auto-increment columns, it does not encompass all possible scenarios, so naming it with "ID" is not advisable.

platinumhamburg · 2025-12-29T06:08:21Z

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

+            this.current = start;
+        }
+
+        public long remaining() {


This part is a bit confusing to me—the outer check already handles negative values, so the Math.max here seems like an unnecessary overhead.

platinumhamburg · 2025-12-29T06:16:51Z

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

    }

+    @Test
+    void testPutAutoIncColumnAndLookup() throws Exception {


To achieve more precise test assertions, I recommend explicitly creating a single-bucket table, writing multiple batches of data—some with duplicate primary keys and others with distinct primary keys—and then verifying that the auto-increment column behaves correctly.

platinumhamburg · 2025-12-29T06:17:50Z

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

+                    .longType()
+                    .defaultValue(100000L)
+                    .withDescription(
+                            "The batch size of auto-increment IDs fetched from the distributed counter each time. This value determines the length of the ID segment cached locally. Default: 100000.");


New configuration options should be documented, including recommended values and a description of the behavioral impact associated with different settings.

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.

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 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 Outdated 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 from 4d674d7 to 19d0475 Compare December 23, 2025 09:27

[kv] Add an implementation of AutoIncIDBuffer

697dbcf

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

beryllw added 4 commits December 24, 2025 12:39

[kv] format code

9848c65

[kv] fix ci

4cba163

[kv] fix java8 compile

1f4f4c3

[kv] fix ci

6d4a2ec

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

rm prefetch and two segment optimize

b3edf78

beryllw commented Dec 25, 2025

View reviewed changes

rm unused change

c92eb94

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

beryllw added 2 commits December 25, 2025 11:45

fix ci

2401a64

fix ci

7c619ca

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

fix review comment

5d1fc42

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

platinumhamburg suggested changes Dec 29, 2025

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

Uh oh!

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!

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants