Map PostgreSQL JSON, JSONB to Presto JSON by guyco33 · Pull Request #81 · trinodb/trino

guyco33 · 2019-01-27T08:25:29Z

No description provided.

ebyhr · 2019-02-04T15:02:31Z

Whereas I could SELECT json from postgresql using this commit, CTAS failed with below message.
→Resolved by appending ?stringtype=unspecified to connection-url

2019-02-04T20:42:16.812+0545	ERROR	remote-task-callback-19	io.prestosql.execution.StageStateMachine	Stage 20190204_145716_00013_4tx9w.1 failed
io.prestosql.spi.PrestoException: Batch entry 0 INSERT INTO "test"."public"."tmp_presto_c00e7b22b456488693c2b5c1a50793d2" VALUES ('{"customer":"John Doe","items":{"product":"Beer","qty":6}}') was aborted: ERROR: column "c1" is of type jsonb but expression is of type character varying
  Hint: You will need to rewrite or cast the expression.
  Position: 83  Call getNextException to see other errors in the batch.
	at io.prestosql.plugin.jdbc.JdbcPageSink.finish(JdbcPageSink.java:186)
	at io.prestosql.operator.TableWriterOperator.finish(TableWriterOperator.java:193)
	at io.prestosql.operator.Driver.processInternal(Driver.java:397)
	at io.prestosql.operator.Driver.lambda$processFor$8(Driver.java:283)
	at io.prestosql.operator.Driver.tryWithLock(Driver.java:675)
	at io.prestosql.operator.Driver.processFor(Driver.java:276)
	at io.prestosql.execution.SqlTaskExecution$DriverSplitRunner.processFor(SqlTaskExecution.java:1075)
	at io.prestosql.execution.executor.PrioritizedSplitRunner.process(PrioritizedSplitRunner.java:162)
	at io.prestosql.execution.executor.TaskExecutor$TaskRunner.run(TaskExecutor.java:483)
	at io.prestosql.$gen.Presto_null__testversion____20190204_145200_1.run(Unknown Source)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.sql.BatchUpdateException: Batch entry 0 INSERT INTO "test"."public"."tmp_presto_c00e7b22b456488693c2b5c1a50793d2" VALUES ('{"customer":"John Doe","items":{"product":"Beer","qty":6}}') was aborted: ERROR: column "c1" is of type jsonb but expression is of type character varying
  Hint: You will need to rewrite or cast the expression.
  Position: 83  Call getNextException to see other errors in the batch.
	at org.postgresql.jdbc.BatchResultHandler.handleError(BatchResultHandler.java:148)
	at org.postgresql.core.ResultHandlerDelegate.handleError(ResultHandlerDelegate.java:50)
	at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2184)
	at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:481)
	at org.postgresql.jdbc.PgStatement.executeBatch(PgStatement.java:840)
	at org.postgresql.jdbc.PgPreparedStatement.executeBatch(PgPreparedStatement.java:1538)
	at io.prestosql.plugin.jdbc.JdbcPageSink.finish(JdbcPageSink.java:178)
	... 12 more
Caused by: org.postgresql.util.PSQLException: ERROR: column "c1" is of type jsonb but expression is of type character varying
  Hint: You will need to rewrite or cast the expression.
  Position: 83
	at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2440)
	at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2183)
	... 16 more

findepi · 2019-02-04T17:19:10Z

@ebyhr this might be related to https://github.com/prestosql/presto/pull/109/files#diff-c3182f661bb5ea4b0c08715a63db20c4R325

guyco33 · 2019-02-05T07:31:20Z

@ebyhr I already encountered this issue when I first used it to CTAS in postgres catalog and solved it by appending ?stringtype=unspecified to all postgres jdbc urls in the catalog config.
When CTAS into hive catalog I got different error: Unsupported Hive type: json but it makes sense since there is no Json type in hive.

ebyhr · 2019-02-05T08:39:49Z

Sorry, my update's notication might have not been sent you. I tried same thing as @guyco33 commented, it works in my environment too.

guyco33 · 2019-02-13T09:20:57Z

@findepi Now after merging #109 the pushdown predicate in QueryBuilder (https://github.com/prestosql/presto/pull/109/files#diff-88e8a0a25b51f372e2a50ab027085b33) picks also JSON types (previously it was prevented by QueryBuilder#isAcceptedType) and it fails since JSON is not orderable. I think to make JSON orderable or to skip QueryBuilder#toPredicate for types that are not orderable

findepi

