Adopt dataType for Literals and preparedValues #3395

g31pranjal · 2025-06-12T17:35:42Z

This PR moves away from using java.sql.Types in the internals of fdb-relational-core, while still maintaining JDBC-compliance for external getType()-like operations. It is replaced with DataType, which provides richer and more diverse type information available for downstream operations. Notably, we can now have types like Uuid, Enum, Version expressed natively. There are several consequences of this:

It eliminates SQLType <-> RecordType conversions and instead relies on DataType <-> RecordType conversions, which were already being used for DDL work. This is more consistent as it is used throughout the codebase.
StructMetadata and ArrayMetadata wrap DataType as the source of truth for their type info. However, they still map to sql.Types externally. Moreover, they now expose getRelationalDataType() that returns DataType to the external users for more transparency.
Aligning to the above changes, grpc protobufs now have a Type enum, moving away from sql.Types too. This was needed to support struct and array operations homogenously across embedded and JDBC drivers. It keeps the old types too for backward compatibility, which I think should be removed in future to simplify conversions.

hatyo · 2025-07-01T13:35:29Z

...al-server/src/testFixtures/java/com/apple/foundationdb/relational/server/ServerTestUtil.java

 import io.prometheus.client.CollectorRegistry;
+import org.apache.logging.log4j.LogManager;


please revert.

hatyo · 2025-07-01T13:37:16Z

fdb-relational-server/src/main/java/com/apple/foundationdb/relational/server/FRL.java

@@ -235,49 +234,33 @@ public Response execute(String database, String schema, String sql, List<Paramet

    private static void addPreparedStatementParameter(RelationalPreparedStatement relationalPreparedStatement,
                                                      Parameter parameter, int index) throws SQLException {


Can you please add @Nonnull annotations where applicable to this method signature?

hatyo · 2025-07-01T14:21:41Z

fdb-relational-grpc/src/main/proto/grpc/relational/jdbc/v1/jdbc.proto

@@ -106,6 +106,7 @@ message Parameter {
  // Reuse the column type here. It has much of what we need carrying
  // across parameters as well as null support, etc.
  Column parameter = 2;
+  Type type = 3;


Can we remove the java_sql_types_code above? the mapping between Type and java SQL type is hardcoded in the codebase, and so it is redundant here, right?

We cannot actually remove the java_sql_types_code straightaway - otherwise mixed-mode testing will start breaking as they still populate the java_sql_types_code field. Part of the code in JDBC deals with using the new Types, if available, otherwise fall to sql types.

We can probably remove sql types in futue release though.

hatyo · 2025-07-01T14:26:19Z

fdb-relational-grpc/src/main/proto/grpc/relational/jdbc/v1/column.proto

  // Indicates a column that isn't part of the DDL for a query, but is part of the returned
  // tuple (and therefore necessary to keep so that our positional ordering is intact)
  bool phantom = 5;
  // If this column has a Struct or an Array, their metadata is here.
  oneof metadata {
    ListColumnMetadata structMetadata = 6;
    ColumnMetadata arrayMetadata = 7;
+    EnumMetadata enumMetadata = 8;


please adapt the comment at line 55.

hatyo · 2025-07-01T14:28:43Z

fdb-relational-grpc/src/main/proto/grpc/relational/jdbc/v1/column.proto

@@ -26,20 +26,39 @@ option java_multiple_files = true;
 option java_package = "com.apple.foundationdb.relational.jdbc.grpc.v1.column";
 option java_outer_classname = "ColumnProto";

+enum Type {
+  NO_TYPE = 0;


I think I am fine with UNKNOWN - just that my early idea came from the fact that there will be no type supplied by older versions.

hatyo · 2025-07-01T15:14:20Z

...ional-core/src/main/java/com/apple/foundationdb/relational/api/EmbeddedRelationalStruct.java


        final List<Object> elements = new ArrayList<>();

        @Override
        public EmbeddedRelationalStruct build() {
-            return new ImmutableRowStruct(new ArrayRow(elements.toArray()), new RelationalStructMetaData(fields.toArray(new FieldDescription[0])));
+            // Ideally, the name of the struct should be something that the user can specify, however, this actually


There is already an open issue about this. The struct building and verification is something the client, and the structure name should be something the user can specify as well.

hatyo · 2025-07-01T15:27:16Z

...rc/main/java/com/apple/foundationdb/relational/recordlayer/EmbeddedRelationalConnection.java

    }

    @Override
    public Struct createStruct(String typeName, Object[] attributes) throws SQLException {
+        // We do not preserve the typeName, as there is no validation around that currently.
+        final var builder = EmbeddedRelationalStruct.newBuilder();


I do like the simplification, nice work.

hatyo · 2025-07-01T15:35:11Z

fdb-relational-core/src/main/java/com/apple/foundationdb/relational/api/RowStruct.java

@@ -240,8 +240,6 @@ public RelationalArray getArray(int oneBasedPosition) throws SQLException {
            for (final var t : coll) {
                if (t instanceof Message) {
                    elements.add(new ImmutableRowStruct(new MessageTuple((Message) t), arrayMetaData.getElementStructMetaData()));
-                } else if (t instanceof ByteString) {


I see that in MessageTuple.sanitizeField we return ByteString::toByteArray() when encountering a ByteString object. However, in this else if branch I see that you removed the dispatch on ByteString without sanitizing t, is this expected? Perhaps it is called indirectly above but I am not sure.

Yes. The hidden contract here is that whatever comes out of MessageTuple#getObjectInternal, is already sanitised, which is depicted by the altered logic here. However, I had to "sanitise" for a special case when we think its a Message but is actually a special-cased nullable wrapper. For this particular case, we will have to sanitise the "repeated" field (content of that array).

hatyo · 2025-07-01T15:59:32Z

...ain/java/com/apple/foundationdb/relational/recordlayer/query/visitors/ExpressionVisitor.java

        }
-        return expression;
+        if (resultType.isArray() && value instanceof AbstractArrayConstructorValue) {


You should check whether the promotion is necessary in case of arrays as well, otherwise, a new LightArrayConstructorValue is constructed, potentially unnecessarily.

nice catch!

fdb-relational-api/src/main/java/com/apple/foundationdb/relational/api/StructMetaData.java

hatyo · 2025-07-04T13:03:35Z

fdb-relational-api/src/main/java/com/apple/foundationdb/relational/api/metadata/DataType.java

+            if (this.isNullable() != otherStructType.isNullable() || fields.size() != otherFields.size()) {
+                return false;
+            }
+            for (int i = 0; i < fields.size(); i++) {


This is not exactly correct. If the other structure has more fields it would still pass although it shouldn't I think.

hatyo · 2025-07-04T14:15:24Z

...al-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/LiteralsUtils.java

-            } catch (SQLException e) {
-                throw new RelationalException(e).toUncheckedWrappedException();
+        DataType distinctType = null;
+        for (var object: objects) {


This is not entirely correct I think in the presence of nullable fields.

hatyo · 2025-07-07T13:23:17Z

...ational-grpc/src/main/java/com/apple/foundationdb/relational/jdbc/RelationalArrayFacade.java

@@ -66,9 +74,16 @@ class RelationalArrayFacade implements RelationalArray {

    RelationalArrayFacade(@Nonnull ColumnMetadata delegateMetadata, Array array) {
        this.delegateMetadata = delegateMetadata;
+        this.type = Suppliers.memoize(this::computeType);


It is almost always a good idea to memoize repeated computations, but I think this should be pushed to the underlying delegate if possible (since this class is a mere facade presumably).

hatyo · 2025-07-07T13:23:50Z

.../src/main/java/com/apple/foundationdb/relational/jdbc/RelationalResultSetMetaDataFacade.java

+    @Nonnull
+    @Override
+    public DataType.StructType getRelationalDataType() throws SQLException {
+        throw new SQLException("Not implemented", ErrorCode.UNSUPPORTED_OPERATION.getErrorCode());


Can we implement this?

hatyo · 2025-07-07T13:25:34Z

fdb-relational-grpc/src/main/java/com/apple/foundationdb/relational/jdbc/TypeConversion.java

        var listColumnMetadataBuilder = ListColumnMetadata.newBuilder();
        for (int oneBasedIndex = 1; oneBasedIndex <= metadata.getColumnCount(); oneBasedIndex++) {
-            var columnMetadata = toColumnMetadata(metadata, oneBasedIndex);
+            var columnMetadata = toColumnMetadata(metadata, oneBasedIndex, oneBasedIndex - 1 + metadata.getLeadingPhantomColumnCount());


was this wrong all the time (in the presence of phantom columns, that is)?

g31pranjal force-pushed the adpot_dataType branch 2 times, most recently from 0e56d68 to bdc3827 Compare June 14, 2025 13:05

g31pranjal force-pushed the adpot_dataType branch 3 times, most recently from def373f to 0630175 Compare June 30, 2025 15:40

Use relational types for structs and arrays

582bd0e

g31pranjal force-pushed the adpot_dataType branch from 0630175 to 582bd0e Compare June 30, 2025 16:49

g31pranjal added enhancement New feature or request bug fix Change that fixes a bug labels Jun 30, 2025

hatyo requested changes Jul 1, 2025

View reviewed changes

g31pranjal added 3 commits July 2, 2025 17:54

address comments

978176f

address comments

c206e2b

revert a file

376ee67

g31pranjal marked this pull request as ready for review July 2, 2025 17:11

g31pranjal requested a review from hatyo July 4, 2025 13:38

hatyo requested changes Jul 4, 2025

View reviewed changes

hatyo requested changes Jul 7, 2025

View reviewed changes

		import io.prometheus.client.CollectorRegistry;
		import org.apache.logging.log4j.LogManager;

		@@ -235,49 +234,33 @@ public Response execute(String database, String schema, String sql, List<Paramet

		private static void addPreparedStatementParameter(RelationalPreparedStatement relationalPreparedStatement,
		Parameter parameter, int index) throws SQLException {

Adopt dataType for Literals and preparedValues #3395

Are you sure you want to change the base?

Adopt dataType for Literals and preparedValues #3395

Uh oh!

Conversation

g31pranjal commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

g31pranjal commented Jun 12, 2025 •

edited

Loading