Add VARIANT_JSON and VARIANT_BINARY client capabilities#29046
Add VARIANT_JSON and VARIANT_BINARY client capabilities#29046
Conversation
| verify(!types.isEmpty(), "Columns must not be empty"); | ||
|
|
||
| boolean supportsParametricDateTime = requireNonNull(session, "session is null") | ||
| Session querySession = requireNonNull(session, "session is null"); |
There was a problem hiding this comment.
| Session querySession = requireNonNull(session, "session is null"); | |
| _ clientCapabilities = session.getClientCapabilities(); |
c1da882 to
aba2fc7
Compare
aba2fc7 to
1ccf36e
Compare
|
Please don't remove |
There is no relevant issue. Someone modified the message to say it fixes a bug, but there is no bug since variant is unreleased. |
1ccf36e to
308d8d8
Compare
|
Tested with trino cli 468 |
| * Whether client supports the `VARIANT` type encoded as a binary payload on the wire. | ||
| * This capability is opt-in, so clients continue to receive the JSON representation by default. | ||
| */ | ||
| VARIANT_BINARY(false), |
There was a problem hiding this comment.
Is this supposed to be mentioned in docs/src/main/sphinx/develop/client-protocol.md ?
I'm seeing in io.trino.jdbc.TrinoConnection#startQuery that it is being added without any checks.
There was a problem hiding this comment.
I don't see Client-Capabilities documented anywhere
| import static java.util.Collections.unmodifiableMap; | ||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| public final class Variant |
| return result; | ||
| }) | ||
| .add("variant", EncodedVariant.class, Variant.class, AbstractTrinoResultSet::decodeVariant) | ||
| .add("variant", EncodedVariant.class, Object.class, value -> decodeVariant(value).toObject()) |
There was a problem hiding this comment.
rs.getObject(column, Object.class)to get the corresponding Java object representation
i don't think this is how getObject should work.
i would expect rs.getObject(column, Object.class) to return same thing as rs.getObject(column) does.
does JDBC spec define this?
| .put("interval day to second", TrinoIntervalDayTime.class) | ||
| .put("map", Map.class) | ||
| .put("row", Row.class) | ||
| .put("variant", Variant.class) |
There was a problem hiding this comment.
When using old JDBC, user gets VARIANT as JSON.
When using new JDBC, if user wants JSON for the VARIANT, what should they do?
| } | ||
|
|
||
| if (columnInfo.getColumnTypeSignature().getRawType().equals("variant") && value instanceof String) { | ||
| return value; |
There was a problem hiding this comment.
Remove.
This looks like shadowing DEFAULT_OBJECT_REPRESENTATION entry.
| if (columnType.getRawType().equals("variant") && value instanceof String) { | ||
| return value; | ||
| } |
There was a problem hiding this comment.
Remove.
This looks like shadowing DEFAULT_OBJECT_REPRESENTATION entry.
| .add("variant", EncodedVariant.class, Map.class, value -> variantToMap(decodeVariant(value))) | ||
| .add("variant", EncodedVariant.class, List.class, value -> variantToList(decodeVariant(value))) |
There was a problem hiding this comment.
I am not convinced about this additional conversions.
They might be useful, but they also prevent as from adding rs.getObject(col, String.class) that would return VARIANT-as-JSON data.
Let's not add them in this PR.
User can always invoke a specific method/cast on the Variant object
| if (columnTypeSignature.getRawType().equals("variant") && object instanceof String) { | ||
| if (type.isInstance(object)) { | ||
| return type.cast(object); | ||
| } | ||
| throw new SQLException(format("Cannot convert VARIANT JSON text to %s", type)); | ||
| } |
There was a problem hiding this comment.
Remove.
This shadows TYPE_CONVERSIONS entry.
| throws Exception | ||
| { | ||
| try (ConnectedStatement connectedStatement = newStatement()) { | ||
| checkRepresentation(connectedStatement.getStatement(), "CAST(NULL AS variant)", Types.JAVA_OBJECT, (rs, column) -> { |
There was a problem hiding this comment.
does variant have NULL and NULL-value concepts like JSON?
| Variant variant = rs.getObject(column, Variant.class); | ||
| assertThat(rs.getObject(column)).isInstanceOf(Variant.class); |
| assertThat(rs.getObject(column)).isInstanceOf(Variant.class); | ||
| assertThat(variant.valueType()).isEqualTo(Variant.ValueType.DATE); | ||
| assertThat(variant.getLocalDate()).isEqualTo(LocalDate.of(2021, 2, 3)); | ||
| assertThat(rs.getObject(column, Object.class)).isEqualTo(LocalDate.of(2021, 2, 3)); |
There was a problem hiding this comment.
Perhaps cool, but let's leave it up to a future PR
Description
Add
VARIANT_JSONclient capability, which when missing sends the data asJSONtype.Add
VARIANT_BINARYclient capability for JDBC. JDBC can accessVARIANTcolumns using:rs.getObject(column)orrs.getObject(column, io.trino.jdbc.Variant.class)to get anio.trino.jdbc.Variantrs.getObject(column, Object.class)to get the corresponding Java object representationrs.getObject(column, Map.class)orrs.getObject(column, List.class)for top-level object and array valuesWhen using
rs.getObject(column, Object.class),VARIANTvalues map to Java objects as follows:nullBooleanByteShortIntegerLongFloatDoubleBigDecimalStringbyte[]LocalDateLocalTimeInstantLocalDateTimeUUIDList<Object>Map<String, Object>The
io.trino.jdbc.Variantclass is a minimal implementation of VARIANT for decoding data. This supports getting the raw underlying byte arrays for the value and metdata is the client wants to interact with the raw data (storing in a separate system directly without reencoding). The SPI Variant class cannot be used because it is compiled with Java 25.Release notes
(x) Release notes are required, with the following suggested text: