Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Push update to connector (All jdbc) #16445

Merged

Conversation

vlad-lyutenko
Copy link
Contributor

@vlad-lyutenko vlad-lyutenko commented Mar 8, 2023

Description

Introduce push update for jdbc connectors in the same way as delete.
So it's kind of shorthand path, where we catch some specific execution plan.
And only if it's match and it's simple we fire the direct connector call,
omitting global update/merge logic.

In initial version it has some limitations like:

  • Push only constant updates, so we not supporting arithmetic expression
  • Not supporting Update ALL, where we update all column values.

Supported connectors:

  • Ignite
  • MariaDB
  • MySql
  • Oracle
  • Postgres
  • Redshift
  • Singlestore
  • SqlServer

Additional context and related issues

Release notes

(x) Release notes are required, with the following suggested text:

Add limited support for UPDATE statement. Only UPDATE with constant assignments and predicates is supported today like UPDATE table SET col1 = 'abc' WHERE col2 = 3

Supported predicates :

  • =, !=, >, <, >=, <=, IN, NOT IN

This version has some limitations like:

  • Not supporting Update ALL, where we update all column values of table row.
  • can not use AND OR in predicates

Supported connectors:

  • Ignite
  • MariaDB
  • MySql
  • Oracle
  • Postgres
  • Redshift
  • Singlestore
  • SqlServer

@cla-bot cla-bot bot added the cla-signed label Mar 8, 2023
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/push-update-to-connector branch from 4a887e3 to d538664 Compare March 10, 2023 12:14
@vlad-lyutenko
Copy link
Contributor Author

Implemented for Postgres to prove that full flow works,
UPDATE nation SET name = 'a' WHERE nationkey = 1; - current query updates nation table

Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach looks good to me. Add tests like the ones related to io.trino.testing.TestingConnectorBehavior#SUPPORTS_DELETE

/**
* Push update into connector
*/
Optional<TableHandle> applyUpdate(Session session, TableHandle tableHandle);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need assignments here? I think there could be a case where predicate is easy and assignments are complex to be pushed down. Is this handled somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand this code executed only in PushMergeWriterUpdateIntoConnector rule, where we catch only concrete node plan for simple cases. So in case of complex assignment like Update with Select, we never fall here.
And flow will fail on beginMerge

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates cannot be pushed into a regular table scan. They have fundamentally different semantics -- scans are side-effect-free, while updates have side effects. This is important for retries and transactionality.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are already doing this for DELETE.

UPDATE, like DELETE, has side effects but it is idempotent operation (as long query is deterministic). It sounds like a good idea that we should not push down DELETE and UPDATE if they are not deterministic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the decision here?

Copy link
Contributor

@chenjian2664 chenjian2664 Apr 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the update(value is deterministic) can be retry as long as the condition is not involved in the update assignments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates cannot be pushed into a regular table scan.

@martint notice that we are pushing update into TableUpdateNode, then have dedicated operator that executes the update operation. So there is no table scan here.

@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/push-update-to-connector branch 4 times, most recently from a1b30bc to 155293a Compare March 20, 2023 21:26
@vlad-lyutenko
Copy link
Contributor Author

Consider expression translation for parsing assignment:

ConnectorExpressionTranslator.ConnectorExpressionTranslation expressionTranslation = ConnectorExpressionTranslator.translateConjuncts(
                    context.getSession(),
                    ((Expression) assigmentNode),
                    context.getSymbolAllocator().getTypes(),
                    plannerContext,
                    typeAnalyzer);

@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/push-update-to-connector branch 3 times, most recently from 10c6926 to e71a212 Compare March 23, 2023 10:49
@vlad-lyutenko vlad-lyutenko changed the title Push update to connector (fro example Postgres) Push update to connector (Postgres) Mar 23, 2023
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/push-update-to-connector branch 4 times, most recently from c71dc9e to ecead73 Compare March 27, 2023 14:25
/**
* Push update into connector
*/
Optional<TableHandle> applyUpdate(Session session, TableHandle tableHandle);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates cannot be pushed into a regular table scan. They have fundamentally different semantics -- scans are side-effect-free, while updates have side effects. This is important for retries and transactionality.

Copy link
Contributor

@chenjian2664 chenjian2664 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice job. Only some doubts here, and one more question: I saw the modification in the fte tests, does it mean we can support fte on Update after this merged?

@vlad-lyutenko
Copy link
Contributor Author

I saw the modification in the fte tests, does it mean we can support fte on Update after this merged?

to be honest didn't get it, can you please describe it more

@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/push-update-to-connector branch 2 times, most recently from 82d3e26 to 3fe2ed2 Compare August 16, 2023 08:36
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/push-update-to-connector branch from 3fe2ed2 to a7dd36e Compare August 16, 2023 13:50
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/push-update-to-connector branch from a7dd36e to ceb991b Compare August 16, 2023 14:07
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/push-update-to-connector branch 5 times, most recently from 978c319 to 8ec6987 Compare August 28, 2023 10:01
@wendigo wendigo self-requested a review September 6, 2023 10:38
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/push-update-to-connector branch 3 times, most recently from c99b853 to c61f578 Compare September 8, 2023 10:58
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/push-update-to-connector branch from c61f578 to 57cd7af Compare September 8, 2023 13:46
@martint martint dismissed their stale review September 11, 2023 16:25

The engine level changes look good now

@kokosing kokosing merged commit 7b80852 into trinodb:master Sep 12, 2023
94 checks passed
@kokosing
Copy link
Member

Merged. Thank you @vlad-lyutenko for the change and thank you @martint for code review!

@kokosing
Copy link
Member

@vlad-lyutenko Can you please update the PR description and suggest the release notes. Please mention explicitly which connectors are now able to support simple UPDATE statements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed hive Hive connector
Development

Successfully merging this pull request may close these issues.

8 participants