ClickHouse / ClickHouse/clickhouse-java

[jdbc-v2] ResultSet label getters return null/0 for an unknown column label instead of throwing SQLException

Abierto
#3,125 0 comentarios 0 reacciones 1 asignado Reclamado por @chernser Ver en GitHub
area:jdbc-read bug jdbc-v2
Lenguaje dominante
Java
Estrellas
1.6k
Forks
636
Merge medio
2 d 23 h
PR fusionados (30 d)
29

Descripción

## Description

In `jdbc-v2`, most `ResultSet` getters that take a **column label** treat an *unknown* label as a SQL `NULL` value instead of reporting an error: they set `wasNull() == true` and return `null` / `0` / `false`.

JDBC requires an `SQLException` when the label does not identify a column in the result set (`ResultSet.getString(String)`: "throws SQLException - if the columnLabel is not valid"). As it is, a typo in a column name is indistinguishable from a genuine NULL, so an application reads silent zeros/nulls instead of failing.

The getter family is also inconsistent: `getDate`, `getTime`, `getTimestamp`, `getBytes`, `getBinaryStream`, `getObject` and `findColumn` all *do* fail on the same unknown label.

### Steps to reproduce
1. Open a JDBC connection with the `jdbc-v2` driver.
2. `SELECT 'abc' AS txt, 42 AS num, CAST(NULL AS Nullable(Int32)) AS nul`
3. Call the label getters with a label that is not in the result set, e.g. `rs.getInt("no_such_column")`.

### Error Log or Exception StackTrace

```
=== sanity: known labels ===
getString("txt") -> "abc", wasNull=false
getInt("num") -> 42, wasNull=false
getInt("nul") [real SQL NULL] -> 0, wasNull=true

=== unknown label "no_such_column" ===
getString -> NO THROW, value=null, wasNull=true <-- indistinguishable from a real NULL
getBoolean -> NO THROW, value=false, wasNull=true
getByte -> NO THROW, value=0, wasNull=true
getShort -> NO THROW, value=0, wasNull=true
getInt -> NO THROW, value=0, wasNull=true
getLong -> NO THROW, value=0, wasNull=true
getFloat -> NO THROW, value=0.0, wasNull=true
getDouble -> NO THROW, value=0.0, wasNull=true
getBigDecimal -> NO THROW, value=null, wasNull=true

=== same unknown label, getters that DO fail ===
getDate -> NoSuchColumnException: Result has no column with name 'no_such_column'
getTime -> NoSuchColumnException: Result has no column with name 'no_such_column'
getTimestamp -> NoSuchColumnException: Result has no column with name 'no_such_column'
getBytes -> NoSuchColumnException: Result has no column with name 'no_such_column'
getBinaryStream -> NoSuchColumnException: Result has no column with name 'no_such_column'
getTimestamp(label, cal) -> NoSuchColumnException: Result has no column with name 'no_such_column'
getObject -> SQLException: Method: getObject("no_such_column", null) encountered an exception.
findColumn -> SQLException: Method: findColumn("no_such_column") encountered an exception.
```

### Expected Behaviour

Every label getter reports an unknown column label as an `SQLException`. Only a column that exists and holds SQL `NULL` should return `null` / `0` with `wasNull() == true`.

Note that the getters that currently fail do so with `com.clickhouse.client.api.metadata.NoSuchColumnException`, which extends `ClientException` -> `ClickHouseException` -> `RuntimeException`. That is an unchecked exception crossing the JDBC boundary, so it does not satisfy the JDBC contract either, although at least it is not silent.

## Root cause

`jdbc-v2` `ResultSetImpl` label getters guard the read with `reader.hasValue(columnLabel)` (for example `ResultSetImpl.java:314` in `getString(String)`, `:378` in `getInt(String)`, `:394` in `getLong(String)`) and fall into the "no value" branch when it returns `false`:

```java
if (reader.hasValue(columnLabel)) {
wasNull = false;
return reader.getString(columnLabel);
} else {
wasNull = true;
return null;
}
```

`AbstractBinaryFormatReader.hasValue(String)` (`client-v2 .../data_formats/internal/AbstractBinaryFormatReader.java:607`) resolves the label with `TableSchema.findColumnIndex`, which returns `-1` for an unknown column (`client-v2 .../metadata/TableSchema.java:136`); `hasValue(int)` then rejects `-1` as out of range and returns `false`.

So `hasValue(String) == false` means **either** "the column is absent" **or** "the column is present and its value is null", and the JDBC layer maps both onto SQL NULL.

The leniency in `client-v2` is deliberate — it is the behaviour requested in #2755 for the `hasValue` predicate, and it is reasonable for a predicate. The defect is in `jdbc-v2` using that predicate as its label-resolution step, where "absent" must be an error.

The getters that do fail take a different route: they resolve the label through `TableSchema.nameToColumnIndex` (`TableSchema.java:113`), which throws `NoSuchColumnException`. Hence the inconsistency inside the same class.

## Suggested fix

Resolve the label **once** in the label getters and make an unresolvable label an `SQLException`, then look the value up by index. Concretely: a single private helper that resolves label -> 1-based index and throws `SQLException` when the column does not exist, used by all label getters, with the null check done on the resolved index (`hasValue(int)`).

This also makes the getter family consistent and converts the current unchecked `NoSuchColumnException` leaks on the `getDate` / `getTime` / `getTimestamp` / `getBytes` / `getBinaryStream` paths into proper `SQLException`s.

Cases that must keep their current behaviour:
- an existing column whose value is SQL `NULL` still returns `null` / `0` with `wasNull() == true` (verified above with `CAST(NULL AS Nullable(Int32)) AS nul`);
- `hasValue(String)` in `client-v2` keeps returning `false` for a missing column (#2755) — the change belongs in `jdbc-v2`, not in `client-v2`.

Related observation, same code path: a **case-mismatched** label for an existing column (`rs.getString("TXT")` for column `txt`) is also silently read as NULL today, because the lookup is an exact-match map. After this fix it would raise an `SQLException`. Whether `jdbc-v2` should additionally match labels case-insensitively (the JDBC javadoc states column names used as input to getter methods are case insensitive) is a separate decision, and is not covered by this report.

### Code Example

```java
try (Connection conn = DriverManager.getConnection(url, props);
Statement stmt = conn.createStatement();
ResultSet rs = stmt.executeQuery("SELECT 'abc' AS txt, 42 AS num")) {
rs.next();

// Expected: SQLException. Actual: returns 0 and wasNull() == true.
int v = rs.getInt("no_such_column");
boolean wasNull = rs.wasNull();

// Expected: SQLException. Actual: returns null and wasNull() == true.
String s = rs.getString("no_such_column");
}
```

### Configuration

#### Environment
* [ ] Cloud
* clickhouse-java: `main` at `91ec4d326` (VERSION `0.11.0-rc1`), module `jdbc-v2`
* ClickHouse server: 26.8.2.7 (local Docker)
* JDK 17, Linux x86_64

## Notes

Found by automated analysis of `jdbc-v2` while working on the indexed-getter read path (#2516 / PR #3124). It is **not** introduced by that PR — it reproduces on plain `main` at the commit above, and the pre-#3124 code reaches the identical `findColumnIndex` -> `-1` path. Verified by running the getters against a live ClickHouse server, not by inspection alone.

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.