Fix Java client time column access throwing ArrayIndexOutOfBoundsException (#17407) - #17408
Fix Java client time column access throwing ArrayIndexOutOfBoundsException (#17407)#17408PDGGK wants to merge 1 commit into
Conversation
4cbf842 to
5d080cb
Compare
|
Hi, you can subscribe to the development mailing list and mention this PR there. This way, other developers can help you review the PR more quickly. https://iotdb.apache.org/Community/Communication-Channels.html#_1-mailing-lists |
5d080cb to
3fcfd28
Compare
|
Thanks for the suggestion, @HTHou. Separately, I noticed CI was red and it was my change that caused it: making I've restored the time-column branch in the |
|
@HTHou — apologies for sitting on your suggestion for four months; picking it back up now, and taking it properly this time. Current status. The bug is still present on master ( Precedent. The C++ side of the same asymmetry is already merged as #17400, approved by @jt2594838 on 2026-04-01. Worth being precise about the design, because that PR's description is stale: it originally proposed returning the timestamp cast to What's wrong with the PR as it stands, all on me:
Plan: rebase onto master, move the messages into |
…ption (apache#17407) In the tree model the time pseudo-column is addressed by a negative TsBlock column index. getLong, getString and getObject handle it; getBoolean, getInt, getFloat, getDouble and getBinary fell through to TsBlock.getColumn(-1), and isNull(index, rowNum) is index >= 0 && ..., so execution entered the value branch. The resulting ArrayIndexOutOfBoundsException is unchecked, while java.sql.ResultSet declares throws SQLException and IoTDBJDBCResultSet only catches StatementExecutionException, so it crossed the JDBC boundary uncaught. Add tsBlockColumnIndex < 0 guards to the five strictly-typed getters, after checkRecord() and before isNull(...), throwing StatementExecutionException. getDate delegates to getInt and is therefore rejected too. The messages go through RpcMessages (en and zh) rather than being string literals, and their texts match the ones merged for the C++ client in apache#17400 verbatim. Add a regression test asserting that the time column surfaces as SQLException rather than leaking an unchecked exception, while getLong, getString, getObject and getTimestamp keep returning the timestamp. Signed-off-by: Zihan Dai <1436286758@qq.com>
3fcfd28 to
5f21e6b
Compare
|
Pushed the rework I described on 2026-08-02, rebuilt on current master One correction to what I wrote in August. I said then that #17400 had already
That cites the current Java behaviour as the model for the C++ change — and it The argument I would actually make is narrower and does not depend on #17400: @HTHou — you suggested the dev list back in April as a way to get more review The push created twelve workflow runs and they are all sitting at |
Reject the tree-model time column in the strictly-typed getters (#17407)
In the tree model a result set's time pseudo-column is addressed by a negative
TsBlock column index.
getLong,getStringandgetObjecthandle that;getBoolean,getInt,getFloat,getDoubleandgetBinarydo not — theyfall straight through to
TsBlock.getColumn(tsBlockColumnIndex)with-1.isNull(index, rowNum)isindex >= 0 && ..., so it returnsfalsefor thetime column and execution enters the value branch rather than the null branch.
Why this is a defect rather than a quirk
java.sql.ResultSet.getIntand its siblings declarethrows SQLException. Whatcomes out here instead is an unchecked
ArrayIndexOutOfBoundsException, andIoTDBJDBCResultSetonly catchesStatementExecutionException:So the exception crosses the JDBC boundary uncaught. A caller writing the
documented
catch (SQLException)cannot handle it. That is the argument for thechange: it is a JDBC contract violation, independent of any judgement about which
exception type is nicer.
The
SessionDataIteratorgetters shareIoTDBRpcDataSetand pass it throughunwrapped as well.
Why #17400 does not settle this for Java
The same defect in the C++ client was fixed in #17400 (approved by @jt2594838,
merged 2026-04-01). This PR is the Java counterpart, deliberately down to the
wording: the same five getters, the same guard placed after
checkRecord()andbefore
isNull(...), and the five messages character-for-character identical tothe ones that merged there.
A user hitting this through two different clients should not have to learn two
different messages.
But #17400 should not be read as having settled this for Java, and I do not
want to imply that it did. In that review @CritasWang wrote:
That describes the behaviour this PR changes, and it describes it accurately —
ArrayIndexOutOfBoundsExceptionis anIndexOutOfBoundsException. So #17400cited the current Java behaviour as the model, and here I am arguing the Java
side should move. I think the JDBC contract is the reason it should, but that is
a new argument and not a conclusion anyone reached before, so it deserves an
explicit objection if you disagree.
getBinaryis in the guarded set because it is in the merged #17400 set: thatreview named four getters, and the patch that was approved and merged carries
five.
Changes
IoTDBRpcDataSet.javatsBlockColumnIndex < 0guards throwingStatementExecutionExceptioni18n/en/RpcMessages.javai18n/zh/RpcMessages.javaIoTDBJDBCResultSetTest.javaThe messages are constants rather than string literals because this class now
routes every message through
RpcMessages(compile-time i18n, #17613 / #18110);the previous revision of this PR predated that and had bare literals.
A side effect worth naming
getDatedelegates togetIntByTsBlockColumnIndex, so it now rejects the timecolumn too. Nobody has decided that — #17400 concerned the C++ client and did not
discuss
getDate. It follows from guardinggetInt, and reading aLocalDateout of the time column was never meaningful, but it is a behaviour change beyond
the five getters and I would rather state it than have it noticed later.
Verification
Test first, against unpatched master:
The assertion distinguishes "leaked an unchecked exception" from "threw
something", so it cannot pass by accident. With the guards applied it is green,
and the module suites are clean:
For the zh locale I checked the compiled artifact rather than the exit code:
Test scope: the test exercises the JDBC layer. The
SessionDataIteratorpathshares
IoTDBRpcDataSetand is fixed by the same guards, but I have not coveredit separately.
CI has never run on this branch's head: both
commits/<head>/check-runsandactions/runs?head_sha=<head>return zero. Iwould appreciate the pending runs being approved.