Some initial comments.
We will need @electrum review for in/out conversions around JsonType, but please apply my feedback first. Please do all new changes as fixups, so i can re-review (i'll let you know when you can squash them).

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/JdbcTypeHandle.java

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/PostgreSqlQueryRunner.java

findepi · 2019-02-13T09:51:47Z

presto-spi/src/main/java/io/prestosql/spi/type/JsonType.java

I recall @electrum's comment saying the JsonType should not be moved to SPI

prestodb/presto#11913 (comment)

(i didn't review this class, i think there were some changes that would require review)

@findepi Should I create a new JsonType in SPI and use the signature to correlate them ?

findepi · 2019-02-13T09:58:14Z

presto-postgresql/src/main/java/io/prestosql/plugin/postgresql/PostgreSqlClient.java

I assume we don't want to push down JSON predicates. Otherwise this requires extensive testing that this does not change query semantics, while this is likely not very needed feature.

To disable pushdown, use ColumnMapping.sliceMapping overload which takes UnaryOperator<Domain> pushdownConverter and pass domain -> Domain.all(domain.getType()) as the converter. Or better, rebase on #225 and use DISABLE_PUSHDOWN constant.

I disabled pushdown and SELECT * FROM test_json where json_column = json'{"x":123}' still fails on Domain type must be orderable

@findepi It works while skipping builder.add(toPredicate(column.getColumnName(), domain, column, accumulator)) in io.prestosql.plugin.jdbc.QueryBuilder when domain type is not orderable:

private List<String> toConjuncts(JdbcClient client, ConnectorSession session, List<JdbcColumnHandle> columns, TupleDomain<ColumnHandle> tupleDomain, List<TypeAndValue> accumulator) { ImmutableList.Builder<String> builder = ImmutableList.builder(); for (JdbcColumnHandle column : columns) { Domain domain = tupleDomain.getDomains().get().get(column); if (domain != null) { domain = pushDownDomain(client, session, column, domain); if (domain.getType().isOrderable()) { builder.add(toPredicate(column.getColumnName(), domain, column, accumulator)); } } } return builder.build(); }

I filed a ticket -- #238

findepi · 2019-02-13T10:04:32Z

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java

Suggested change

value -> format("JSON'%s'", value),

value -> format("JSON '%s'", value),

This actually applies to in a few places in your test queries. Also, by a convention, we always uppercase type name in type constructors like JSON '....' rather than json '...' (or json'...'.)

presto-tests/src/main/java/io/prestosql/tests/H2QueryRunner.java

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java

findepi

PIng me when you address all my comments (or when you feel blocked).

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java

findepi · 2019-02-14T21:23:37Z

presto-postgresql/src/main/java/io/prestosql/plugin/postgresql/PostgreSqlClient.java

replace jsonPushdown() method with io.prestosql.plugin.jdbc.ColumnMapping#DISABLE_PUSHDOWN (static-import it)

here you're changing domain's type. Why so?

I see the reason now. Can you check if #238 is sufficient? I didn't test that change with your code.

Your fix works for me. Thanks!

findepi · 2019-02-14T21:26:42Z

presto-postgresql/src/main/java/io/prestosql/plugin/postgresql/PostgreSqlClient.java

nit: format with each arg on separate line, for readability

ColumnMapping.sliceMapping( JSON, (resultSet, columnIndex) -> jsonParse(utf8Slice(resultSet.getString(columnIndex))), jsonWriteFunction(), jsonPushdown());

findepi · 2019-02-14T21:41:28Z

Thanks for the fixups, that was helpful. You can squash what you have so far.
Then we should move JsonType back to where it was. I will defer to @electrum on that one.

findepi

Please squash the fixups you have so far.

findepi · 2019-02-15T07:54:25Z

presto-postgresql/src/main/java/io/prestosql/plugin/postgresql/PostgreSqlClient.java

Suggested change

domain -> Domain.all(domain.getType()));

DISABLE_PUSHDOWN);

electrum · 2019-02-15T17:57:14Z

Sorry for the confusion and delay on this PR. We'd like to get this in, but there are two main things to resolve:

We shouldn't need to add the JSON type to the SPI. The intended way for connectors to access types is by looking them up via TypeManager. The name is available by StandardTypes.JSON. Instance checks can be converted to type.getTypeSignature().getBase().equals(StandardTypes.JSON).
For constructing the object value for the JSON type, we should use the JSON type constructor. We need to expose this to the SPI. I will update [WIP] Add explicit support for type constructors #245 to do that after the massive function change Redesign function management abstraction #196 is landed, since it will conflict.

Please update the commit title to "Map PostgreSQL JSON to Presto JSON"

electrum · 2019-02-15T22:46:01Z

For the JSON type constructor, it will likely be several weeks before the function and constructor PRs will land, so we can move forward on this PR using the duplicated code (and clean it up later).

We do still need to revert the part about moving JsonType into the SPI.

To get access to TypeManager:

Add it as a constructor parameter to PostgreSqlClient.
Change JdbcConnectorFactory to bind it in Guice:

Bootstrap app = new Bootstrap(
        binder -> binder.bind(TypeManager.class).toInstance(context.getTypeManager()),
        new JdbcModule(catalogName),
        module);

Then in the PostgreSqlClient constructor:

this.jsonType = typeManager.getType(new TypeSignature(StandardTypes.JSON));

electrum · 2019-02-15T22:49:42Z

@guyco33 Thanks for your work on this so far. Please let me know when you've updated the PR so that we can do a final review and merge. Feel free to ping me on Slack if you have any questions.

electrum · 2019-02-18T06:54:04Z

