Repository navigation
[JDBC] Fix table-model driver defects that break generic JDBC clients - #18962
JackieTien97 wants to merge 5 commits into
Conversation
…ject(Class) IoTDBRelationalDatabaseMetadata.getTables / getColumns: - Honor tableNamePattern, columnNamePattern and types. A null pattern no longer turns into LIKE 'null', and quotes in patterns are escaped. - Report the real database and table names in TABLE_SCHEM / TABLE_NAME instead of the (escaped) search patterns; add TABLE_CAT to getTables and report it as NULL in both, since the table model has no catalogs. - Return an empty result set instead of null, and never send an empty TsBlock, which the result set read as one bogus row. - Derive NULLABLE / IS_NULLABLE from the TIME category instead of the column name, and number ORDINAL_POSITION within each table. - Apply the same patterns in the SHOW / DESC fallback used when information_schema cannot be queried, and fix removeEscape, which never removed an escape. IoTDBTablePreparedStatement: - Record the query id of prepared executions so that cancel() reaches the server, and reset the cancelled state for every execution. - Send the statement fetch size (capped by maxRows) with TSExecutePreparedReq. - Convert setTimestamp values to the connection's time precision (ms / us / ns), rejecting values the precision cannot represent. IoTDBJDBCResultSet: - Implement getObject(int, Class) for String, Boolean, Integer, Long, Float, Double, byte[], Timestamp and LocalDate.
…abel - getPrimaryKeys reports the real database and table names with a NULL TABLE_CAT, adds no filter for a null schema or table, escapes quotes, matches the given names exactly whether or not they are escaped, numbers KEY_SEQ within each table and returns an empty result set when nothing matches. It now shares the column listing (and SHOW / DESC fallback) with getColumns. - getTimestamp returns null for SQL NULL instead of 1970-01-01, and the by-label variant now honors the time precision and reports an unknown label as a SQLException. - Implement getObject(String, Class) on top of getObject(int, Class).
…ide SQL binding IoTDBConnection: - setReadOnly records the hint instead of throwing, and isReadOnly reports it. Trino calls setReadOnly(true) on every read connection. - New connections are in auto-commit mode, as JDBC requires; IoTDB has no transactions, so commit / rollback stay no-ops. - getCatalog returns null in the table dialect, matching the NULL TABLE_CAT of its DatabaseMetaData, and setCatalog(null) is ignored instead of failing with an NPE. The tree dialect keeps "Apache IoTDB", which its DatabaseMetaData compares against, now null-safely. IoTDBTablePreparedStatement: - setObject (all overloads) accepts java.time.LocalDate and binds it like setDate. - SQL completed on the client is split at '?' the way the table-model lexer tokenizes: string literals, double-quoted and back-quoted identifiers and comments hide '?', quotes are escaped only by doubling, and a backslash is an ordinary character. - NaN is written as the string 'NaN', which the server stores as NaN in FLOAT / DOUBLE columns. Infinity has no accepted SQL form (insert and CAST both reject it), so binding it there fails with a clear SQLException; server-prepared queries still send it in binary. Metadata: - Report OBJECT columns with type name OBJECT and java.sql.Types.OTHER in ResultSetMetaData (class String, as getObject describes the value) and in DatabaseMetaData.getColumns.
…l-metadata-cancel
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
These three reproducible issues should be addressed before merging. This review is submitted as COMMENT because GitHub does not allow a pull request's author to submit REQUEST_CHANGES on their own pull request.
The findings cover a regression when reading pre-epoch timestamps by label, missed parameter substitution after a carriage-return-terminated line comment, and incorrect rejection of valid timestamps near the nanosecond lower bound.
Validation: a clean JDBC reactor build passed all 121 existing tests. Three additional focused tests reproduced these issues on a64c948; the first two tests also passed against the PR base, 4e05c9f. These tests exercise the driver with mocked RPC responses; no live cluster was used.
| public Timestamp getTimestamp(String columnName) throws SQLException { | ||
| return new Timestamp(getLong(columnName)); | ||
| try { | ||
| return ioTDBRpcDataSet.getTimestamp(columnName); |
There was a problem hiding this comment.
[P2] Preserve pre-epoch timestamps when reading by label
With millisecond precision and a timestamp column containing -1500, getTimestamp("ts") now throws SQLException: nanos > 999999999 or < 0; the same test passes on the PR base and returns new Timestamp(-1500L). This new path delegates to RpcUtils.convertToTimestamp(), whose ordinary division/remainder produces a negative nanos component for negative values that are not whole seconds. Please fix the shared conversion to use floor-based seconds and a nonnegative fractional component, and cover negative timestamps, before routing this getter through it.
| } else if (sql.startsWith("--", i)) { | ||
| int end = sql.indexOf('\n', i); | ||
| i = end < 0 ? sql.length() : end + 1; |
There was a problem hiding this comment.
[P2] Recognize carriage returns as line-comment terminators
The table-model lexer's SIMPLE_COMMENT rule ends a comment at either \r or \n, but this scanner only searches for \n. For the Java string "INSERT -- comment\rINTO t(time, s1) VALUES(?, ?)\n", binding both parameters and calling executeUpdate() still sends the two ? markers unchanged because the scanner treats the remainder of the INSERT as a comment. This substitution test passes on the PR base and fails here. Please recognize lone CR as well as LF and CRLF so the client and server agree on where parameter markers are visible.
| return Math.addExact( | ||
| Math.multiplyExact(seconds, timeFactor), x.getNanos() / (1_000_000_000 / timeFactor)); |
There was a problem hiding this comment.
[P3] Avoid intermediate underflow for valid nanosecond timestamps
With nanosecond precision, setTimestamp(1, Timestamp.from(Instant.parse("1677-09-21T00:12:43.145224193Z"))) throws the new out-of-range exception, although the correct value is Long.MIN_VALUE + 1 and fits in a long. Flooring to whole seconds makes seconds * timeFactor underflow before the positive fractional component can bring the final result back into range. Please rearrange the negative-value arithmetic or check the complete converted value without overflowing the intermediate product, and add a lower-bound test alongside the existing far-future overflow test.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18962 +/- ##
============================================
+ Coverage 47.09% 47.69% +0.60%
- Complexity 737 806 +69
============================================
Files 5539 5561 +22
Lines 405217 408215 +2998
Branches 53029 53585 +556
============================================
+ Hits 190854 194716 +3862
+ Misses 214363 213499 -864 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ower bound - RpcUtils.getMilliSecond / getNanoSecond floor the seconds, so that a pre-epoch time keeps a nanos part in [0, 1e9). Reading a value such as -1500 ms through getTimestamp (by index or label) or getObject(Timestamp) failed with "nanos > 999999999 or < 0". - The client-side SQL scanner ends a line comment at CR as well as LF, as the lexer's SIMPLE_COMMENT rule does, so the parameter markers after a CR-terminated comment are substituted again. - setTimestamp borrows a second for a negative value with a fraction, as Instant.toEpochMilli() does, so nanosecond values down to Long.MIN_VALUE no longer overflow in the intermediate product.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| List<TableInfo> tables = new ArrayList<>(); | ||
| for (String database : databases) { | ||
| try (ResultSet rs = | ||
| stmt.executeQuery("show tables details from " + quoteIdentifier(database))) { |
| + "." | ||
| + quoteIdentifier(table.tableName()) | ||
| + " details"; | ||
| try (ResultSet rs = stmt.executeQuery(sql)) { |
2 participants
Summary
Fixes in the JDBC driver for the table model (
sql_dialect=table) that break generic JDBC clients such as Trino'sBaseJdbcClientand DBeaver. This is a driver-only change: the server is untouched.DatabaseMetaData (
IoTDBRelationalDatabaseMetadata)getTables/getColumns/getPrimaryKeysreport the real database and table names inTABLE_SCHEM/TABLE_NAMEinstead of echoing the (escaped) search patterns, and reportTABLE_CATas NULL (getTableshad noTABLE_CATcolumn at all).tableNamePattern,columnNamePatternandtypesare honored. A null pattern no longer becomesLIKE 'null', and quotes in patterns are escaped.null, and no empty TsBlock is sent any more (it was read back as one bogus row).NULLABLE/IS_NULLABLEfollow the TIME category instead of the column name, so a custom-named time column is reported correctly.ORDINAL_POSITIONandKEY_SEQare numbered within each table.getPrimaryKeysmatches the given names exactly, whether or not the caller escaped them.information_schemacannot be queried, applies the same patterns.removeEscapepreviously never removed an escape.java.sql.Types.OTHER.Statements and result sets
cancel()reaches the server, and they send the statementfetchSize.setTimestampconverts values to the server time precision (ms / us / ns).setObject(all overloads) acceptsjava.time.LocalDate.?the way the table-model lexer tokenizes it: string literals, quoted identifiers and comments hide?, and a backslash is an ordinary character.NaNis written as'NaN', which the server stores as NaN. Infinity has no accepted SQL form, so binding it in client-completed SQL fails with a clearSQLException. Server-prepared queries still send it in binary.getObject(int, Class)andgetObject(String, Class)support String, Boolean, Integer, Long, Float, Double, byte[], Timestamp and LocalDate.getTimestampreturns null for SQL NULL, and the by-label variant honors the time precision.ResultSetMetaDatadescribes OBJECT columns (OBJECT,Types.OTHER).Connection
setReadOnly(true)is accepted as a hint. Trino calls it on every read connection.getCatalog()returns null in the table dialect, andsetCatalog(null)is ignored. The tree dialect keepsApache IoTDB.Known limitation (needs server work)
Statement.cancel()still cannot stop a query.ClientRPCServiceImpl#cancelOperationis unimplemented and always returnsQUERY_NOT_ALLOWED, and the synchronized RPC client makes a cancel from another thread wait for the in-flight execute RPC. Now that prepared executions record their query id,cancel()on a prepared statement reaches the server and fails with aSQLException, as it already did for plain statements.Tests
mvn test -pl iotdb-client/jdbc: all 121 tests pass, both in the default build and with-P with-zh-locale. 47 of them are new.LocalDatebinding, NaN/Infinity handling and client-side?splitting against an IoTDB 2.0.10 server.🤖 Generated with Claude Code