Skip to content

Commit c25a003

Browse files
mkaufmannmkaufmann
committed
refactor: address PR feedback on type refactor
Applies the reviewer's comments on #174 as a layered commit so the incremental diff is preserved. - SimpleResultSet: replace default: throw blocks with HyperTypes.isIntegerLike / isStringLike helpers whose own switches are exhaustive, so a new HyperTypeKind forces a compile-time decision in one place. - HyperTypes.fromJdbcTypeCode: drop default, let fallthrough hit the post-switch throw. - ParameterAccumulator: flatten — single concrete class, drop the interface and DefaultParameterAccumulator. - DataCloudPreparedStatement: construct the ParameterAccumulator internally; drop the constructor parameter. Tests assert against the real accumulator rather than a mock. - setBigDecimal: rewrite the ternary as explicit if/else with a comment explaining the null branch. - ArrowUtils.toArrowByteArray: wrap RootAllocator in try-with-resources. - ColumnMetadata: expand the typeName-override javadoc with a concrete MetadataSchemas example. - HyperTypes.getScale: rewrite the FLOAT comment against the pgjdbc reference (scale == precision: 8 for REAL, 17 for DOUBLE). - QueryMetadataUtil.constructColumnData: forward HyperTypes.getPrecision(hyperType) as COLUMN_SIZE and hyperType.getScale() as DECIMAL_DIGITS for DECIMAL, replacing the hardcoded 255/2 constants. numeric(10,5) now reports scale=5, char(1) reports size=1, etc. - ParameterBinding: switch to @value for equality-based test assertions. - Add HyperTypeArrowRoundtripTest: 19 positive round-trips plus 3 pinned lossy paths (CHAR->VARCHAR unlimited, VARCHAR(n)->unlimited, TIME_TZ->TIME) documenting where Arrow can't preserve Hyper-side distinctions. - Update integration tests: scale/precision cross-checks now assert full Arrow/pg_catalog agreement except for the float + fractional- second cases noted inline.
1 parent 93dee9d commit c25a003

14 files changed

Lines changed: 358 additions & 203 deletions

‎jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/DataCloudConnection.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
import com.salesforce.datacloud.jdbc.protocol.async.core.AsyncStreamObserverIterator;
2323
import com.salesforce.datacloud.jdbc.protocol.async.core.SyncIteratorAdapter;
2424
import com.salesforce.datacloud.jdbc.protocol.data.ColumnMetadata;
25-
import com.salesforce.datacloud.jdbc.protocol.data.DefaultParameterAccumulator;
2625
import com.salesforce.datacloud.jdbc.protocol.grpc.QueryAccessGrpcClient;
2726
import com.salesforce.datacloud.jdbc.util.Deadline;
2827
import com.salesforce.datacloud.jdbc.util.JdbcURL;
@@ -183,7 +182,7 @@ public PreparedStatement prepareStatement(String sql) {
183182
}
184183

185184
private DataCloudPreparedStatement getQueryPreparedStatement(String sql) {
186-
return new DataCloudPreparedStatement(this, sql, new DefaultParameterAccumulator());
185+
return new DataCloudPreparedStatement(this, sql);
187186
}
188187

