Write mapping support by infvg · Pull Request #25124 · prestodb/presto

infvg · 2025-05-15T13:08:14Z

Description

Currently, write mapping is hard coded in JdbcPageSink.appendColumn, making it difficult to add additional write support. This PR adds WriteMapping to match the ReadMapping functionality & make it possible to add additional type support on a per connector basis.

Motivation and Context

It will allow us to support additional types on a per connector basis

Impact

None

Test Plan

Unit tests

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

== RELEASE NOTES ==

JDBC Driver Changes
* Improve type mapping API to add WriteMapping functionality.

ZacBlanco

A few initial comments

.gitignore

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

github-actions · 2025-05-22T08:34:15Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 91a8685...1218d8c.

No notifications.

pratyakshsharma

Thank you for raising the PR @infvg
Taken first pass and have few comments. Please let me know if something remains unclear

pratyakshsharma · 2025-05-27T11:06:35Z

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

-            .put(TIMESTAMP_WITH_TIME_ZONE, "timestamp with timezone")
-            .put(UuidType.UUID, "uuid")
+    private static final Map<Type, WriteMapping> TYPE_MAPPINGS = ImmutableMap.<Type, WriteMapping>builder()
+            .put(BOOLEAN, booleanMapping("boolean", (BooleanWriteFunction) booleanColumnMapping().getWriteFunction()))


Are these explicit casts to respective writeFunctions needed?

Yes these are the default write functions for these types & will be used when getting a write function for a specific type.

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

