Repository navigation
Conversation
IoTDBTablePreparedStatement.processQueryResult() called the 14-argument IoTDBJDBCResultSet constructor, which hard-codes tableModel=false and replaces the server-provided columnIndex2TsBlockColumnIndexList with an identity mapping. As a result, every table-model PreparedStatement query with a non-identity projection returned columns in physical order, and a repeated projection/alias (duplicate mapping entries) threw ArrayIndexOutOfBoundsException. Use the full constructor, forwarding the connection charset, the response's tableModel flag and a null-safe defensive copy of the column mapping, matching what IoTDBStatement already does. The copy is required because IoTDBRpcDataSet prepends the tree-model time column in place, and it must stay null-safe so the existing fallback still applies when the server sends no mapping. Add mocked-RPC unit tests (reordered columns, repeated projection, identity control, missing mapping fallback) and real-server IT coverage in the JDBC and PREPARE/EXECUTE paths, including TIMESTAMP projection.
…d-statement-column-mapping # Conflicts: # iotdb-client/jdbc/src/test/java/org/apache/iotdb/jdbc/IoTDBTablePreparedStatementTest.java
JackieTien97
left a comment
There was a problem hiding this comment.
The fix itself is correct and I verified it end to end; the requested changes are about test coverage and scope claims rather than the change under test.
Verification performed (PR head 7ca4f353 merged into master@509c3954, Windows, JDK 21)
| Check | Result |
|---|---|
IoTDBTablePreparedStatementTest |
28/28 pass |
Same unit tests with IoTDBTablePreparedStatement reverted to master |
exactly the reordered and repeated cases fail (expected:<20> but was:<10> / ArrayIndexOutOfBoundsException); the identity and missing-mapping controls still pass |
IoTDBTablePreparedStatementJDBCIT (TableSimpleIT, 1C1D) |
21/21 pass |
IoTDBTablePreparedStatementIT (TableSimpleIT, 1C1D) |
14/14 pass |
| Live 1C1D server probe, PR client | PreparedStatement and Statement return the same column values on all 12 query shapes tried (reordered, repeated/aliased, *, tag/time, expressions, aggregates, ORDER BY, subquery) |
| Same probe, master client | 10 of 12 shapes wrong or throwing (UnsupportedOperationException / ArrayIndexOutOfBoundsException / silently shifted values when the types happen to match); only SELECT * and the identity projection are correct |
fetchSize=1 paging and repeated executeQuery() on one statement, with reordered/repeated projections |
correct with the PR client |
| google-java-format 1.28.0 dry run | no changes needed, no unused imports |
Requested changes:
- Add a test for the forwarded
charset(inline comment with the measured before/after bytes). It is a real behaviour fix that currently nothing guards. - Correct the PR description where it does not match the code (inline comments): the two new
IoTDBTablePreparedStatementITcases are not regression tests for this fix,tableModelis an inert argument, and the defensive copy is not required.
Out of scope but worth a separate issue: IoTDBTablePreparedStatement.execute() on a query calls executeInternal() and discards the response, so getResultSet() afterwards returns null. This predates the PR and is unchanged by it.
Not verified here: the TableClusterIT (1C3D) topology for the two IT classes; CI has to cover that.
| resp.isSetMoreData() && resp.isMoreData(), | ||
| zoneId); | ||
| zoneId, | ||
| charset, |
There was a problem hiding this comment.
Forwarding charset is a genuine behaviour fix, but it has no test. Measured on a live server with a connection opened with charset=GBK, reading a row whose value is 中文:
- master client,
getBytes(1)on aPreparedStatement:[-28, -72, -83, -26, -106, -121](UTF-8, wrong) - master client, same value through a plain
Statement:[-42, -48, -50, -60](GBK, correct) - PR client, both paths:
[-42, -48, -50, -60]
Please add a mock-based unit test for this (a STRING/TEXT column plus a non-default charset passed to the six-argument constructor, asserting getBytes), otherwise a future refactor can drop the argument again without any test failing.
| zoneId); | ||
| zoneId, | ||
| charset, | ||
| resp.isSetTableModel() && resp.isTableModel(), |
There was a problem hiding this comment.
Two scope notes on this call:
tableModelis inert.IoTDBRpcDataSettakes the parameter but never reads it (declared atIoTDBRpcDataSet.java:112, no other use), so the previous hard-codedfalsehad no observable effect. Forwarding it is fine for consistency withIoTDBStatement, but the PR description should not present it as one of the fixed bugs.- The defensive copy is harmless but not required. The in-place
columnIndex2TsBlockColumnIndexList.add(0, -1)inIoTDBRpcDataSetruns only whenignoreTimeStamp == false, which does not happen for the table model, andIoTDBStatementpasses the Thrift list through the same constructor without copying. Keeping the copy is fine; please soften the "required" wording in the description, since readers may otherwise take it as evidence of a mutation path that does not exist here.
| } | ||
|
|
||
| @Test | ||
| public void testPrepareAndExecuteWithReorderedProjection() { |
There was a problem hiding this comment.
This case and testPrepareAndExecuteWithRepeatedProjection below do not exercise the code changed by this PR. SQL-level PREPARE / EXECUTE goes through IoTDBStatement.executeQuery() and the full IoTDBJDBCResultSet constructor, which already forwarded columnIndex2TsBlockColumnIndexList before this PR. The client-side IoTDBTablePreparedStatement (the ?-parameter path through executePreparedStatement and processQueryResult) is never involved.
Verified with the master client on a live 1C1D server, using the same two projections on an equivalent table (filtering on an INT32 column instead of id): PREPARE ... FROM SELECT value, name ... followed by EXECUTE ... USING already returns 200.3, Bob, and the name AS x, name AS y variant already returns Bob, Bob. So both new cases pass with and without this fix.
They are still useful as coverage for the server-side prepared-statement path, but please do not count them as regression tests for this fix in the PR description. The regression coverage for the changed code is the new unit tests plus IoTDBTablePreparedStatementJDBCIT, whose reordered / repeated / TIMESTAMP queries I confirmed fail against the master client.
…edStatement Add a mocked-RPC unit test for the charset argument forwarded to IoTDBJDBCResultSet. A STRING column holds 中文 and the statement is opened with a GBK charset; getBytes(int) and getBytes(String) must encode with GBK rather than the TSFileConfig default. The test fails against the pre-fix code (UTF-8 bytes) and passes with it. buildQueryResp() now takes the column data types so a non-INT32 column can be described; the existing INT32 helper delegates to it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Problem
IoTDBTablePreparedStatement.processQueryResult()built its result set with the 14-argumentIoTDBJDBCResultSetconstructor. That constructor substitutesIntStream.range(0, columnNameList.size())for the server-providedcolumnIndex2TsBlockColumnIndexList, and never assignscharset, so the field keeps itsTSFileConfig.STRING_CHARSETinitializer.Consequences for table-model prepared statements:
SELECT b, a FROM t WHERE a > ?) silently return values in physical order instead of the projection order.SELECT a AS x, a AS y ...) map the second logical column to a physical index that does not exist, raisingArrayIndexOutOfBoundsException.getBytes(...)ignores the connection charset. On a connection opened withcharset=GBKand a stored value of中文, the master client returns UTF-8 bytes ([-28,-72,-83,-26,-106,-121]) through aPreparedStatementbut GBK bytes ([-42,-48,-50,-60]) through a plainStatement, for the same response.Notably,
DatasetHeader.setTableColumnToTsBlockIndexMapbuilds the mapping from each output symbol's position in the child output symbol list, so non-identity mappings and duplicate entries are both reachable from the server, not only from synthetic fixtures.Fix
In
IoTDBTablePreparedStatement.processQueryResult(), call the fullIoTDBJDBCResultSetconstructor, asIoTDBStatementalready does, and forward:charset— a genuine behaviour fix, covered by a new test.resp.isSetTableModel() && resp.isTableModel()— for consistency withIoTDBStatementonly. This parameter is inert:IoTDBRpcDataSettakes it and never reads it, so the previous hard-codedfalsehad no observable effect. It is not one of the fixed bugs.resp.getColumnIndex2TsBlockColumnIndexList(). The copy is not needed to fix any known path: the in-placecolumnIndex2TsBlockColumnIndexList.add(0, -1)inIoTDBRpcDataSetruns only whenignoreTimeStamp == false, which does not happen for the table model, andIoTDBStatementpasses the Thrift list straight through without copying. It is kept as cheap isolation from the response object, and is null-safe so a response without a mapping still falls back to the existing derivation.The 14-argument constructor is left in place; it still has callers in the metadata classes.
Tests
Mocked-RPC unit tests (
IoTDBTablePreparedStatementTest) assert bothgetInt(int)andgetInt(String)plus the actual values:["b","a"][1,0][10,20][20,10],b=20,a=10["x","y"][0,0][10][10,10],x=10,y=10(no exception)["a","b"][0,1][10,20][10,20](identity control)["a","b"][10,20][10,20](fallback regression)A separate unit test covers the charset forward: a
STRINGcolumn holding中文, with aGBKcharset passed to the six-argument constructor, asserting the bytes from bothgetBytes(int)andgetBytes(String). It fails against the pre-fix code (UTF-8 bytes, 6 long) and passes after.Real-server ITs:
IoTDBTablePreparedStatementJDBCITcovers reordered projection, repeated projection/aliases, identity control, and TIMESTAMP projection, and the reviewer confirmed these fail against the master client.IoTDBTablePreparedStatementITaddsPREPARE/EXECUTEcoverage for reordered and repeated projections. That path goes throughIoTDBStatement.executeQuery()and the full constructor, which already forwarded the mapping before this PR, so those two cases pass with and without the fix. They are coverage for the server-side prepared-statement path, not regression tests for this change.Results on this branch (TableSimpleIT, 1 ConfigNode + 1 DataNode):
IoTDBTablePreparedStatementTest: 29/29 passIoTDBTablePreparedStatementJDBCIT: 21/21 passIoTDBTablePreparedStatementIT: 14/14 passOut-of-range ordinals / unknown column names returning
SQLExceptionis a separate JDBC-conformance concern and is not gated on by this fix.Closes #18846
🤖 Generated with Claude Code