PDGGK commented on PR #17408: URL: https://github.com/apache/iotdb/pull/17408#issuecomment-5156108903
@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. `ge tBoolean` 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 (#17613, #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. --- -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
