Benbellick/handle structured udt #609

benbellick · 2025-11-17T19:32:38Z

Closes #603

Support both opaque (google.protobuf.Any) and structured (Literal.Struct) encodings for user-defined type literals per Substrait spec. - Split UserDefinedLiteral into UserDefinedAny and UserDefinedStruct - Move type parameters to interface level for parameterized types - Add ExtensionCollector.getExtensionCollection() method - Full Calcite integration with REINTERPRET pattern for Any-based UDTs - Add Scala visitor methods and comprehensive documentation - Comprehensive test coverage including roundtrip tests

asolimando

I have some initial suggestions, there are a couple of places where I am not 100% sure like switching from SqlTypeName.ROW to SqlTypeName.OTHER and I have explicitly mentioned that, the rest should be safe to be implemented, feel free to ping me when it's reviewable again :)

asolimando · 2025-11-20T20:10:23Z

isthmus/src/test/java/io/substrait/isthmus/utils/UserTypeFactory.java


+  /**
+   * Test-specific variant of the core implementation that treats Calcite-copied types as
+   * equivalent, even when they are not the same Java instance. Calcite often clones UDTs during


I don't think this is accurate, in general you use canonize to "create" data types, which internally uses a cache and the same object is returned if already seen before.

Lots of places in Calcite compare them by instance (Java's ==), so if that was true it would break in a few places.

RelDataTypeFactory is pretty explicit on this in its javadoc.

public RelDataType createUserDefinedType( String urn, String name, List<Type.Parameter> params, boolean nullable) { SubstraitUserDefinedType type = new SubstraitUserDefinedType(urn, name, params, nullable); return canonize(type); <-- this is what guarantees uniqueness for your types }

Make sure you never create types manually, at that point you should be able to drop methods like matchesCalciteAlias that are just working around the issue, possibly also matchesSubstraitType

asolimando · 2025-11-21T13:46:49Z

isthmus/src/main/java/io/substrait/isthmus/type/SubstraitUserDefinedType.java

+
+  /** Creates a SubstraitUserDefinedType from a Substrait Type.UserDefined. */
+  public static SubstraitUserDefinedType from(io.substrait.type.Type.UserDefined type) {
+    return new SubstraitUserDefinedType(


Here you must canonize, and for doing that you need a handle to a Calcite type factory

asolimando · 2025-11-21T13:47:19Z

isthmus/src/main/java/io/substrait/isthmus/type/SubstraitUserDefinedType.java

+
+    /** Creates a SubstraitUserDefinedAnyType from a Substrait Type.UserDefined. */
+    public static SubstraitUserDefinedAnyType from(io.substrait.type.Type.UserDefined type) {
+      return new SubstraitUserDefinedAnyType(


Same, canonization is needed here

asolimando · 2025-11-21T13:47:38Z

isthmus/src/main/java/io/substrait/isthmus/type/SubstraitUserDefinedType.java

+        io.substrait.type.Type.UserDefined type,
+        List<RelDataType> fieldTypes,
+        List<String> fieldNames) {
+      return new SubstraitUserDefinedStructType(


Canonization needed

asolimando · 2025-11-21T14:20:01Z

isthmus/src/main/java/io/substrait/isthmus/type/SubstraitUserDefinedType.java

+        throw new IllegalArgumentException("Field types and names must have same length");
+      }
+      this.fieldTypes = fieldTypes;
+      this.fieldNames = fieldNames;


Here you must call again computeDigest() after setting fieldTypes and fieldNames (true for any field in general), because RelDataTypeImpl identity is based on the digest field. This is true for all subclasses adding or changing the field.

Similarly, I expect to see equals() and hashCode() overrides when you have extra fields, I think you should add them.

RelDataTypeImpl is Serializable, are we making sure the extra fields are serializable too?

asolimando · 2025-11-21T14:28:08Z

isthmus/src/main/java/io/substrait/isthmus/type/SubstraitUserDefinedType.java

+      // Can be considered as ROW since it has structure
+      return SqlTypeName.ROW;


I am not 100% sure, but I am afraid you would then be matching built-in ROW types, I'd rather play it safe and use SqlTypeName.OTHER (like the base class) and use isStruct() override to indicate structure, something like:

@Override public boolean isStruct() { return true; }

I am starting from the assumption that, if any structured type would make sense as ROW, we would not have this boolean method, but I don't have any evidence for saying you shouldn't do it.

asolimando · 2025-11-21T14:32:31Z

isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java

+    java.util.List<String> fieldNames =
+        java.util.stream.IntStream.range(0, expr.fields().size())
+            .mapToObj(i -> "f" + i)
+            .collect(java.util.stream.Collectors.toList());


From https://substrait.io/types/type_classes/#communicating-user-defined-types I was under the impression that UDTs were coming with their own field names, expr.fields() might have them set?

If that's not necessarily the case, we should at least preserve them whenever they are set.

asolimando · 2025-11-21T14:36:47Z

isthmus/src/main/java/io/substrait/isthmus/type/SubstraitUserDefinedType.java

+    }
+
+    @Override
+    public RelDataTypeField getField(String fieldName, boolean caseSensitive, boolean elideRecord) {


The elideRecord parameter is not used here, IIRC it's used for nested field access. You should check how RelDataTypeImpl.getField() behaves and possibly:

remove the override if not needed,

call super.getField() at the top and add your extra logic after,

add the nested type handling and honor the elideRecord param

asolimando · 2025-11-21T14:51:29Z

isthmus/src/test/java/io/substrait/isthmus/type/SubstraitUserDefinedTypeTest.java

+        new SubstraitUserDefinedType.SubstraitUserDefinedAnyType(
+            URN, NAME, List.of(integerParam), true);
+
+    assertEquals(left, right);


you should be able to assert left == right after fixing type canonization

asolimando · 2025-11-21T14:54:56Z

isthmus/src/test/java/io/substrait/isthmus/type/SubstraitUserDefinedTypeTest.java

Can we add tests for nested UDT field access? (to exercise the elideRecord argument in getField) and, if supported, a UDT within a struct within UDT (if such a mixup is valid)?

It might be interesting to test also two UDTs with same URN/name but different type parameters like Point<i32> and Point<i64>, I expect them to be different, as i32 != i64

benbellick · 2025-11-24T22:41:05Z

I decided to break this up into 2 PRs, one for core and one for isthmus. The one for core is here: #613

benbellick · 2025-11-26T21:52:17Z

Working on another version of this here: #623

This will be built off of #613

benbellick force-pushed the benbellick/handle-structured-udt branch 4 times, most recently from b7ea41e to bee33ea Compare November 18, 2025 22:00

benbellick force-pushed the benbellick/handle-structured-udt branch from bee33ea to b4fbb07 Compare November 18, 2025 22:05

benbellick added 10 commits November 19, 2025 17:38

some more refactor

dcb1529

refactor: small simplification

e9ea6cd

test: simplify roundtrip test

5a8bc40

revert: drop getExtensionCollection() impl from ExtensionCollector

ad94adc

tweak: simplify expression creator

5eeff70

revert: simple extension

fe01302

tweak: simplify TypeConverter.java

ccbd5a3

tweak: use default types and simplify code a bit

99fe14c

tweak: more adjustments

8e05488

tweak: more adjustments

3c3b63f

asolimando reviewed Nov 21, 2025

View reviewed changes

		// Can be considered as ROW since it has structure
		return SqlTypeName.ROW;

Benbellick/handle structured udt #609

Are you sure you want to change the base?

Benbellick/handle structured udt #609

Uh oh!

Conversation

benbellick commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asolimando left a comment

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!

benbellick commented Nov 24, 2025

Uh oh!

benbellick commented Nov 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benbellick commented Nov 17, 2025 •

edited

Loading