Fix pushdown filter for PostgreSQL enum type

[guyco33]

Filters on PostgreSQL enum type are failing since they are pushdown to connector (enum type is considered as a Jdbc VARCHAR type:
JdbcTypeHandle.jdbcType=java.sql.Types.VARCHAR ,JdbcTypeHandle.jdbcTypeName=<name of user-defined enum type>)

In order to fix it and keep the pushdown, a cast to the proper typeName is implemented by passing a proper bindingPlaceholder

[ebyhr]

You can retrieve JdbcColumnHandle from 4th argument JdbcColumnHandle column. Why don't you reuse the exising argument?

[guyco33]

Here it's just the function's signature. I am using the actual 4th argument in the apply in the return clause

[ebyhr]

Sorry, my comment was not enough. I meant we can change the type to Optional<String> bindingPlaceholder or boolean and then change like below. This is just my idea, therefore let's wait for other member's review. I understand your implementation is opened for future changes.

bindingPlaceholder.map(bind -> bind + column.getJdbcTypeHandle().getJdbcTypeName()).orElse("?")

[guyco33]

Function<JdbcColumnHandle, String> is much generic and the connector can also pass function that results with something like this: "cast(? as typeName)"

[ebyhr]

I agree with the generics. I wanted to avoid optional-functional argument. Let's follow kokosing's suggestion.

[kokosing]

• No point to pass Optional here. You can always pass function that always return "?"
• Can you make it a field of QueryBuilder, this function should not changed between queries, right?

[guyco33]

Make sense. Thanks @kokosing . done

[kokosing]

private final

[kokosing]

requireNonNull

[kokosing]

extra ( and ), please remove

[kokosing]

please extract (columnHandle) -> "?::" + columnHandle.getJdbcTypeHandle().getJdbcTypeName() as method, then replace it with this::yourNewMethodName

[kokosing]

Please format this like:

            assertQuery(
                "SELECT column_name, data_type FROM information_schema.columns WHERE table_schema = 'tpch' AND table_name = 'test_enum'",
                "VALUES ('id','integer'),('col_s','varchar'),('col_e','varchar')");

[findepi]

@guyco33 I think we should investigate alternatives within existing framework before committing to this approach.
According to jdbi/jdbi#420 (comment) you should be able to bind enum value using java.sql.PreparedStatement#setObject(int, java.lang.Object, int) with java.sql.Types.OTHER. There might be also other ways, like what we do with json in https://github.com/prestosql/presto/blob/371bc0b1a533d7e6a43705f6dcb2a3bb24e7fb85/presto-postgresql/src/main/java/io/prestosql/plugin/postgresql/PostgreSqlClient.java#L151-L154

[guyco33]

Thanks @findepi for your feedback!

How are we going to filter enum types in PostgreSqlClient#toPrestoType?
I came up with this:
typeHandle.getJdbcType() == Types.VARCHAR && !typeHandle.getJdbcTypeName().equals("varchar")

I've pushed a fixup to illustrate it

[findepi]

Thanks @guyco33!
Looks better to me and lower risk of side effects.

Some comments. Then we need to look into how to pull "is enum" information more directly.
Can you please check what are the other types that show up as Types.VARCHAR in Postgres?

[findepi]

Call it typedVarcharWriteFunction. Name is not perfect, but at least it signals some varchar-like Slice and UTF-8 decoding takes place.

[findepi]

just typedStringWriteFunction("json"), there is no need to use jdbcTypeName here

[findepi]

This was writing JSON as "json", not "jsonb".
Please use "json". If your change is intentional, please extract to a separate commit

[findepi]

Can you revert changes related to "bindingPlaceholder"?

[findepi]

For now add explanatory comment here:

// This can be e.g. an ENUM

[findepi]

Once you cleanup, I will see whether we can pull "is enum" information in some better way.

[findepi]

revert

[findepi]

call it typedVarcharColumnMapping

[findepi]

@guyco33 good job!

I looked into how to improve/replace if (typeHandle.getJdbcType() == Types.VARCHAR && !typeHandle.getJdbcTypeName().equals("varchar")), but neither information_schema.columns nor pg_attribute didn't seem to explicitly say "this is enum".
Let's leave it as it is.

Several minor comments in the test code. Otherwise LGTM.
Please squash everything into single commit.

[findepi]

remove column col_s

[findepi]

insert another data row

[findepi]

rename col_e to enum_column

[findepi]

nit: upper case "AS ENUM" as in Postgres docs https://www.postgresql.org/docs/11/datatype-enum.html

[guyco33]

I looked into how to improve/replace if (typeHandle.getJdbcType() == Types.VARCHAR && !typeHandle.getJdbcTypeName().equals("varchar")), but neither information_schema.columns nor pg_attribute didn't seem to explicitly say "this is enum".

select * from pg_type where typcategory = 'E' and typname='enum_t'

[findepi]

Merged, thanks!