189188
/**

‎jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/DataCloudPreparedStatement.java‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -53,26 +53,25 @@
5353
@Slf4j
5454
public class DataCloudPreparedStatement extends DataCloudStatement implements PreparedStatement {
5555
private String sql;
56-
private final ParameterAccumulator parameterManager;
56+
// Package-private for tests that need to inspect bound parameters.
57+
final ParameterAccumulator parameters = new ParameterAccumulator();
5758
private final Calendar calendar = Calendar.getInstance(TimeZone.getTimeZone("UTC"));
5859
// True if we are currently fetching metadata from the server, this influences the query param generation
5960
// to not return any data.
6061
private boolean fetchingMetadata = false;
6162

62-
DataCloudPreparedStatement(DataCloudConnection connection, ParameterAccumulator parameterManager) {
63+
DataCloudPreparedStatement(DataCloudConnection connection) {
6364
super(connection);
64-
this.parameterManager = parameterManager;
6565
}
6666

67-
DataCloudPreparedStatement(DataCloudConnection connection, String sql, ParameterAccumulator parameterManager) {
67+
DataCloudPreparedStatement(DataCloudConnection connection, String sql) {
6868
super(connection);
6969
this.sql = sql;
70-
this.parameterManager = parameterManager;
7170
}
7271

7372
private <T> void setParameter(int parameterIndex, HyperType type, T value) throws SQLException {
7473
try {
75-
parameterManager.setParameter(parameterIndex, type, value);
74+
parameters.setParameter(parameterIndex, type, value);
7675
} catch (IllegalArgumentException ex) {
7776
throw new SQLException(ex.getMessage(), ex);
7877
}
@@ -97,7 +96,7 @@ protected QueryParam.Builder getQueryParamBuilder(
9796

9897
final byte[] encodedRow;
9998
try {
100-
encodedRow = toArrowByteArray(parameterManager.getParameters(), calendar);
99+
encodedRow = toArrowByteArray(parameters.getParameters(), calendar);
101100
} catch (IOException e) {
102101
throw new SQLException("Failed to encode parameters on prepared statement", e);
103102
} catch (IllegalArgumentException e) {
@@ -189,7 +188,13 @@ public void setDouble(int parameterIndex, double x) throws SQLException {
189188

190189
@Override
191190
public void setBigDecimal(int parameterIndex, BigDecimal x) throws SQLException {
192-
HyperType type = x != null ? HyperType.decimal(x.precision(), x.scale(), true) : HyperType.decimal(0, 0, true);
191+
final HyperType type;
192+
if (x == null) {
193+
// Precision/scale are unknown; let Arrow encoding pick a suitable default.
194+
type = HyperType.decimal(0, 0, true);
195+
} else {
196+
type = HyperType.decimal(x.precision(), x.scale(), true);
197+
}
193198
setParameter(parameterIndex, type, x);
194199
}
195200

@@ -235,7 +240,7 @@ public void setBinaryStream(int parameterIndex, InputStream x, int length) throw
235240

236241
@Override
237242
public void clearParameters() {
238-
parameterManager.clearParameters();
243+
parameters.clearParameters();
239244
}
240245

241246
@Override

‎jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/QueryMetadataUtil.java‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -186,10 +186,16 @@ private static List<Object> constructColumnData(ResultSet resultSet) throws SQLE
186186
rowData[DATA_TYPE_INDEX] = HyperTypes.toJdbcTypeCode(hyperType);
187187
rowData[TYPE_NAME_INDEX] = HyperTypes.toJdbcTypeName(hyperType);
188188

189-
int columnSize = 255;
189+
// COLUMN_SIZE: per JDBC spec this is the declared precision (digits for numerics,
190+
// characters for strings). HyperTypes.getPrecision already implements this for
191+
// every HyperTypeKind, so we just forward.
192+
int columnSize = HyperTypes.getPrecision(hyperType);
190193
rowData[COLUMN_SIZE_INDEX] = columnSize;
191194

192-
rowData[DECIMAL_DIGITS_INDEX] = HyperTypes.needsDecimalDigits(hyperType) ? 2 : 0;
195+
// DECIMAL_DIGITS: only meaningful for fixed-scale decimals; for those the scale
196+
// comes from the HyperType that PgCatalogTypeParser extracted from
197+
// format_type(atttypmod) (e.g. "numeric(10,5)" → scale=5).
198+
rowData[DECIMAL_DIGITS_INDEX] = HyperTypes.needsDecimalDigits(hyperType) ? hyperType.getScale() : 0;
193199
rowData[NUM_PREC_RADIX_INDEX] = 10;
194200
rowData[NULLABLE_INDEX] = notNull ? DatabaseMetaData.columnNoNulls : DatabaseMetaData.columnNullable;
195201
rowData[DESCRIPTION_INDEX] = resultSet.getString("description");

‎jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/resultset/SimpleResultSet.java‎

Lines changed: 28 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
package com.salesforce.datacloud.jdbc.core.resultset;
66

77
import com.salesforce.datacloud.jdbc.core.metadata.DataCloudResultSetMetaData;
8+
import com.salesforce.datacloud.jdbc.core.types.HyperTypes;
9+
import com.salesforce.datacloud.jdbc.protocol.data.HyperTypeKind;
810
import java.io.InputStream;
911
import java.io.Reader;
1012
import java.math.BigDecimal;
@@ -168,20 +170,13 @@ public int getInt(int columnIndex) throws SQLException {
168170

169171
@Override
170172
public long getLong(int columnIndex) throws SQLException {
171-
switch (metadata.getColumn(columnIndex).getType().getKind()) {
172-
case INT8:
173-
case INT16:
174-
case INT32:
175-
case INT64:
176-
case OID: {
177-
OptionalLong v = getAccessor(columnIndex).getAnyInteger(getSubclass());
178-
wasNull = !v.isPresent();
179-
return v.orElse(0L);
180-
}
181-
default:
182-
throw new SQLException("Unsupported column type for integer-like types: "
183-
+ metadata.getColumn(columnIndex).getType().toString());
173+
val type = metadata.getColumn(columnIndex).getType();
174+
if (!HyperTypes.isIntegerLike(type)) {
175+
throw new SQLException("Unsupported column type for integer-like types: " + type);
184176
}
177+
OptionalLong v = getAccessor(columnIndex).getAnyInteger(getSubclass());
178+
wasNull = !v.isPresent();
179+
return v.orElse(0L);
185180
}
186181

187182
private static final double LONG_MAX_DOUBLE = StrictMath.nextDown((double) Long.MAX_VALUE);
@@ -202,40 +197,26 @@ public float getFloat(int columnIndex) throws SQLException {
202197

203198
@Override
204199
public double getDouble(int columnIndex) throws SQLException {
205-
switch (metadata.getColumn(columnIndex).getType().getKind()) {
206-
case INT8:
207-
case INT16:
208-
case INT32:
209-
case INT64:
210-
case OID: {
211-
OptionalLong v = getAccessor(columnIndex).getAnyInteger(getSubclass());
212-
wasNull = !v.isPresent();
213-
return v.orElse(0L);
214-
}
215-
default:
216-
throw new SQLException("Unsupported column type for floating-point types: "
217-
+ metadata.getColumn(columnIndex).getType().toString());
200+
val type = metadata.getColumn(columnIndex).getType();
201+
if (!HyperTypes.isIntegerLike(type)) {
202+
throw new SQLException("Unsupported column type for floating-point types: " + type);
218203
}
204+
OptionalLong v = getAccessor(columnIndex).getAnyInteger(getSubclass());
205+
wasNull = !v.isPresent();
206+
return v.orElse(0L);
219207
}
220208

221209
@Override
222210
public BigDecimal getBigDecimal(int columnIndex) throws SQLException {
223-
switch (metadata.getColumn(columnIndex).getType().getKind()) {
224-
case INT8:
225-
case INT16:
226-
case INT32:
227-
case INT64:
228-
case OID: {
229-
OptionalLong v = getAccessor(columnIndex).getAnyInteger(getSubclass());
230-
wasNull = !v.isPresent();
231-
return v.isPresent() ? new BigDecimal(v.getAsLong()) : null;
232-
}
211+
val type = metadata.getColumn(columnIndex).getType();
212+
if (!HyperTypes.isIntegerLike(type)) {
233213
// TODO: apparently, PostgreSQL does not support float/decimal conversion. Double-check this with test
234214
// cases.
235-
default:
236-
throw new SQLException("Unsupported column type for numeric types: "
237-
+ metadata.getColumn(columnIndex).getType().toString());
215+
throw new SQLException("Unsupported column type for numeric types: " + type);
238216
}
217+
OptionalLong v = getAccessor(columnIndex).getAnyInteger(getSubclass());
218+
wasNull = !v.isPresent();
219+
return v.isPresent() ? new BigDecimal(v.getAsLong()) : null;
239220
}
240221

241222
@Override
@@ -291,21 +272,15 @@ public Array getArray(int columnIndex) throws SQLException {
291272

292273
@Override
293274
public Object getObject(int columnIndex) throws SQLException {
294-
switch (metadata.getColumn(columnIndex).getType().getKind()) {
295-
case INT32: {
296-
val v = getInt(columnIndex);
297-
if (wasNull) {
298-
return null;
299-
}
300-
return v;
301-
}
302-
case CHAR:
303-
case VARCHAR:
304-
return getString(columnIndex);
305-
default:
306-
throw new SQLException("Unsupported column type in `getObject`: "
307-
+ metadata.getColumn(columnIndex).getType().toString());
275+
val type = metadata.getColumn(columnIndex).getType();
276+
if (type.getKind() == HyperTypeKind.INT32) {
277+
val v = getInt(columnIndex);
278+
return wasNull ? null : v;
308279
}
280+
if (HyperTypes.isStringLike(type)) {
281+
return getString(columnIndex);
282+
}
283+
throw new SQLException("Unsupported column type in `getObject`: " + type);
309284
}
310285

311286
@Override

‎jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/core/types/HyperTypes.java‎

Lines changed: 79 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -151,9 +151,8 @@ public static HyperType fromJdbcTypeCode(int jdbcTypeCode, boolean nullable) {
151151
return HyperType.array(HyperType.varcharUnlimited(nullable), nullable);
152152
case Types.NULL:
153153
return HyperType.nullType();
154-
default:
155-
throw new IllegalArgumentException("Unsupported JDBC type code: " + jdbcTypeCode);
156154
}
155+
throw new IllegalArgumentException("Unsupported JDBC type code: " + jdbcTypeCode);
157156
}
158157

159158
/**
@@ -301,7 +300,9 @@ public static int getScale(HyperType t) {
301300
return UNKNOWN_SCALE;
302301
case FLOAT4:
303302
case FLOAT8:
304-
// Matches legacy behaviour: floating-point scale equals precision.
303+
// Match Postgres JDBC: for binary floats it reports scale == precision (8 for
304+
// REAL, 17 for DOUBLE). Binary floats have no decimal scale, so this is a
305+
// display convention rather than a meaningful fractional-digit count.
305306
return getPrecision(t);
306307
case DECIMAL:
307308
return t.getScale();
@@ -414,6 +415,81 @@ public static boolean needsCharOctetLength(HyperType t) {
414415
}
415416
}
416417

418+
/**
419+
* {@code true} when the value is representable as a Java {@code long} (any-signed integer
420+
* kind, including Hyper's {@code OID}). Used by {@code ResultSet.getLong / getInt /
421+
* getDouble / getBigDecimal} to decide whether the accessor's integer path applies.
422+
*
423+
* <p>The switch is exhaustive on purpose: adding a new {@link HyperTypeKind} should prompt
424+
* the author to decide whether it belongs in this family, rather than silently defaulting
425+
* to {@code false}.
426+
*/
427+
public static boolean isIntegerLike(HyperType t) {
428+
switch (t.getKind()) {
429+
case INT8:
430+
case INT16:
431+
case INT32:
432+
case INT64:
433+
case OID:
434+
return true;
435+
case BOOL:
436+
case FLOAT4:
437+
case FLOAT8:
438+
case DECIMAL:
439+
case CHAR:
440+
case VARCHAR:
441+
case BINARY:
442+
case VARBINARY:
443+
case DATE:
444+
case TIME:
445+
case TIME_TZ:
446+
case TIMESTAMP:
447+
case TIMESTAMP_TZ:
448+
case ARRAY:
449+
case NULL:
450+
case INTERVAL:
451+
case JSON:
452+
case UNKNOWN:
453+
return false;
454+
}
455+
throw new IllegalStateException("Unhandled HyperTypeKind: " + t.getKind());
456+
}
457+
458+
/**
459+
* {@code true} when the value is representable as a Java {@code String} through the JDBC
460+
* {@code getString} / {@code getObject} text paths.
461+
*/
462+
public static boolean isStringLike(HyperType t) {
463+
switch (t.getKind()) {
464+
case CHAR:
465+
case VARCHAR:
466+
return true;
467+
case BOOL:
468+
case INT8:
469+
case INT16:
470+
case INT32:
471+
case INT64:
472+
case OID:
473+
case FLOAT4:
474+
case FLOAT8:
475+
case DECIMAL:
476+
case BINARY:
477+
case VARBINARY:
478+
case DATE:
479+
case TIME:
480+
case TIME_TZ:
481+
case TIMESTAMP:
482+
case TIMESTAMP_TZ:
483+
case ARRAY:
484+
case NULL:
485+
case INTERVAL:
486+
case JSON:
487+
case UNKNOWN:
488+
return false;
489+
}
490+
throw new IllegalStateException("Unhandled HyperTypeKind: " + t.getKind());
491+
}
492+
417493
// ----------------------------------------------------------------------
418494
// getTypeInfo rows
419495
// ----------------------------------------------------------------------

‎jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/protocol/data/ArrowUtils.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,10 +78,10 @@ private static HyperType materializeDecimal(ParameterBinding parameterBinding) {
7878
}
7979

8080
public static byte[] toArrowByteArray(List<ParameterBinding> parameters, Calendar calendar) throws IOException {
81-
RootAllocator allocator = new RootAllocator(Long.MAX_VALUE);
8281
Schema schema = ArrowUtils.createSchemaFromParameters(parameters);
8382

84-
try (VectorSchemaRoot root = VectorSchemaRoot.create(schema, allocator)) {
83+
try (RootAllocator allocator = new RootAllocator(Long.MAX_VALUE);
84+
VectorSchemaRoot root = VectorSchemaRoot.create(schema, allocator)) {
8585
root.allocateNew();
8686
VectorPopulator.populateVectors(root, parameters, calendar);
8787

‎jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/protocol/data/ColumnMetadata.java‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,16 @@ public class ColumnMetadata {
1919
/**
2020
* Optional override for {@code ResultSetMetaData.getColumnTypeName}; {@code null} means the
2121
* JDBC layer should use the default derived from {@link #type}. Used for metadata result sets
22-
* (e.g. {@code DatabaseMetaData.getTables}) where the JDBC spec pins a specific column-type
23-
* label that differs from the underlying Hyper type.
22+
* where the JDBC spec pins a specific column-type label that differs from the underlying
23+
* Hyper type.
24+
*
25+
* <p>Example: the columns of {@link java.sql.DatabaseMetaData#getTables} are spec'd as
26+
* {@code TEXT} and {@code SHORT} (see JDBC 4.2 §28.12), not {@code VARCHAR} / {@code SMALLINT}.
27+
* {@link com.salesforce.datacloud.jdbc.core.MetadataSchemas} constructs those columns as
28+
* {@code new ColumnMetadata("TABLE_NAME", HyperType.varcharUnlimited(true), "TEXT")} so that
29+
* {@code ResultSetMetaData.getColumnTypeName} returns {@code "TEXT"} while the accessor
30+
* machinery still sees a {@code VARCHAR}. Regular query result columns pass {@code null}
31+
* (via the two-arg constructor) and get {@code "VARCHAR"} from {@link HyperType}.
2432
*/
2533
String typeName;
2634

‎jdbc-core/src/main/java/com/salesforce/datacloud/jdbc/protocol/data/DefaultParameterAccumulator.java‎

Lines changed: 0 additions & 33 deletions
This file was deleted.

0 commit comments

Comments
 (0)