pratyakshsharma · 2025-05-27T11:13:55Z

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

        }
        else {
-            throw new PrestoException(NOT_SUPPORTED, "Unsupported column type: " + type.getDisplayName());
+            ((ObjectWriteFunction) writeFunction).set(statement, parameter, type.getObject(block, position));


Wondering if this can cover all other types properly and if we should wrap this in try-catch with a proper user facing error?

read Object flow is wrapped in try-catch already.

pratyakshsharma · 2025-05-27T11:21:32Z

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

            else {
-                throw new UnsupportedOperationException("Can't handle type: " + typeAndValue.getType());
+                //no inspection, unchecked raw types
+                ((ObjectWriteFunction) writeFunction).set(statement, parameterIndex, value);


Need a try-catch here as well

pratyakshsharma · 2025-05-27T13:10:21Z

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

+    public static ColumnMapping sliceMapping(Type prestoType, SliceReadFunction readFunction, SliceWriteFunction writeFunction)
+    {
+        return new ColumnMapping(prestoType, readFunction, writeFunction);
+    }


ColumnMapping for objects is missing. Please add that and move the class variables above the methods.

Added a column mapping method - although currently it will be unused since none of the current supported types need it (postgresql array type will need it in the future).

pratyakshsharma · 2025-05-27T13:12:04Z

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

+        this.writeFunction = requireNonNull(writeFunction, "writeFunction is null");
+        checkArgument(
+                type.getJavaType() == readFunction.getJavaType(),
+                "Presto type %s is not compatible with read function %s returning %s",


nit: returning -> using

I think this is still not done. Please double check.

My bad seems like it didn't get added to the commit - fixed.

pratyakshsharma · 2025-05-27T13:12:11Z

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

+                readFunction.getJavaType());
+        checkArgument(
+                type.getJavaType() == writeFunction.getJavaType(),
+                "Presto type %s is not compatible with write function %s returning %s",


pratyakshsharma · 2025-05-28T08:51:43Z

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

    {
        super(connectorId, config, "\"", new DriverConnectionFactory(new Driver(), config));
        this.jsonType = typeManager.getType(new TypeSignature(StandardTypes.JSON));
-        this.uuidType = typeManager.getType(new TypeSignature(StandardTypes.UUID));


why are we removing this? I think you mentioned something about this during our discussion that I cannot recall now.

This was needed for to create the columnMapping for UUID types but since that is already in StandardColumnMappings we can remove the redundant implementation:
https://github.com/prestodb/presto/pull/25124/files#diff-625237786d8bc595087896db71d758467722c6dfeb8556e48c99420f96d5c842R125

pratyakshsharma · 2025-05-28T08:54:11Z

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

 import static com.facebook.presto.common.type.SmallintType.SMALLINT;
 import static com.facebook.presto.common.type.TimeType.TIME;
 import static com.facebook.presto.common.type.TimeWithTimeZoneType.TIME_WITH_TIME_ZONE;
-import static com.facebook.presto.common.type.TimestampType.TIMESTAMP;


Did we miss adding support for this since you have added support for TIME, TIME_WITH_TIME_ZONE and TIMESTAMP_WITH_TIME_ZONE.?

Yeah I didn't see any implementations for them in the original (although I don't think they had proper support back then so it probably was not missed, just not needed).

Right, all of them were not supported back then. Since you added support for some of them now, was just checking to see if you missed TIMESTAMP somehow

Ah, I see - Timestamp types require some type info to get the correct write function so the implementation for timestamp is here:
https://github.com/prestodb/presto/pull/25124/files#diff-a585323eb9886bcb1569de59ffc9ba78fb29e5da66b43666aa0072a8de747172R816

pratyakshsharma · 2025-06-06T21:17:18Z

@ZacBlanco can you please take another pass on this

Co-authored-by: pratyakshsharma <pratyaksh13@gmail.com>

pratyakshsharma · 2025-06-17T14:44:30Z

@tdcmeehan @aaneja @ZacBlanco can you guys take a pass and approve this?

author

prestodb-ci · 2025-06-18T04:48:14Z

@ethanyzhang imported this issue as lakehouse/tracker #25124

prestodb-ci added the from:IBM PR from IBM label May 15, 2025

infvg force-pushed the write-mapping branch from 03383ef to fdb0d69 Compare May 15, 2025 16:36

infvg marked this pull request as ready for review May 15, 2025 16:38

infvg requested a review from a team as a code owner May 15, 2025 16:38

infvg requested a review from ZacBlanco May 15, 2025 16:38

prestodb-ci requested review from a team, NivinCS and bibith4 and removed request for a team May 15, 2025 16:38

infvg force-pushed the write-mapping branch 4 times, most recently from 50a7742 to e7b8684 Compare May 15, 2025 17:13

infvg requested a review from pratyakshsharma May 15, 2025 17:13

infvg force-pushed the write-mapping branch 8 times, most recently from b03db2f to de13a86 Compare May 20, 2025 09:16

infvg changed the title ~~[DNR] Write mapping support~~ Write mapping support May 20, 2025

ZacBlanco reviewed May 21, 2025

View reviewed changes

infvg force-pushed the write-mapping branch 2 times, most recently from c065212 to 84ded12 Compare May 22, 2025 08:31

infvg force-pushed the write-mapping branch 2 times, most recently from 3af817a to 5f8e951 Compare May 25, 2025 08:15

pratyakshsharma previously requested changes May 28, 2025

View reviewed changes

infvg force-pushed the write-mapping branch 2 times, most recently from 3b33147 to 5e75eb4 Compare May 28, 2025 13:10

ZacBlanco previously approved these changes Jun 9, 2025

View reviewed changes

pratyakshsharma force-pushed the write-mapping branch from 9651460 to f0d25c6 Compare June 10, 2025 16:20

Add write mapping

1218d8c

Co-authored-by: pratyakshsharma <pratyaksh13@gmail.com>

pratyakshsharma dismissed ZacBlanco’s stale review via 1218d8c June 10, 2025 16:32

pratyakshsharma force-pushed the write-mapping branch from f0d25c6 to 1218d8c Compare June 10, 2025 16:32

aaneja approved these changes Jun 17, 2025

View reviewed changes

tdcmeehan approved these changes Jun 17, 2025

View reviewed changes

NivinCS approved these changes Jun 18, 2025

View reviewed changes

aaneja merged commit f19fee9 into prestodb:master Jun 18, 2025
97 checks passed

sebastianopeluso mentioned this pull request Jun 19, 2025

Revert #25124 Write mapping support #25369

Merged

6 tasks

unidevel mentioned this pull request Jun 19, 2025

Add release notes for 0.294 unix280/presto#33

Closed

6 tasks

zation99 mentioned this pull request Jun 30, 2025

Add a JDBC write mapping interface #25437

Merged

6 tasks

This was referenced Jul 4, 2025

Add release notes for 0.294 unix280/presto#35

Merged

Add release notes for 0.294 unix280/presto#36

Closed

Add release notes for 0.294 unix280/presto#37

Closed

This was referenced Jul 24, 2025

Add release notes for 0.294 unix280/presto#39

Merged

Add release notes for 0.294 unix280/presto#40

Merged

prestodb-ci mentioned this pull request Jul 28, 2025

Add release notes for 0.294 #25633

Merged

6 tasks

Conversation

infvg commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pratyakshsharma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

infvg May 28, 2025 • 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!

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!

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!

pratyakshsharma commented Jun 6, 2025

Uh oh!

pratyakshsharma commented Jun 17, 2025

Uh oh!

prestodb-ci commented Jun 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

infvg commented May 15, 2025 •

edited

Loading

github-actions bot commented May 22, 2025 •

edited

Loading

infvg May 28, 2025 •

edited

Loading