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

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.

platinumhamburg · 2025-12-31T03:23:06Z

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

-@Internal
-public interface RowColumnVector extends ColumnVector {
+/** IncIDGenerator is used to generate auto increment column ID. */
+public interface IncIDGenerator {


Please rename to SequenceGenerator

platinumhamburg · 2025-12-31T03:23:39Z

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

+
+/** Segment ID generator, fetch ID with a batch size. */
+@NotThreadSafe
+public class SegmentIncIDGenerator implements IncIDGenerator {


platinumhamburg · 2025-12-31T03:27:22Z

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

+import org.apache.fluss.server.zk.data.ZkData;
+
+/** AutoIncProcessor is used to process auto increment column. */
+public interface AutoIncProcessor {


Perhaps the AutoIncColumnProcessor could be renamed to AutoIncProcessor, and this static factory class could be renamed to AutoIncProcessors.

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.

platinumhamburg · 2025-12-31T03:39:30Z

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

+
+    @Nullable
+    @Override
+    public BinaryValue processAutoInc(BinaryValue oldValue) {


It would be great to add test cases that validate the system’s behavior under schema evolution, particularly when handling rows from different schema versions where the column set and/or column order may differ.

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 and others added 2 commits January 1, 2026 09:09

[kv] Add an implementation of AutoIncIDBuffer

449e035

stash code

0da32af

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

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

platinumhamburg commented Dec 29, 2025

Uh oh!

beryllw commented Dec 31, 2025

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants