Allow user to control how unsupported JDBC type should be handled

[kokosing]

No description provided.

[kokosing]

Better version of #1105

[findepi]

This could be extracted to a helper method:

checkNull(block, position);

[findepi]

I think we need a session toggle here. Otherwise SELECT * .. LIMIT 1 isn't going to work.
Add tests for the session toggle.

Also, no INSERT statements are going to work (even ones specifying column names), due to #1185.

[kokosing]

I will address that in separate commit.

[findepi]

You can assume table creation is always in the form of CREATE TABLE table_name ....
See how @kokosing did this in io.prestosql.plugin.sqlserver.TestSqlServerIntegrationSmokeTest#withTable

[kokosing]

I prefer to use TestTable because it generates unique name. Here, it is not a problem, however recently I got several issues with tests that are using non unique name so I try to push the new convention.
Once you have an unique name you need something more than AutoCloseable.

[findepi]

That's orthogonal. You can return TestTable that a) is autocloseable and b) has generated name, all without having people ever touch {TABLE_NAME} constant

[kokosing]

Updated. {TABLE_NAME} got removed.

[findepi]

You can try do this without nested try-catch, i.e. try with try:

catch (Exception e) {
   try (AutoCloseable self = this) {
      throw e;
   }
}

[findepi]

This doesn't add to readability.
For now, i'd just use TABLE_NAME_PLACEHOLDER value directly, so without need to string format, like in https://github.com/prestosql/presto/blob/c74b775e103a7a239aafef1b7ca15fe6370495fe/presto-tests/src/main/java/io/prestosql/tests/datatype/CreateAsSelectDataSetup.java#L48

Later, i'd prefer to follow @kokosing 's https://github.com/prestosql/presto/blob/3f1d5d4cd0576ec012ea388055546816af0fdef5/presto-sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestSqlServerIntegrationSmokeTest.java#L72-L74

[kokosing]

Applied the first part.

[findepi]

Define ctor where data is String without Optional

[findepi]

Can you use some more esoteric data type? We should support PostgreSQL's GEOMETRY.

[kokosing]

Here I am using testing H2. Other types would need to add some plugins to h2. Do you think that presto-base-jdbc is going to support GEOMETRY some day?

[findepi]

This should succeed. #1185
Add a TODO

[kokosing]

Addressed comments. Also add session properties to ignore and convert unsupported types to varchar.

[kokosing]

Here I am using testing H2. Other types would need to add some plugins to h2. Do you think that presto-base-jdbc is going to support GEOMETRY some day?

[kokosing]

Applied the first part.

[kokosing]

I will address that in separate commit.

[kokosing]

I prefer to use TestTable because it generates unique name. Here, it is not a problem, however recently I got several issues with tests that are using non unique name so I try to push the new convention.
Once you have an unique name you need something more than AutoCloseable.

[kokosing]

@findepi ping

[electrum]

The unknown type part needs @martint to review it. My memory is that this is an internal detail which shouldn’t be exposed.

[findepi]

I think IGNORE should be the default.

[findepi]

Regarding the default behavior (https://github.com/prestosql/presto/pull/1182/files#r309276154), let me quote my comment from previous PR (#1105 (comment)):

I propose a different new behavior:

• expose all columns, including those unsupported (perhaps as unknown type)
• fail query only when it accesses unsupported columns (either explicitly, or SELECT *)

Regarding default behavior, there are different use-cases for SELECT *:

1. copying/migrating data (usually CREAT TABLE .. AS SELECT * FROM ..)

   • in this case, the new behavior is more appropriate

2. poking around (usually SELECT * FROM .. LIMIT 1)

   • in this case, the current behavior is more appropriate

Since current behavior is more appropriate in certain use-cases, I would not change current default behavior.

[kokosing]

The unknown type part needs @martint to review it. My memory is that this is an internal detail which shouldn’t be exposed.

Notice that:

presto:tiny> select typeof(null);
  _col0
---------
 unknown
(1 row)

To me it is like regular type, which may have only single value null.

[kokosing]

I think IGNORE should be the default.

@findepi done.

[findepi]

cc @guyco33 . In particular the CONVERT_TO_VARCHAR option. This is related to yours #186

[findepi]

@kokosing Regarding CONVERT_TO_VARCHAR option, please see previous discussion at #186 > "future-compatibility".

[findepi]

inline sessionProperties

[findepi]

Why not handle the conversion in jdbcClient.toPrestoType?

[findepi]

columnMapping.orElseGet ?

[findepi]

Add

assertQuery("SELECT * FROM " + table.getName(), "VALUES 1");

[findepi]

from... from where?

[findepi]

use more descriptive column names, eg regular_column, unsupported_column.

[findepi]

Please add a test
SELECT * FROM .. WHERE x = 'wrong value'

make sure PPD is disbaled then

[kokosing]

@findepi Please notice that even if we have jdbc-types-mapped-to-varchar then unsupported-type-handling=CONVERT_TO_VARCHAR is still useful for cases where underlying type does not fit Presto type. Like DECIMAL(180,40), where user would like to keep regular DECIMAL mapping but handle this particular type as VARCHAR.

[guyco33]

Would be helpful for #2088
Users will be able to query DECIMAL column values of precision larger than 38

[kokosing]

This is handled in toPrestoType. Here we should validate:

default:
checkState(unsupportedTypeHandling != CONVERT_TO_VARCHAR);

[findepi]

LGTM

@electrum do you want to check the piece around unknown type and SPI?

[findepi]

Should be sufficient to add a const to StandardTypes instead of moving the class

[findepi]

incl whole typeHandle

also you can pull column name from io.prestosql.plugin.jdbc.JdbcOutputTableHandle#columnNames

you can inline columnMapping now

[findepi]

"Unsupported data type for column: "

[findepi]

MAP_TO_UNKNOWN ?

[findepi]

null -> NULL

[findepi]

typo: tt

[findepi]

rebase

[findepi]

old version; rebase

[findepi]

as a test in postgres, with decimal(129567) or sth

[findepi]

i am not convinced this is a needed functionality. let's discuss

[kokosing]

Applied comments. I removed options to map to unknown type and failing the query. Let's us revisit that later if needed.

[findepi]

this needs to move too

[findepi]

"Unsupported type handling is set to %s, but toPrestoType() returned empty"

also

verify(columnMapping.isPresent() || getUnsupportedTypeHandling(session) == IGNORE, .. so that you can place it above, without else

[findepi]

"is force mapped to" is not applicable wording when type is unsupported

[findepi]

"unsupported type" -> "an unsupported type"

or, rather, just "Unsupported type handling strategy", as in sess prop desc

[findepi]

revert? i think this should be "unreachable" now

[findepi]

why the code here is so different than in super.?

[findepi]

private; this class is not expected to be extended

[findepi]

private

[findepi]

could what?

[findepi]

withUnsupportedType ?

[kokosing]

Addressed comments. Thank you!

[guyco33]

use import static com.google.common.base.Verify.verify