add jdbc mapping for postgres json by guyco33 · Pull Request #11913 · prestodb/presto

guyco33 · 2018-11-13T10:36:27Z

relates to issue #11821

With this PR postgres native json & jsonb types are mapped to presto json type

findepi

Thanks for the PR!

i have a couple of initial comments.

presto-postgresql/src/main/java/com/facebook/presto/plugin/postgresql/PostgreSqlClient.java

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcTypeHandle.java

presto-postgresql/src/main/java/com/facebook/presto/plugin/postgresql/PostgreSqlClient.java

findepi · 2018-11-13T14:42:50Z

presto-postgresql/src/main/java/com/facebook/presto/plugin/postgresql/PostgreSqlClient.java


+    public static ReadMapping jsonReadMapping()
+    {
+        return sliceReadMapping(JSON, (resultSet, columnIndex) -> utf8Slice(resultSet.getString(columnIndex)));


Looking at impl of = for JSON type (com.facebook.presto.operator.scalar.JsonOperators#equals), the value should be normalized. At least i would naively expect that {'a': 1, 'b': 2} = {'b': 2, 'a': 1}.

You probably need to do something like cast(some_varchar as json) is doing -- see com.facebook.presto.operator.scalar.JsonOperators#castFromVarchar. Probably the impl of the cast should be moved to utility method (somewhere) and reused.

@electrum json is not part of SPI. How do you think this should be addressed?

We should be able to do this with TypeManager from ConnectorContext by resolving the CAST operator. This will require a small change to resolveOperator() (or adding a new method ... need to get input from @martint and others on this):

@Override public MethodHandle resolveOperator(OperatorType operatorType, List<? extends Type> argumentTypes) { requireNonNull(functionRegistry, "functionRegistry is null"); Signature signature; if (operatorType == OperatorType.CAST) { checkArgument(argumentTypes.size() == 2, "CAST requires exactly two arguments"); signature = functionRegistry.getCoercion(argumentTypes.get(0), argumentTypes.get(1)); } else { signature = functionRegistry.resolveOperator(operatorType, argumentTypes); } return functionRegistry.getScalarFunctionImplementation(signature).getMethodHandle(); }

...ostgresql/src/test/java/com/facebook/presto/plugin/postgresql/TestPostgreSqlTypeMapping.java

findepi · 2018-11-13T14:44:55Z

presto-postgresql/src/main/java/com/facebook/presto/plugin/postgresql/PostgreSqlClient.java

+                case "jsonb":
+                case "json":
+                    return Optional.of(jsonReadMapping());
+            }


Will predicate-pushdown work correctly with json? We definitely need some tests for that.

Can you please elaborate about this and add some references to similar tests ?

findepi · 2018-11-13T14:45:20Z

presto-spi/src/main/java/com/facebook/presto/spi/type/JsonType.java

+/**
+ * The stack representation for JSON objects must have the keys in natural sorted order.
+ */
+public class JsonType


Is this a copy of com.facebook.presto.type.JsonType?

yes. I copied it to spi to be aligned with other types

@findepi
Somehow I need to merge them in away that json column will be of type com.facebook.presto.spi.type.JsonType

done by extending com.facebook.presto.type.JsonType from com.facebook.presto.spi.type.JsonType and setting it's static JSON attribute to be com.facebook.presto.spi.type.JsonType.JSON

electrum · 2018-11-18T23:18:10Z

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcPageSink.java

We don't need the type instance to be in the SPI. Instead, compare like this:

type.getTypeSignature().getBase().equals(StandardTypes.JSON)

findepi · 2018-11-19T08:58:36Z

presto-postgresql/src/main/java/com/facebook/presto/plugin/postgresql/PostgreSqlClient.java

@electrum with JsonType not being in SPI, how can a connector know this is the correct runtime representation of the JsonType?
We certainly don't need to move JsonType to SPI, but we probably need to have some SPI factory method (or other SPI-level contract) for this.

My first thought was to resolve the CAST operator using TypeManager. However, cast from varchar to json is actually different from the type constructor. Maybe we should add a method to lookup the type constructor?

@martint thoughts?

Sill question -- why is the cast from varchar different from type constructor? Is't not intuitive.

Casting a varchar to JSON results in a JSON string. The type constructor takes a JSON literal and is equivalent to json_parse(). I agree it's confusing, but my memory is that it works like that so that it's consistent when done recursively. For example, casting varchar and array(varchar) work the same way.

See here for a bit more detail: https://prestodb.io/docs/current/functions/json.html#json_parse

I just put up a PR to allow resolving the type constructor: #12007

@electrum Can you explain how can I use it from PostgreSqlClient ?
Currently I am using a copied version of jsonParse jsonParse(utf8Slice(resultSet.getString(columnIndex)))), since it's not exposed in the spi. Same for JsonType that is needed by com.facebook.presto.plugin.jdbc.ReadMapping

guyco33 · 2018-12-14T10:38:14Z

presto-postgresql/src/main/java/com/facebook/presto/plugin/postgresql/PostgreSqlClient.java

@findepi
Any idea how can I use com.facebook.presto.operator.scalar.JsonFunctions#jsonParse from here without the need to duplicate code ?
Same for com.facebook.presto.type.JsonType that is needed by ReadMapping

…ative json/jsonb and presto native json

facebook-github-bot added the CLA Signed label Nov 13, 2018

findepi reviewed Nov 13, 2018

View reviewed changes

guyco33 force-pushed the add_support_for_postgres_json branch from 1289bcf to 3786c36 Compare November 14, 2018 13:00

electrum reviewed Nov 18, 2018

View reviewed changes

findepi reviewed Nov 19, 2018

View reviewed changes

guyco33 force-pushed the add_support_for_postgres_json branch 2 times, most recently from b9e746b to 61a22b7 Compare November 24, 2018 20:26

guyco33 force-pushed the add_support_for_postgres_json branch from 61a22b7 to 09913cc Compare November 29, 2018 18:11

guyco33 commented Dec 14, 2018

View reviewed changes

guyco33 force-pushed the add_support_for_postgres_json branch 3 times, most recently from 794b218 to 3345ded Compare December 20, 2018 12:26

findepi mentioned this pull request Dec 29, 2018

Treat other SQL types as varchar #12141

Closed

guyco33 force-pushed the add_support_for_postgres_json branch from 3345ded to 3decb18 Compare January 3, 2019 09:34

guyco33 force-pushed the add_support_for_postgres_json branch from 3decb18 to dae7d0e Compare January 17, 2019 12:22

guyco33 added 3 commits January 30, 2019 16:23

add jdbc mapping for postgres json

5836837

add roundtrip tests for json data

813c43f

add jsonParse in readMapping to support proper equality of postgres n…

6a88688

…ative json/jsonb and presto native json

guyco33 force-pushed the add_support_for_postgres_json branch from dae7d0e to 6a88688 Compare January 30, 2019 14:23

This was referenced Feb 7, 2019

Add an optional JDBC connector type mapping to varchar trinodb/trino#186

Closed

Map PostgreSQL JSON, JSONB to Presto JSON trinodb/trino#81

Merged

guyco33 closed this Jun 29, 2019

Conversation

guyco33 commented Nov 13, 2018

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

guyco33 Dec 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

guyco33 Dec 9, 2018 •

edited

Loading