Skip to content

Optimize relational tablet batch inserts#18132

Open
jt2594838 wants to merge 13 commits into
apache:masterfrom
Caideyipi:table_insert_tablets
Open

Optimize relational tablet batch inserts#18132
jt2594838 wants to merge 13 commits into
apache:masterfrom
Caideyipi:table_insert_tablets

Conversation

@jt2594838

@jt2594838 jt2594838 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Description

This PR optimizes relational insertTablets handling in the Java Session client.

  • Add a merge switch for relational tablets in Session builders and pools.
  • Reuse column index maps when checking and merging relational tablets.
  • Avoid unnecessary bitmap creation during merge.
  • Copy tablet data column-by-column with array copy.
  • Auto-disable merge after consecutive misses or when merge overhead is too high.
  • Add an ignored integration test for manually comparing insertTablet and insertTablets performance as tablet count increases on the same table.

Tests

  • mvn test -pl iotdb-client/session -am "-Dtest=SessionTest#testMergeRelationalTabletsWithHighDuplicatedColumns+testMergeRelationalTabletsSkipLowDuplicatedColumnsAndDifferentTables+testMergeRelationalTabletsStopAfterConsecutiveMisses+testMergeRelationalTabletsResetConsecutiveMissesAfterMerge+testMergeTabletsDisabledWhenMergeCostExceedsHalfOfInsertCost+testMergeTabletsKeepsEnabledWhenMergeCostIsNotTooHigh+testEnableMergeTabletsUsingBuilder" "-DfailIfNoTests=false" "-Dsurefire.failIfNoSpecifiedTests=false"
  • mvn test-compile -pl integration-test -am -P with-integration-tests -DskipTests "-Drat.skip=true"
  • mvn verify "-DskipUTs" "-Dit.test=IoTDBSessionRelationalIT#compareInsertTabletAndInsertTabletsPerformanceWithIncreasingTabletCount" "-DfailIfNoTests=false" "-Dfailsafe.failIfNoSpecifiedTests=false" "-Drat.skip=true" -pl integration-test -am -PTableSimpleIT -P with-integration-tests

@Caideyipi

Copy link
Copy Markdown
Collaborator

I found a few issues that should be addressed before merging:

  1. High: Relational insertTablets cannot auto-create a new table.

In InsertTablets.validateTableSchema, the call to validateTableSchema passes allowCreateTable=false. This makes ITableSession.insertTablets(List) fail when inserting into a new table, while the existing single-tablet insert path can auto-create the table. The current integration tests all create the tables first, so this behavior difference is not covered. Please either align this with the single-tablet path or clearly document and test that batch tablet insertion requires the table to exist.

  1. Medium: The new insertRelationalTablets path bypasses table-model redirection.

Session.insertRelationalTablets always sends the request through getDefaultSessionConnection().insertTablets(request) and ignores RedirectException. The existing insertRelationalTablet path uses the table-model leader cache and groups rows/tablets by endpoint when redirection is enabled. In a cluster, this new path can keep sending requests to the default node and will not refresh tableModelDeviceIdToEndpoint, which may offset the intended performance gain. Please consider reusing or extending the existing relational tablet redirection logic for the batch path.

  1. Low: Merge cost is recorded even when the insert fails.

recordMergeTabletsCost is called from finally, so failed RPCs or execution failures still contribute to the auto-disable decision. This can permanently disable merge due to failure-path latency rather than actual merge overhead. It would be safer to record the cost only after a successful insert, or explicitly exclude failed attempts from the heuristic.

Question: Is insertTablets intentionally not supposed to support auto table creation? If yes, the API documentation and tests should make that clear.

@Caideyipi

Copy link
Copy Markdown
Collaborator

Thanks for addressing the previous review comments. I re-checked the latest revision and there is still one blocking issue.

High: the new table-model insertTablets path can still drop data in cluster mode.

The current batch path catches RedirectException and only updates the leader cache via handleRelationalTabletsRedirection, but it does not retry the redirected tablets/rows. In addition, SessionConnection.insertTablets() throws the multi-device redirection as deviceEndPointMap, while the new handleRelationalTabletsRedirection only reads getEndPointList(), so in this case the table-model leader cache may not even be refreshed.

This is also visible in the current CI failure, and it hits the new/related tests directly:

  • IoTDBSessionRelationalIT.insertTabletsAutoCreateTableTest: expected 40 rows but got 20.
  • IoTDBInsertTableSessionPoolIT.testInsertTabletsWithDifferentTables: expected 5 rows but got 0.

Could you please make the batch insertTablets redirection handling complete? After receiving per-device/per-row redirect information, the client should regroup the affected tablets/rows by endpoint and retry the unwritten data, instead of only updating the cache and returning successfully.

Comment thread iotdb-client/session/src/main/java/org/apache/iotdb/session/Session.java Outdated
Comment thread iotdb-client/session/src/main/java/org/apache/iotdb/session/Session.java Outdated
request.addToColumnCategoriesList(toEnumOrdinalsAsBytes(tablet.getColumnTypes()));
}
try {
getDefaultSessionConnection().insertTablets(request);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

High: In the enableRedirection == false branch, this still calls the one-argument insertTablets(request) overload. That overload passes request.getPrefixPaths() to verifySuccessWithRedirectionForMultiDevices; for a relational request this list has one table name per Tablet, while the server now emits one redirect sub-status per row. For example, a single two-row Tablet whose rows are redirected produces two sub-statuses but only one prefix path, so processing the second status accesses devices.get(1) and throws IndexOutOfBoundsException after the write has already succeeded, instead of reaching the ignored RedirectException. Please pass row-aligned device IDs here as the enabled-redirection path does, or use a verification path that deliberately ignores redirect metadata. A regression test should cover redirection disabled, a multi-row Tablet, and a non-local leader.

long mergeTabletsCost = 0;
if (enableMergeTablets) {
final long mergeTabletsStartTime = System.nanoTime();
tablets = mergeRelationalTablets(tablets);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Low: Zero-row relational Tablets are not filtered before this merge. Two compatible empty Tablets are merged into a new Tablet with rowCount == 0, hence zero-capacity value arrays. With redirection enabled, addRelationalTabletToGroup then calls SessionUtils.isTabletContainsSingleDevice, which evaluates getDeviceID(0); a tagged zero-capacity Tablet throws IndexOutOfBoundsException there. This is inconsistent with insertRelationalTablet, which explicitly treats rowSize == 0 as a no-op, and with the server parser, which skips empty Tablets. Please filter zero-row Tablets before merging, define the all-empty batch behavior, and add a regression test.

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.

3 participants