Skip to content

Conversation

@sebastianopeluso
Copy link
Contributor

@sebastianopeluso sebastianopeluso commented Jun 19, 2025

Motivation and Context

Summary:
This PR reverts #25124 Write mapping support. #25124 changed JdbcClient interface and breaks facebook internal downstream dependencies blocking the internal release. A fix requires non-trivial changes to our internal implementations of the interface, and we need more time to work on a proper fix.

Original commit changeset: 9bb915e4d34b

Original Phabricator Diff: D76875662

Differential Revision: D76945079

Impact

Revert code

Test Plan

Build

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

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Fix: Revert the Write mapping support

Summary:
Original commit changeset: 9bb915e4d34b

Original Phabricator Diff: D76875662

Differential Revision: D76945079
@sebastianopeluso sebastianopeluso requested a review from a team as a code owner June 19, 2025 00:08
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 19, 2025

CLA Signed

  • ✅login: sebastianopeluso / (07282d3)

The committers listed above are authorized under a signed CLA.

@facebook-github-bot
Copy link
Collaborator

This pull request was exported from Phabricator. Differential Revision: D76945079

@sebastianopeluso sebastianopeluso changed the title Back out "[presto][diff_train] Add write mapping" Revert #25124 Write mapping support Jun 19, 2025
@sebastianopeluso sebastianopeluso requested a review from infvg June 19, 2025 00:31
@gggrace14 gggrace14 self-requested a review June 19, 2025 01:58
@sebastianopeluso
Copy link
Contributor Author

@infvg I was wondering whether we can proceed with some incremental changes to the interface so as to avoid breaking the existing implementations.

@infvg
Copy link
Contributor

infvg commented Jun 19, 2025

@sebastianopeluso sure that works

@infvg
Copy link
Contributor

infvg commented Jun 19, 2025

@sebastianopeluso could you please elaborate on what issues were faced with these changes & which parts of the PR caused breaking changes that were difficult to fix?

@sebastianopeluso
Copy link
Contributor Author

@infvg, as the return type of toPrestoType changed from Optional<ReadMapping> to Optional<ColumnMapping>, that broke our internal implementation of toPrestoType. I tried to adapt that implementation. The main challenge was on the replacement of sliceReadMapping with ColumnMapping.sliceMapping, as I am not sure on the additional parameter of type SliceWriteFunction to use. Not sure if a lambda throwing an UnsupportedOperationException would be just enough in that case.
Another challenge is on the implementation of the additional toWriteMapping.
@gggrace14, whenever you get the chance, can you take a look as you have context on the existing implementation? Happy to discuss a path forward.

@sebastianopeluso
Copy link
Contributor Author

Hi @infvg, would it be possible for you to have a chat on Slack? We can figure out how to move forward with this change, and I can also get more context. What's your name on Slack? CC: @amitkdutta

@sebastianopeluso
Copy link
Contributor Author

We just had a meeting to discuss the details of this PR. Thanks @infvg for your time and the explanations!
Attendees: @infvg, @gggrace14 , @rschlussel , @sebastianopeluso , @zation99

@infvg explained the details of the change, and how was able to adapt existing implementations to work with the new interface. @infvg also explained that it did not take more than few hours to adapt the existing implementations, and given that @infvg had full context about this PR. Internally, ibm has a qa team to implement test cases for this change, and going through testing several scenarios.

After the meeting, we believe that, adapting to the interface's change of this PR for Meta's internal implementation might be quick. However, we need extensive testing for correctness because we are touching every type.
Therefore, we agreed to merge this revert #25369 PR, and then provide an ETA on when we are going to have the adaptation and testing ready to bring the changes from the original #25124 PR back.

@sebastianopeluso sebastianopeluso merged commit 8f6f16e into prestodb:master Jun 23, 2025
181 of 188 checks passed
@prestodb-ci prestodb-ci mentioned this pull request Jul 28, 2025
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants