Skip to content

Support database-scoped tablet insertion in table sessions - #19237

Open
shuwenwei wants to merge 6 commits into
masterfrom
feature/table-session-db-insert
Open

shuwenwei wants to merge 6 commits into
masterfrom
feature/table-session-db-insert

Conversation

@shuwenwei

Copy link
Copy Markdown
Member

Motivation

Table session Tablet insertion currently resolves the target database from the database configured when the session was opened or from the session's current database. A session that needs to write the same Tablet operation to multiple databases must execute USE, which changes session state and is inconvenient for mixed-database workloads.

This change adds a request-scoped database argument to Tablet insertion without changing the session's current database.

Changes

  • Add ITableSession.insert(Tablet tablet, String database).
  • Propagate the optional database through TSInsertTabletReq, including leader-cache grouping and redirects.
  • Apply the requested database only to the current InsertTabletStatement; the session and connection database remain unchanged.
  • Treat a null database like the existing one-argument API and use the session's current database.
  • Reject empty or tree-model database names at the server boundary.
  • Update both direct and pooled ITableSession implementations.
  • Add table-model integration coverage for writing to two databases, null fallback, and current-database preservation.

Example

Before, changing the write target also changes session state:

session.executeNonQueryStatement("USE db1");
session.insert(tabletForDb1);

session.executeNonQueryStatement("USE db2");
session.insert(tabletForDb2);

After, each insertion can select its database without changing the current database:

session.insert(tabletForDb1, "db1");
session.insert(tabletForDb2, "db2");

// Null uses the session's current database.
session.insert(tabletForCurrentDb, null);

Compatibility

The database is carried by a new optional Thrift field, so existing one-argument insert requests remain unchanged. As agreed for this change, the new overload relies on a same-version or newer server; an older server can ignore the unknown optional field and fall back to its session database.

Validation

  • ./mvnw spotless:apply succeeded for the affected modules.
  • git diff --check passed.
  • Per repository constraints, no build or test execution was performed.

@codecov

codecov Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.71186% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.94%. Comparing base (7b407ec) to head (f53ab9a).
⚠️ Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
...rc/main/java/org/apache/iotdb/session/Session.java 79.54% 9 Missing ⚠️
...db/queryengine/plan/parser/StatementGenerator.java 12.50% 7 Missing ⚠️
...apache/iotdb/session/pool/TableSessionWrapper.java 0.00% 3 Missing ⚠️
...in/java/org/apache/iotdb/session/TableSession.java 0.00% 2 Missing ⚠️
...ava/org/apache/iotdb/session/pool/SessionPool.java 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #19237      +/-   ##
============================================
+ Coverage     48.89%   48.94%   +0.05%     
  Complexity      689      689              
============================================
  Files          5588     5589       +1     
  Lines        413444   413897     +453     
  Branches      54537    54601      +64     
============================================
+ Hits         202154   202586     +432     
- Misses       211290   211311      +21     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@JackieTien97 JackieTien97 left a comment

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.

Thanks for the PR. I built this head (f53ab9a) as a 1C1D distribution and drove it with a small client program that uses the new ITableSession.insert(Tablet, String). I also ran the PR client against a server built from an older master (2075032) to check mixed versions.

What works

  • An explicit database receives the rows, and null falls back to the session database. The session's current database is unchanged afterwards, including after USE.
  • Auto-created tables land in the explicit database.
  • Privileges are checked against the explicit (lower-case) database. A user with INSERT only on db1 gets 803 for db2, and no table is created there. information_schema gives 701 and __audit gives 803.
  • TableSessionPool works, and the pooled session's database stays as configured.
  • Both retry paths reuse the request, so the database survives redirection and broken-connection retries.
  • CI: all Java UT/IT jobs pass. The new IT ran in Table 1C1D (31/31) and in 1C3D.

Blocking (details inline)

  1. A new client talking to an older server silently writes into the wrong database. The old server drops unknown field 14. So insert(tablet, "cdb2") on a session whose database is cdb1 returns success, the rows land in cdb1, and a missing table is auto-created in cdb1. The description mentions this fallback, but the client neither detects nor reports it.
  2. An upper-case tree prefix gets past the tree-model check, and the rows are written into the tree database. isTableModelDatabase is case-sensitive and runs before lower-casing. Take tree database root.treedb and table database treedb, both present. insert(tablet, "ROOT.treedb") returns success but creates aligned devices root.treedb.tt.<tag> in the tree database, and neither model can read the rows. This also works for a user who has only table INSERT ON DATABASE treedb and who gets 803 for a tree insert into root.treedb. If there is no matching table database, the same input fails with 500/301 errors that leak internal exception names.

Should fix

  • Database names are not validated. "" gives 301, and " " and "a.b" give 500. All three log a WARN with a full stack trace on every call. The two reused messages are also wrong for this API, in both en and zh.

  • ISession.openWithTableModelDatabaseCache widens the public API for an internal caller. The new abstract methods on ISession/ITableSession also break third-party implementations.

  • Route-cache keys are not normalized, and entries are created before the server validates the name.

  • Tests are missing for:

    • rejecting tree-prefixed names, in both lower and upper case
    • the grouped multi-device path with an explicit database

    Also, the "" assertion doesn't check the status code.

PR description
The Validation section says "Per repository constraints, no build or test execution was performed". No such constraint exists in CONTRIBUTING.md, AGENTS.md or the PR template. In fact, AGENTS.md asks for a full-reactor test-compile in both locales for cross-module and i18n changes. Some intermediate commits did not compile:

  • 7ab0db7 is missing the IDeviceID import, fixed in e8a15c6.
  • The IT's leaders::addAll only compiles after f53ab9a.

The head is green, so please just correct that sentence.

CI notes

  • The failing Python job is the pandas OutOfBoundsDatetime issue fixed on master by #19248, so a rebase should clear it.
  • The Sonar gate fails on Reliability issues in files this PR doesn't touch (InsertValues, TierManager, UDFTable).

11: optional bool isCompressed
12: optional list<byte> encodingTypes
13: optional byte compressType
14: optional string database

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.

Blocking: an older server ignores this field and writes into the session database.

Thrift servers skip unknown optional fields, so a server without this change never sees database and falls back to the session's current database. I ran the PR client against a server built from 2075032:

// session opened with database("cdb1"); cdb1.t1 and cdb2.t1 both exist
session.insert(tablet("t1", 3 rows), "cdb2");          // returns OK
session.insert(tablet("only_in_cdb2", 2 rows), "cdb2"); // returns OK

Result: cdb1.t1 gets the 3 rows, cdb2.t1 stays at 0, and table only_in_cdb2 is auto-created in cdb1. With no session database the call fails with 701, but whenever a session database is set the misrouting is silent.

The description acknowledges this fallback ("an older server can ignore the unknown optional field and fall back to its session database"). Documenting it isn't enough, though: the call reports success while the data lands in a different database, and the caller has no way to detect that. A rolling upgrade is a realistic way to hit this, since a redirect can send the request to a node that hasn't been upgraded yet.

Suggestion: in openSession, have the server advertise a capability in TSOpenSessionResp.configuration, next to the existing time-precision entry. The client records it per SessionConnection, because redirect connections are opened separately. When database != null and the connection lacks the capability, the client throws before it sends the request.

throw new IllegalArgumentException(
DataNodeQueryMessages.DATABASE_IS_NOT_SPECIFIED_FOR_INSERT);
}
if (!PathUtils.isTableModelDatabase(insertTabletReq.getDatabase())) {

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.

Blocking: the tree-model check is case-sensitive and runs before lower-casing, so ROOT.xxx gets through and the rows are written into the tree database.

PathUtils.isTableModelDatabase is just !startsWith("root."), so "ROOT.treedb" and "Root.treedb" pass. The name is then lower-cased to root.treedb. Schema validation and the privilege check resolve the table through unQualifyDatabaseName(...), which gives table treedb.tt, while schema/data partitioning and device creation use root.treedb. The other entry points don't have this gap: openSession lower-cases before IClientSession.setDatabaseName checks the name, and SQL rejects a "ROOT.xxx".t prefix with 701 (The tree model database shall not be specified in table model.).

Repro on this head. Tree database root.treedb has device d1, and table database treedb has table tt(tag1 TAG, m1 FIELD):

session.insert(tablet("tt", tags d0, d1; 2 rows), "ROOT.treedb"); // OK
session.insert(tablet("tt", 1 row), "Root.treedb");               // OK
session.insert(tablet("tt", 1 row), "treedb");                    // OK (control)
before after
table SELECT count(*) FROM treedb.tt 0 1 (only the control row)
tree COUNT DEVICES root.treedb.** 1 3: new aligned devices root.treedb.tt.d0 and root.treedb.tt.d1
tree COUNT TIMESERIES root.treedb.** 1 1

The rows are written: after FLUSH they are in a TsFile under sequence/root.treedb/.... Neither model can read them, though. Tree SELECT * FROM root.treedb.tt.d0 returns 0 rows and SHOW TIMESERIES root.treedb.tt.** is empty.

This also gets around the tree privilege. Take a user that has only GRANT INSERT ON DATABASE treedb:

  • tree INSERT INTO root.treedb.d9(time, s1) VALUES (1, 1) → 803, needs WRITE_DATA on [root.treedb.d9.s1]
  • insert(tablet, "root.treedb") → 701
  • insert(tablet, "ROOT.treedb") → OK, and device root.treedb.tt.pv2 is created in the tree database

If there is no matching table database, the same input surfaces internal errors instead of a clean 701:

  • "ROOT.db1" → 500 Create SchemaPartition failed because the database: root.db1 is not exists
  • "ROOT.db1" with a table that doesn't exist yet → 301 org.apache.iotdb.confignode.exception.DatabaseNotExistsException: Database: root.db1 doesn't exist.
  • "ROOT.treedb" before table database treedb existed → 500 Unknown database treedb

Also, the reused DATABASE_NAME_IS_TREE_MODEL_DATABASE ends with "...which is not allowed to set in the client session." (zh: "...不允许设置到客户端会话中。"). This API deliberately leaves the session untouched. THE_TREE_MODEL_DATABASE_SHALL_NOT_BE_SPECIFIED, which the SQL path uses for the same case, fits better.

Suggested shape:

final String database = insertTabletReq.getDatabase().toLowerCase(Locale.ENGLISH);
if (!PathUtils.isTableModelDatabase(database)) {
  throw new SemanticException(
      DataNodeQueryMessages.THE_TREE_MODEL_DATABASE_SHALL_NOT_BE_SPECIFIED);
}
TableConfigTaskVisitor.validateDatabaseName(database); // see the comment on the empty-name check
insertStatement.setDatabaseName(database);

insertStatement.setColumnCategories(columnCategories);
if (insertTabletReq.isSetDatabase()) {
if (insertTabletReq.getDatabase().isEmpty()) {
throw new IllegalArgumentException(

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.

Malformed names come back with inconsistent status codes:

database result
"" 301 [EXECUTE_STATEMENT_ERROR(301)] Exception occurred: insertTablet failed. database is not specified for insert:
" " 500 Unknown database
"a.b" 500 Unknown database a.b
null with no session database 701 database is not specified

The DataNode also logs each 301/500 response as a WARN with a full stack trace, on every call. For "" the cause is that ErrorHandlingUtils.onQueryException doesn't recognize IllegalArgumentException, so the exception falls through to onNpeOrUnexpectedException (DN log: StatementGenerator.java:394). In addition, DATABASE_IS_NOT_SPECIFIED_FOR_INSERT is a prefix that is supposed to be followed by the statement, as in the relational Analyzer. Used here, the message ends in a dangling colon (zh: "未指定插入操作的数据库:").

CREATE DATABASE already validates names with TableConfigTaskVisitor.validateDatabaseName. It rejects "", blanks, ., characters outside DATABASE_PATTERN, and names longer than 64. It throws a SemanticException that wraps IllegalPathException, which comes back as 509 and is logged at INFO without a stack trace. Calling it after lower-casing (see the comment above) gives every malformed name a clean user error.

INodeSupplier nodeSupplier)
throws IoTDBConnectionException;

void openWithTableModelDatabaseCache(

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.

This adds an internal cache-sharing hook to ISession, a public interface that users program against, and its nested-map parameter exposes the cache layout. The only callers are in SessionPool. The method has to be on the interface only because getSession() declares ISession session = queue.poll();. Opening the new connection through a local Session would avoid that:

Session newSession = constructNewSession();
newSession.openWithTableModelDatabaseCache(...);
session = newSession;

tryConstructNewSession() already uses a Session.

Separately, the new abstract methods here and the new ITableSession.insert(Tablet, String) break every third-party implementation of these interfaces at compile time. ISession already uses default methods for this kind of evolution (e.g. isEnableCacheLeader). insert(Tablet, String) could be a default method that delegates to insert(tablet) when database == null and throws UnsupportedOperationException otherwise.

}

private String resolveTableModelRouteDatabase(String database) {
String routeDatabase = database == null ? this.database : database;

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.

Nit: the route-cache key is the raw string, while the server lower-cases it, so "DB1" and "db1" get separate route maps for the same database. getTableModelDatabaseRoutes also calls computeIfAbsent before the server has validated the name. Every invalid name ("", "ROOT.x", typos) therefore leaves an empty entry in a map that a SessionPool shares across sessions. Lower-casing here, as the server does, and only creating the entry when a redirect is actually recorded would avoid both problems.

}
}

private static void setRelationalRequestDatabase(TSInsertTabletReq request, String database) {

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.

Nit: the null check is redundant. The generated setDatabase(null) leaves the field unset, because isSetDatabase() is database != null. More importantly, the database is attached at three separate call sites, each right after setWriteToTable(true), so a future send path can easily forget it. A single builder for relational tablet requests, taking the tablet and the database, would keep this in one place. That builder is also the natural home for the server-capability check suggested on client.thrift.

assertEquals("db1", dataSet.next().getField(0).getStringValue());
}

assertThrows(StatementExecutionException.class, () -> session.insert(tablet, ""));

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.

Please assert the status code here, not just the exception type. "" currently returns 301, the generic failure code, so this assertion would still pass if the call failed for an unrelated reason.

Missing cases:

  • Tree-prefixed names. Try "root.xxx", "ROOT.xxx" and "Root.xxx" while both tree database root.xxx and table database xxx exist. Assert the error code, and assert that no device appears under root.xxx.** (see the comment on StatementGenerator).
  • The grouped path with an explicit database. No test runs insertRelationalTabletByGroup with a database set. The new unit test uses a single device. In the IT, each explicit-database insert starts with an empty route map for that database, so it always goes to the default connection. Extending SessionCacheLeaderTest with a multi-device tablet whose devices have different cached leaders, written to an explicit database that differs from the session's, would cover the grouping, the redirect bookkeeping and the per-group requests carrying the database.

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.

2 participants