presto-main/src/main/java/io/prestosql/testing/MaterializedResult.java

The comparison below needs to be against JsonType.JSON

electrum

A few minor comments, otherwise this looks good. Please address the comments and squash into a single commit.

electrum · 2019-02-18T06:57:39Z

presto-postgresql/src/main/java/io/prestosql/plugin/postgresql/PostgreSqlClient.java

Make this protected final (no static) since it is initialized in the constructor

I need it to be static since it used in private static ColumnMapping jsonColumnMapping()

Make that method non-static. Initializing a static field from a constructor is problematic.

Right, got it, thanks!

electrum · 2019-02-18T06:59:08Z

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java

Why the 2 at the end? I don't see any other usages of this name

tpch.postgresql_test_json used to be there before :) Fixed

electrum · 2019-02-18T06:59:39Z

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java

Remove //

electrum · 2019-02-18T06:59:57Z

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java

Nit: add space after , and lowercase column and table names (but keep SQL keywords in uppercase)

electrum · 2019-02-18T07:01:45Z

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java

Nit: static import dataType and identity

electrum · 2019-02-18T07:02:10Z

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java

This doesn't work if the value has single quotes, but that's probably fine for this test

value -> { checkArgument(!value.contains("'")); return format("JSON '%s'", value); }

electrum · 2019-02-18T07:02:22Z

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java

Make this private since it's not used elsewhere

electrum · 2019-02-18T07:02:51Z

presto-tests/src/main/java/io/prestosql/tests/H2QueryRunner.java

You can use JsonType.JSON here since it is available to test code

electrum · 2019-02-18T07:03:15Z

presto-tests/src/main/java/io/prestosql/tests/H2QueryRunner.java

Nit: static import jsonParse

electrum · 2019-02-18T07:03:37Z

presto-tests/src/main/java/io/prestosql/tests/TestingPrestoClient.java

Same, this can use JsonType.JSON

electrum

This is ready to merge. GitHub is showing a merge conflict, please rebase and then I'll merge it.

electrum · 2019-02-23T01:51:38Z

presto-postgresql/src/main/java/io/prestosql/plugin/postgresql/PostgreSqlClient.java

I don't think we need to check the JDBC type here. The switch on the type name should be sufficient.

findepi · 2019-02-23T20:17:44Z

@guyco33 congrats!

findepi requested a review from electrum January 27, 2019 15:28

guyco33 force-pushed the add_support_for_postgres_json branch from d80ab2c to 2e0f193 Compare February 1, 2019 09:55

cla-bot bot added the cla-signed label Feb 4, 2019

trinodb deleted a comment from cla-bot bot Feb 4, 2019

guyco33 force-pushed the add_support_for_postgres_json branch 3 times, most recently from 5085008 to a9bad83 Compare February 12, 2019 14:49

findepi reviewed Feb 13, 2019

View reviewed changes

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java Outdated Show resolved Hide resolved

findepi reviewed Feb 14, 2019

View reviewed changes

findepi reviewed Feb 15, 2019

View reviewed changes

guyco33 force-pushed the add_support_for_postgres_json branch from 00c2319 to 6748f0b Compare February 15, 2019 09:37

findepi assigned electrum Feb 15, 2019

guyco33 changed the title ~~map postgres json to presto json~~ Map PostgreSQL JSON to Presto JSON Feb 15, 2019

electrum reviewed Feb 18, 2019

View reviewed changes

guyco33 force-pushed the add_support_for_postgres_json branch from a58aed2 to 8b27ae4 Compare February 18, 2019 16:12

electrum approved these changes Feb 23, 2019

View reviewed changes

Map PostgreSQL JSON to Presto JSON

653ee44

guyco33 force-pushed the add_support_for_postgres_json branch from 8b27ae4 to 653ee44 Compare February 23, 2019 10:27

electrum merged commit 1f70a1b into trinodb:master Feb 23, 2019

electrum mentioned this pull request Feb 24, 2019

Release notes for 304 #235

Closed

6 tasks

findepi changed the title ~~Map PostgreSQL JSON to Presto JSON~~ Map PostgreSQL JSON, JSONB to Presto JSON Jun 28, 2019

	value -> format("JSON'%s'", value),
	value -> format("JSON '%s'", value),

Conversation

guyco33 commented Jan 27, 2019

Uh oh!

ebyhr commented Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

findepi commented Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guyco33 commented Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ebyhr commented Feb 5, 2019

Uh oh!

guyco33 commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

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!

findepi commented Feb 14, 2019

Uh oh!

findepi 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!

electrum commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

electrum commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

electrum commented Feb 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

electrum left a comment

Choose a reason for hiding this comment

ebyhr commented Feb 4, 2019 •

edited

Loading

findepi commented Feb 4, 2019 •

edited

Loading

guyco33 commented Feb 5, 2019 •

edited

Loading

guyco33 commented Feb 13, 2019 •

edited

Loading

electrum commented Feb 15, 2019 •

edited

Loading

electrum commented Feb 15, 2019 •

edited

Loading