Various cleanups and improvements in TypeParameter #27574

martint · 2025-12-08T07:26:43Z

Description

See individual commit details.

Release notes

(x) Release notes are required, with the following suggested text:

## SPI
* Remove support for `TypeSignatureParameter`. Use `TypeParameter`, instead. ({issue}`27574`)
* Remove support for `ParameterKind`. Use `TypeParameter.Type`, `TypeParameter.Numeric`, `TypeParameter.Variable`, instead. ({issue}`27574`)
* Remove support for `NamedType`, `NamedTypeSignature` and `NamedTypeParameter`. Use `TypeParameter.Type`, instead. ({issue}`27574`)

core/trino-spi/src/main/java/io/trino/spi/type/TypeParameter.java

findepi

Get type arguments directly from the type LGTM

core/trino-main/src/main/java/io/trino/operator/scalar/Re2JCastToRegexpFunction.java

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaHiveTypeTranslator.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/util/HiveTypeTranslator.java

findepi

Remove TypeParameter LGTM

core/trino-main/src/main/java/io/trino/type/ArrayParametricType.java

core/trino-main/src/main/java/io/trino/type/RowParametricType.java

findepi

Rename TypeSignatureParameter -> TypeParameter LGTM assuming it's mechanical rename

findepi

Fix incorrect type declaration LGTM

core/trino-main/src/test/java/io/trino/operator/scalar/CustomFunctions.java

findepi

Remove NamedType and friends LGTM

client/trino-jdbc/src/main/java/io/trino/jdbc/AbstractTrinoResultSet.java

core/trino-main/src/main/java/io/trino/server/protocol/ProtocolUtil.java

core/trino-main/src/main/java/io/trino/type/RowParametricType.java

core/trino-spi/src/main/java/io/trino/spi/type/TypeParameter.java

findepi · 2025-12-10T16:32:08Z

core/trino-spi/src/main/java/io/trino/spi/type/TypeParameter.java

+
        if (kind == ParameterKind.VARIABLE) {
-            prefix = "@";
+            prefix += "@";


aren't name.isPresent() and kind == ParameterKind.VARIABLE exclusive conditions?
if so, better to have a check then lenient +=

In theory, it would be possible to have a named field with an unbound type.

plugin/trino-hive/src/test/java/io/trino/plugin/hive/HiveTestUtils.java

findepi

Deprecate Type.getTypeParameters()

TL;DR

many of these refactors looks good on their own, but it's hard to review all of them at once

findepi · 2025-12-10T16:40:46Z

core/trino-main/src/main/java/io/trino/sql/gen/columnar/InColumnarFilterGenerator.java

        // Until we support HASH_SWITCH strategy for code generation here, we treat structural type as an unsupported case
        // and fall back to existing expression evaluator for small lists
-        if (!type.getTypeParameters().isEmpty()) {
+        if (type instanceof ArrayType || type instanceof MapType || type instanceof RowType) {


ParametricType is in SPI so one could imagine a plugin adding a structural type. The new check doesn't reject this.

in fact, ClassifierType is an example for which the new check returns different result

Agreed. However, that's only a theoretical concern right now. There's also a difference from a structural type (i.e., a container such as map/array/row) and a specialized type for type-safety and static type validations.

in fact, i don't know why we throw for some types, and return false for some other types.
it's looks like throwing isn't necessary in this method at all, because the final type validation/rejection needs to be done somewhere later ... and therefore it doesn't matter much what we do here. the new validation is weaker (more persmissive), and therefore likely incorrect, but it's whatever

core/trino-spi/src/main/java/io/trino/spi/type/Type.java

findepi

Improve type safety for TypeParameter

this change is least mechanical of all the commits in this PR.
i'd suggest separate PR.

martint · 2025-12-10T18:47:22Z

i'd suggest separate PR.

Happy to do that. However, since all these commits affect the SPI, I wanted to get them all in the same timeframe to avoid thrash across multiple releases.

martint · 2025-12-10T20:09:21Z

many of these refactors looks good on their own, but it's hard to review all of them at once

They are meant to be reviewed individually. They are all logically independent from each other, although they build on/touch the same areas the previous commit touched.

findepi · 2025-12-11T17:06:18Z

many of these refactors looks good on their own, but it's hard to review all of them at once

They are meant to be reviewed individually.

fair & my understanding as well. just the diff is longish

findepi

Deprecate Type.getTypeParameters()

i don't like this single long diff of manual & interesting changes to be forced upon me within this PR.
but ok, looked thru and LGTM % comments

lib/trino-hive-formats/src/main/java/io/trino/hive/formats/avro/AvroPageDataReader.java

findepi · 2025-12-11T17:09:14Z

lib/trino-hive-formats/src/test/java/io/trino/hive/formats/FormatTestUtils.java

+        Type elementType = switch (type) {
+            case RowType rowType -> rowType.getFields().getFirst().getType();
+            case ArrayType arrayType -> arrayType.getElementType();
+            default -> throw new IllegalArgumentException("Unexpected type: " + type);


it's weird to take just a first field of a row. worth code comment

the code also worked for map type which is now unhandled. i understand map never lands in this (test) code?

That was confusing to me, too. ~~But I think this method is designed to handle the case of decoding a list of uniform values and either pivot it into a row or map it to a Trino array.~~ I actually have no idea why this method would be called with a row type where only the first field type matters. It seems like a I'm not sure what the semantics would be for a Map (maybe key-value pairs?), either, but it doesn't matter since it doesn't happen in practice. We can always add that whenever the use case comes up.

BTW, getting the first field the row is not a new behavior -- that's implicitly how it it was done before. I just ported the behavior.

...test/java/io/trino/hive/formats/avro/TestAvroPageDataReaderWithAvroNativeTypeManagement.java

...rmats/src/test/java/io/trino/hive/formats/avro/TestAvroPageDataWriterWithoutTypeManager.java

lib/trino-parquet/src/main/java/io/trino/parquet/reader/ParquetReader.java

findepi · 2025-12-11T17:19:08Z

Happy to do that. However, since all these commits affect the SPI, I wanted to get them all in the same timeframe to avoid thrash across multiple releases.

I don't think it's a really big problem.

TypeSignature should not be used in contexts where the Type is available. It's purpose is mainly for doing type inference and generic manipulation of types.

It's only used during instantiation of parametric types, as a mirror of TypeSignatureParameter. The translation is not needed.

The type parameter must match the argument it corresponds to.

It's not an ideal abstraction. Naming of fields is orthogonal to what type the argument is, so we don't need a dedicated class for it.

For general use, it's preferable to call the type-specific methods (e.g., ArrayType.getElementType(), MapType.getKeyType, MapType.getValueType())

Use a sum type instead of a tag + value. It makes it possible to use deconstruction patterns and switch statements/expressions to manipulate the values of the type.

martint · 2025-12-12T16:56:36Z

@findepi , updated

cla-bot bot added the cla-signed label Dec 8, 2025

github-actions bot added iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector mongodb MongoDB connector blackhole Blackhole connector labels Dec 8, 2025

martint commented Dec 8, 2025

View reviewed changes

core/trino-spi/src/main/java/io/trino/spi/type/TypeParameter.java Outdated Show resolved Hide resolved

martint force-pushed the type-parameter branch from 9ae3f14 to 1ed4023 Compare December 8, 2025 08:08

github-actions bot added the jdbc Relates to Trino JDBC driver label Dec 8, 2025

martint force-pushed the type-parameter branch 3 times, most recently from e62a20a to 6fa0b0c Compare December 9, 2025 01:35

github-actions bot added bigquery BigQuery connector kafka Kafka connector pinot Pinot connector labels Dec 9, 2025

martint force-pushed the type-parameter branch 3 times, most recently from 84e65fc to c0b738d Compare December 9, 2025 04:08

martint marked this pull request as ready for review December 9, 2025 05:02

martint requested review from dain and findepi December 9, 2025 05:03

findepi reviewed Dec 10, 2025

View reviewed changes

core/trino-main/src/main/java/io/trino/type/ArrayParametricType.java Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/type/RowParametricType.java Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/type/RowParametricType.java Outdated Show resolved Hide resolved

findepi reviewed Dec 10, 2025

View reviewed changes

core/trino-main/src/test/java/io/trino/operator/scalar/CustomFunctions.java Show resolved Hide resolved

core/trino-main/src/test/java/io/trino/operator/scalar/CustomFunctions.java Show resolved Hide resolved

martint force-pushed the type-parameter branch from c0b738d to aa9d352 Compare December 10, 2025 16:00

findepi reviewed Dec 10, 2025

View reviewed changes

martint force-pushed the type-parameter branch from aa9d352 to 3ad5d93 Compare December 10, 2025 20:08

martint force-pushed the type-parameter branch from 3ad5d93 to ca6668b Compare December 10, 2025 21:21

findepi reviewed Dec 11, 2025

View reviewed changes

martint force-pushed the type-parameter branch from ca6668b to 7cd92ab Compare December 11, 2025 22:59

martint added 7 commits December 11, 2025 18:29

Get type arguments directly from the type

1bafbb7

TypeSignature should not be used in contexts where the Type is available. It's purpose is mainly for doing type inference and generic manipulation of types.

Remove TypeParameter

8186d7d

It's only used during instantiation of parametric types, as a mirror of TypeSignatureParameter. The translation is not needed.

Rename TypeSignatureParameter -> TypeParameter

047bcc0

Fix incorrect type declaration in test

8aea877

The type parameter must match the argument it corresponds to.

Remove NamedType and friends

43a53fd

It's not an ideal abstraction. Naming of fields is orthogonal to what type the argument is, so we don't need a dedicated class for it.

Deprecate Type.getTypeParameters()

b3365d6

For general use, it's preferable to call the type-specific methods (e.g., ArrayType.getElementType(), MapType.getKeyType, MapType.getValueType())

Improve type safety for TypeParameter

a4c9055

Use a sum type instead of a tag + value. It makes it possible to use deconstruction patterns and switch statements/expressions to manipulate the values of the type.

martint force-pushed the type-parameter branch from 7cd92ab to a4c9055 Compare December 12, 2025 16:54

Various cleanups and improvements in TypeParameter #27574

Are you sure you want to change the base?

Various cleanups and improvements in TypeParameter #27574

Conversation

martint commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release notes

Uh oh!

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

findepi Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

martint Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

findepi Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

martint Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

findepi Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

martint commented Dec 10, 2025

Uh oh!

martint commented Dec 10, 2025

Uh oh!

findepi commented Dec 11, 2025

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

findepi Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

martint Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

martint commented Dec 8, 2025 •

edited

Loading

martint Dec 11, 2025 •

edited

Loading