refactor: round 2 of PR feedback — fix CHAR/VARCHAR roundtrip, docs

Addresses the second round of review comments on #174.

- HyperTypeToArrow.toFieldType now stamps Hyper-compatible field
  metadata for string kinds: hyper:type=Char1 for CHAR(1),
  hyper:type=Char + hyper:max_string_length for CHAR(n), and
  hyper:max_string_length alone for bounded VARCHAR(n). The decode
  path in ArrowToHyperTypeMapper already reads these keys (it was
  written for Hyper's inbound Arrow stream), so CHAR(1), CHAR(n) and
  VARCHAR(n) now round-trip through Arrow without loss. Unbounded
  VARCHAR stays metadata-free.

- HyperTypeArrowRoundtripTest expanded: CHAR(1), CHAR(16),
  VARCHAR(255) and VARCHAR(1024) are now positive parameterised
  cases. TIME_TZ stays explicitly lossy — Arrow Time has no timezone
  slot, time-with-time-zone stores a per-value offset that cannot
  ride on per-column metadata, and the driver's read-side accessor
  factory also drops TZ on ingress. Comment rewritten to document
  why fixing it needs either a synthetic Arrow schema or Hyper
  protocol coordination, rather than a metadata stamp.

- HyperTypes.toJdbcType: inline comment on the OID branch explaining
  why it surfaces as BIGINT (pgjdbc parity; preserves getLong() on
  OID columns; internal HyperType.oid keeps precision accurate).

- SimpleResultSet: class-level javadoc now calls out that this
  implementation deliberately only covers the JDBC-metadata type
  slice (INT32 for columns like DATA_TYPE, CHAR/VARCHAR, integer
  family via isIntegerLike) and is planned for removal once
  DataCloudMetadataResultSet migrates to StreamingResultSet.

Also worth flagging as an observable behavior alongside the ones
listed in the first commit: DataCloudDatabaseMetadata.getTypeInfo()
returns a real 18-column result derived from HyperTypeKind (was
empty before). Mentioning here since the reviewer asked for it in
the "Observable behaviour changes" section and I'm avoiding
amending the first commit on a published PR.

Author: mkaufmann

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

@@ -39,6 +39,16 @@
  *
  * Access to SQL values is provided via {@link ColumnAccessor} instances. This class
  * already takes care of casting from SQL types to the compatible Java types.
+ *
+ * <p><b>Planned for removal.</b> This implementation only covers the narrow slice of
+ * {@link HyperTypeKind} used by JDBC metadata result sets (INT32 for columns like
+ * {@code DATA_TYPE}, CHAR/VARCHAR for text columns, and INT8-INT64/OID via the
+ * {@code isIntegerLike} helper) — not the full query-result type universe. Rather
+ * than expanding the {@link #getLong}, {@link #getDouble}, {@link #getBigDecimal},
+ * {@link #getObject} switches to cover every kind, we intend to migrate
+ * {@code DataCloudMetadataResultSet} to build on {@link StreamingResultSet} so there
+ * is only one result-set implementation in the driver. Treat this class as
+ * maintenance-only until that refactor lands.
  */
 @AllArgsConstructor
 public abstract class SimpleResultSet<SELF>

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

@@ -54,7 +54,13 @@ public static JDBCType toJdbcType(HyperType t) {
             case INT32:
                 return JDBCType.INTEGER;
             case INT64:
+                return JDBCType.BIGINT;
             case OID:
+                // Hyper's OID is an unsigned 32-bit internal type. We surface it as BIGINT to
+                // match the pgjdbc reference (see JDBCReferenceTest): pgjdbc reports oid with
+                // Types.BIGINT, which keeps `getLong("oid_col")` working for integer-treating
+                // callers. HyperType.oid stays a distinct kind so the pg_catalog path can keep
+                // precision/display accurate (10 digits for the u32 range).
                 return JDBCType.BIGINT;
             case FLOAT4:
                 return JDBCType.REAL;

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

@@ -5,6 +5,8 @@
 package com.salesforce.datacloud.jdbc.protocol.data;
 
 import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
 import org.apache.arrow.vector.types.DateUnit;
 import org.apache.arrow.vector.types.FloatingPointPrecision;
 import org.apache.arrow.vector.types.TimeUnit;
@@ -35,10 +37,46 @@ public static Field toField(String name, HyperType type) {
         return new Field(name, toFieldType(type), null);
     }
 
-    /** Map a {@link HyperType} to an Arrow {@link FieldType}. */
+    /**
+     * Map a {@link HyperType} to an Arrow {@link FieldType}.
+     *
+     * <p>For string kinds Arrow only has the unparameterised {@code Utf8}, so we stamp the
+     * Hyper-side length on the field as metadata using the same keys Hyper's own server emits
+     * ({@code hyper:type} and {@code hyper:max_string_length}). {@link ArrowToHyperTypeMapper}
+     * reads those keys back, letting CHAR(n) / CHAR(1) / VARCHAR(n) round-trip through Arrow
+     * without loss.
+     */
     public static FieldType toFieldType(HyperType type) {
         ArrowType arrowType = toArrowType(type);
-        return type.isNullable() ? FieldType.nullable(arrowType) : FieldType.notNullable(arrowType);
+        Map<String, String> metadata = metadataFor(type);
+        if (type.isNullable()) {
+            return new FieldType(true, arrowType, null, metadata);
+        }
+        return new FieldType(false, arrowType, null, metadata);
+    }
+
+    /** Hyper-compatible field metadata for types whose length is not carried in the ArrowType. */
+    private static Map<String, String> metadataFor(HyperType type) {
+        switch (type.getKind()) {
+            case CHAR:
+                if (type.getPrecision() == 1) {
+                    return Collections.singletonMap("hyper:type", "Char1");
+                }
+                // A general CHAR(n). Stamp both the discriminator and the length.
+                Map<String, String> charMeta = new HashMap<>();
+                charMeta.put("hyper:type", "Char");
+                charMeta.put("hyper:max_string_length", Integer.toString(type.getPrecision()));
+                return charMeta;
+            case VARCHAR:
+                if (type.getPrecision() == HyperType.UNLIMITED_LENGTH) {
+                    // No length to stamp — unbounded VARCHAR is indistinguishable from "text" in
+                    // Arrow and that's fine; both decode to varcharUnlimited.
+                    return null;
+                }
+                return Collections.singletonMap("hyper:max_string_length", Integer.toString(type.getPrecision()));
+            default:
+                return null;
+        }
     }
 
     /** Map a {@link HyperType} to an Arrow {@link ArrowType} (no nullability). */

jdbc-core/src/test/java/com/salesforce/datacloud/jdbc/protocol/data/HyperTypeArrowRoundtripTest.java

@@ -6,10 +6,9 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
+import java.util.Collections;
 import java.util.stream.Stream;
-import org.apache.arrow.vector.types.pojo.ArrowType;
 import org.apache.arrow.vector.types.pojo.Field;
-import org.apache.arrow.vector.types.pojo.FieldType;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
@@ -27,8 +26,10 @@ class HyperTypeArrowRoundtripTest {
 
     /**
      * Core positive cases. Every HyperType listed here must round-trip back to an equal
-     * HyperType. Kinds with documented lossy encodings (CHAR/bounded VARCHAR, TIME_TZ) live in
-     * {@link #deviationsFromRoundtrip()} instead.
+     * HyperType. CHAR(n) / CHAR(1) / VARCHAR(n) round-trip via {@code hyper:*} field metadata
+     * stamped by {@link HyperTypeToArrow#toFieldType} and read back by
+     * {@link ArrowToHyperTypeMapper}. TIME_TZ lives in {@link #lossyRoundtrip_timeTzCollapsesToTime}
+     * instead (see comment there for why it stays lossy).
      */
     static Stream<Arguments> encodableTypes() {
         return Stream.of(
@@ -42,6 +43,10 @@ static Stream<Arguments> encodableTypes() {
                 Arguments.of(HyperType.float8(false)),
                 Arguments.of(HyperType.decimal(10, 2, true)),
                 Arguments.of(HyperType.decimal(38, 0, false)),
+                Arguments.of(HyperType.fixedChar(1, true)),
+                Arguments.of(HyperType.fixedChar(16, true)),
+                Arguments.of(HyperType.varchar(255, false)),
+                Arguments.of(HyperType.varchar(1024, true)),
                 Arguments.of(HyperType.varcharUnlimited(true)),
                 Arguments.of(HyperType.binary(16, true)),
                 Arguments.of(HyperType.varbinary(false)),
@@ -56,21 +61,16 @@ static Stream<Arguments> encodableTypes() {
     @ParameterizedTest
     @MethodSource("encodableTypes")
     void roundtripReproducesHyperType(HyperType original) {
-        // Forward: HyperType -> Arrow.
-        ArrowType arrowType = HyperTypeToArrow.toArrowType(original);
-        Field field = HyperTypeKind.ARRAY == original.getKind()
-                // Arrow list Fields need a child field whose type matches the list element.
-                ? new Field(
-                        "col",
-                        new FieldType(original.isNullable(), arrowType, null),
-                        java.util.Collections.singletonList(new Field(
-                                "",
-                                new FieldType(
-                                        original.getElement().isNullable(),
-                                        HyperTypeToArrow.toArrowType(original.getElement()),
-                                        null),
-                                java.util.Collections.emptyList())))
-                : new Field("col", new FieldType(original.isNullable(), arrowType, null), null);
+        // Forward via the full toField helper so any field-level metadata (hyper:type,
+        // hyper:max_string_length) travels alongside the ArrowType.
+        Field field;
+        if (original.getKind() == HyperTypeKind.ARRAY) {
+            // Arrow list Fields need a child field whose type matches the list element.
+            Field child = new Field("", HyperTypeToArrow.toFieldType(original.getElement()), Collections.emptyList());
+            field = new Field("col", HyperTypeToArrow.toFieldType(original), Collections.singletonList(child));
+        } else {
+            field = new Field("col", HyperTypeToArrow.toFieldType(original), null);
+        }
 
         // Reverse: Arrow Field -> HyperType.
         HyperType roundTripped = ArrowToHyperTypeMapper.toHyperType(field);
@@ -96,37 +96,27 @@ void nonEncodableKinds() {
         assertThat(HyperTypeKind.NULL).isNotNull();
     }
 
-    /**
-     * Known, documented lossy round-trips. Each is something Arrow cannot preserve without
-     * additional Hyper-specific field metadata, which the Arrow stream Hyper emits today does
-     * not include on the parameter-binding side. Fixing any of these needs
-     * {@link HyperTypeToArrow} to stamp hyper:* metadata on the Field and
-     * {@link ArrowToHyperTypeMapper} to read it back.
-     */
-    @Test
-    void lossyRoundtrip_fixedCharCollapsesToVarcharUnlimited() {
-        // HyperTypeToArrow maps CHAR(n) to Utf8 (Arrow has no fixed-length string). The reverse
-        // mapping has no length hint and reports unlimited VARCHAR.
-        HyperType original = HyperType.fixedChar(16, true);
-        Field field = new Field("col", new FieldType(true, HyperTypeToArrow.toArrowType(original), null), null);
-        assertThat(ArrowToHyperTypeMapper.toHyperType(field)).isEqualTo(HyperType.varcharUnlimited(true));
-    }
-
-    @Test
-    void lossyRoundtrip_boundedVarcharCollapsesToUnlimited() {
-        // HyperTypeToArrow maps VARCHAR(n) to Utf8 with no length metadata; the reverse cannot
-        // reconstruct the declared bound.
-        HyperType original = HyperType.varchar(255, false);
-        Field field = new Field("col", new FieldType(false, HyperTypeToArrow.toArrowType(original), null), null);
-        assertThat(ArrowToHyperTypeMapper.toHyperType(field)).isEqualTo(HyperType.varcharUnlimited(false));
-    }
-
     @Test
     void lossyRoundtrip_timeTzCollapsesToTime() {
-        // HyperTypeToArrow maps both TIME and TIME_TZ to Arrow Time(MICROSECOND, 64); Arrow Time
-        // has no timezone slot, so the distinction is lost on the way back.
+        // TIME WITH TIME ZONE cannot round-trip through Arrow today, and fixing it cleanly needs
+        // more than a metadata stamp:
+        //
+        //   - Arrow's Time type has no timezone slot (unlike Timestamp, whose Timestamp.timezone
+        //     field we already use for TIMESTAMP_TZ). So a per-value offset cannot ride on the
+        //     ArrowType itself.
+        //   - SQL `time with time zone` stores a per-*value* UTC offset (e.g. "14:00+02" vs
+        //     "14:00-05" in the same column). Field metadata is per-column, so it cannot carry
+        //     a value-varying offset.
+        //   - The driver's read-side QueryJDBCAccessorFactory has no TIME_TZ branch either —
+        //     server-returned `timetz` columns already collapse to the plain TimeVectorAccessor,
+        //     so the offset is dropped on ingress as well. Fixing the write-side alone would
+        //     create an asymmetry worse than the current uniform loss.
+        //
+        // Proper round-trip needs a synthetic schema (e.g. struct{time, offset_seconds}) or a
+        // Hyper-protocol coordination. Leaving TIME_TZ explicitly pinned as lossy until that
+        // design lands.
         HyperType original = HyperType.timeTz(true);
-        Field field = new Field("col", new FieldType(true, HyperTypeToArrow.toArrowType(original), null), null);
+        Field field = new Field("col", HyperTypeToArrow.toFieldType(original), null);
         assertThat(ArrowToHyperTypeMapper.toHyperType(field)).isEqualTo(HyperType.time(true));
     }
 }