Skip to content

Fix Java client time column access throwing ArrayIndexOutOfBoundsException (#17407) - #17408

Open
PDGGK wants to merge 1 commit into
apache:masterfrom
PDGGK:fix/java-client-time-column-guard
Open

Fix Java client time column access throwing ArrayIndexOutOfBoundsException (#17407)#17408
PDGGK wants to merge 1 commit into
apache:masterfrom
PDGGK:fix/java-client-time-column-guard

Conversation

@PDGGK

@PDGGK PDGGK commented Mar 31, 2026

Copy link
Copy Markdown
Contributor

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, getString and getObject handle that;
getBoolean, getInt, getFloat, getDouble and getBinary do not — they
fall straight through to TsBlock.getColumn(tsBlockColumnIndex) with -1.

isNull(index, rowNum) is index >= 0 && ..., so it returns false for the
time column and execution enters the value branch rather than the null branch.

Why this is a defect rather than a quirk

java.sql.ResultSet.getInt and its siblings declare throws SQLException. What
comes out here instead is an unchecked ArrayIndexOutOfBoundsException, and
IoTDBJDBCResultSet only catches StatementExecutionException:

public int getInt(int columnIndex) throws SQLException {
  try {
    return ioTDBRpcDataSet.getInt(columnIndex);
  } catch (StatementExecutionException e) {
    throw new SQLException(e.getMessage());
  }
}

So the exception crosses the JDBC boundary uncaught. A caller writing the
documented catch (SQLException) cannot handle it. That is the argument for the
change: it is a JDBC contract violation, independent of any judgement about which
exception type is nicer.

The Session DataIterator getters share IoTDBRpcDataSet and pass it through
unwrapped 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() and
before isNull(...), and the five messages character-for-character identical to
the ones that merged there.

client-cpp  throw IoTDBException("Cannot read int32 from time column");
this PR     CANNOT_READ_INT32_FROM_TIME_COLUMN = "Cannot read int32 from time column";

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:

A fast-fail exception … is much safer, and it aligns perfectly with our Java
client which typically throws IndexOutOfBoundsException or
ClassCastException for mismatched types on the Time column.

That describes the behaviour this PR changes, and it describes it accurately —
ArrayIndexOutOfBoundsException is an IndexOutOfBoundsException. So #17400
cited 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.

getBinary is in the guarded set because it is in the merged #17400 set: that
review named four getters, and the patch that was approved and merged carries
five.

Changes

File
IoTDBRpcDataSet.java five tsBlockColumnIndex < 0 guards throwing StatementExecutionException
i18n/en/RpcMessages.java five message constants
i18n/zh/RpcMessages.java the same five, translated
IoTDBJDBCResultSetTest.java regression test

The 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

getDate delegates to getIntByTsBlockColumnIndex, so it now rejects the time
column too. Nobody has decided that — #17400 concerned the C++ client and did not
discuss getDate. It follows from guarding getInt, and reading a LocalDate
out 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:

Tests run: 2, Failures: 1
java.lang.AssertionError: expected a SQLException but the time column leaked an
  unchecked java.lang.ArrayIndexOutOfBoundsException: Index -1 out of bounds for length 3

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:

iotdb-client/jdbc + iotdb-client/service-rpc     114 tests, 0 failures
mvn -P with-zh-locale test-compile               BUILD SUCCESS

For the zh locale I checked the compiled artifact rather than the exit code:

javap -p -constants org.apache.iotdb.rpc.i18n.RpcMessages
  CANNOT_READ_INT32_FROM_TIME_COLUMN = "无法从时间列读取 int32 值"

Test scope: the test exercises the JDBC layer. The Session DataIterator path
shares IoTDBRpcDataSet and is fixed by the same guards, but I have not covered
it separately.

CI has never run on this branch's head: both
commits/<head>/check-runs and actions/runs?head_sha=<head> return zero. I
would appreciate the pending runs being approved.

@PDGGK
PDGGK force-pushed the fix/java-client-time-column-guard branch from 4cbf842 to 5d080cb Compare March 31, 2026 11:49
@HTHou

HTHou commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

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

@PDGGK
PDGGK force-pushed the fix/java-client-time-column-guard branch from 5d080cb to 3fcfd28 Compare June 25, 2026 17:28
@PDGGK

PDGGK commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the suggestion, @HTHou. Separately, I noticed CI was red and it was my change that caused it: making getObject reject the time pseudo-column was too aggressive. getObject on the time column is a valid, already-tested path (IoTDBJDBCResultSetTest#testQuery) and should keep returning the timestamp.

I've restored the time-column branch in the TIMESTAMP case of getObjectByTsBlockIndex and kept the new tsBlockColumnIndex < 0 guards only on the strictly-typed getters (boolean/int32/float/double/binary); getLong, getString and getObject continue to read the time column. IoTDBJDBCResultSetTest passes locally now.

@PDGGK

PDGGK commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

@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 (75eaa40, 2026-07-31) and nothing else has fixed it in the meantime. In IoTDBRpcDataSet, getLongByTsBlockColumnIndex handles the time pseudo-column (:424) and getStringByTsBlockColumnIndex does too (:524), but the five strictly-typed getters — getBooleanByTsBlockColumnIndex (:333), getDoubleByTsBlockColumnIndex (:353), getFloatByTsBlockColumnIndex (:373), getIntByTsBlockColumnIndex (:393), getBinaryTsBlockColumnIndex (:451) — go straight to curTsBlock.getColumn(tsBlockColumnIndex). Since isNull(index, rowNum) is index >= 0 && ... (:322), it returns false for the time column and execution enters the value branch with index -1. getDate inherits it as well, because getDateByTsBlockColumnIndex delegates to getInt (:599) — six user-visible surfaces, not five. The resulting ArrayIndexOutOfBoundsException is unchecked and IoTDBJDBCResultSet only catches StatementExecutionException (e.g. getBoolean at :349-355), so it escapes through the JDBC API instead of arriving as a SQLException. The Session DataIterator getters pass it through unwrapped too.

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 int32/float/double/Binary, and the throw-everywhere behaviour was imposed in review — @jt2594838 objected to casting on overflow grounds and @CritasWang asked for IoTDBException on the numeric getters. This PR is the Java counterpart of that outcome, and I should have said so in the description rather than leaving it to be inferred.

What's wrong with the PR as it stands, all on me:

  • The five messages are bare string literals. That predates the compile-time i18n work (feat: add compile-time i18n for log and exception messages #17613, Complete zh locale i18n: convert leftover literals, review translations, add zh-compile CI #18110); this class now routes every exception message through RpcMessages, so these need constants added under both the en and zh roots in the existing // IoTDBRpcDataSet / IoTDBJDBCDataSet section.
  • No test. The C++ twin shipped one, and @jt2594838 asked for it explicitly in that review. IoTDBJDBCResultSetTest already builds tree-model TsBlocks in-process, so a unit test asserting that getInt(1)/getFloat(1)/getBoolean(1) surface as SQLException while getLong(1)/getString(1)/getObject(1) still return the timestamp is straightforward — no IT needed.
  • CI has never run on the current head: zero workflow runs on that sha, and the last visible signal on this PR is a failed run from 2026-03-31, on a matrix that no longer matches the current one. A rebase and repush should at least create the runs, though they'll sit in action_required until someone with write access approves them.

Plan: rebase onto master, move the messages into RpcMessages (en + zh), add the unit test, verify mvn test-compile -P with-zh-locale locally, repush, and rewrite the description to lead with the #17400 review outcome. On the semantics I don't think a list thread is needed: #17400 already settled it — @jt2594838 raised the overflow objection against casting and @CritasWang asked for an IoTDBException, so I'll mirror that and ask both of you to confirm on the PR rather than re-open a decided question on the list. I'd also appreciate the pending CI runs being approved when you get a chance, since none have run on the current head.


…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>
@PDGGK
PDGGK force-pushed the fix/java-client-time-column-guard branch from 3fcfd28 to 5f21e6b Compare September 5, 2026 09:37
@PDGGK

PDGGK commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Pushed the rework I described on 2026-08-02, rebuilt on current master
(219332dacb8) rather than rebased — the old head no longer shared a merge base
with master. The description is rewritten. In short: the five messages now go
through RpcMessages in both en and zh instead of being string literals,
there is a regression test that fails on unpatched master with expected a SQLException but the time column leaked an unchecked java.lang.ArrayIndexOutOfBoundsException: Index -1 out of bounds for length 3,
and the jdbc and service-rpc suites plus -P with-zh-locale are green.

One correction to what I wrote in August. I said then that #17400 had already
settled the semantics and that I would simply mirror it. Re-reading that thread,
that is not what it says. @CritasWang's comment there was:

A fast-fail exception … is much safer, and it aligns perfectly with our Java
client which typically throws IndexOutOfBoundsException or
ClassCastException for mismatched types on the Time column.

That cites the current Java behaviour as the model for the C++ change — and it
describes it accurately, since ArrayIndexOutOfBoundsException is an
IndexOutOfBoundsException. So #17400 settled the C++ side while treating Java's
behaviour as fine. Citing it as authority for changing Java gets the reference
backwards, and I should not have written that.

The argument I would actually make is narrower and does not depend on #17400:
java.sql.ResultSet.getInt declares throws SQLException, IoTDBJDBCResultSet
only catches StatementExecutionException, so the unchecked exception crosses
the JDBC boundary and a caller writing catch (SQLException) cannot handle it.
#17400 is then cross-client consistency rather than precedent — same five
getters, same guard position, and the five message texts verbatim identical to
the ones merged there. That reframing is a new argument rather than a settled
conclusion, so it is worth an objection if you disagree with it.

@HTHou — you suggested the dev list back in April as a way to get more review
attention, and I answered as though you had asked whether the design was settled.
That was the wrong answer to the question you actually asked, and it is the
reason I did not follow your suggestion. Since then this branch has had no review
and no CI at all, so you were right on your own terms. I will post a note to
dev@ rather than ask you again.

The push created twelve workflow runs and they are all sitting at
conclusion=action_required. I would appreciate them being approved when someone
has a moment — there has still been no CI signal on this branch.

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