Refactor Phoenix MERGE implementation #23114
Conversation
chenjian2664
force-pushed
the
refat_jdbc_sink
branch
from
6129867 to
3e434b9
Compare
plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMergeSink.java
Outdated
Show resolved
Hide resolved
chenjian2664
force-pushed
the
refat_jdbc_sink
branch
9 times, most recently
from
6c77aa4 to
556f1c9
Compare
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcPageSink.java
Outdated
Show resolved
Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcPageSink.java
Outdated
Show resolved
Hide resolved
plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMergeSink.java
Show resolved
Hide resolved
plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMergeSink.java
Outdated
Show resolved
Hide resolved
plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMergeTableHandle.java
Outdated
Show resolved
Hide resolved
plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMergeSink.java
Outdated
Show resolved
Hide resolved
plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMergeTableHandle.java
Outdated
Show resolved
Hide resolved
plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMetadata.java
Show resolved
Hide resolved
chenjian2664
force-pushed
the
refat_jdbc_sink
branch
2 times, most recently
from
c6eade8 to
6355ba3
Compare
chenjian2664
changed the title
kokosing
changed the title
chenjian2664
force-pushed
the
refat_jdbc_sink
branch
from
5ad7ce2 to
8edf88c
Compare
This allow us avoid to call non-static method in the JdbcPageSink. Also avoid call non-static method in PhoenixMergeSink and remove the unnecessary internal classes DeleteSink and UpdateSink
Use static method in PhoenixMergeSink to create the `updateSink` and `deleteSink` to eliminate the dependency on the order of statements
Follow the params order in the `JdbcPageSink`
chenjian2664
force-pushed
the
refat_jdbc_sink
branch
from
c83c8eb to
7a7fdc1
Compare
|
Thank you! |
| // The TupleDomain for building the conjuncts of the primary keys | ||
| ImmutableMap.Builder<ColumnHandle, Domain> primaryKeysDomainBuilder = ImmutableMap.builder(); | ||
| // This value is to build the TupleDomain, but it won't affect the query field in result of the `DefaultQueryBuilder#prepareDeleteQuery` | ||
| Domain dummy = Domain.singleValue(BIGINT, 0L); | ||
| for (JdbcColumnHandle columnHandle : phoenixClient.getPrimaryKeyColumnHandles(session, plainTable)) { | ||
| primaryKeysDomainBuilder.put(columnHandle, dummy); | ||
| } |
There was a problem hiding this comment.
this is the only thing that's a bit weird to me.
I don't understand why we say "it won't affect query".
The DefaultQueryBuilder can change in future. Either way it does use the tuple domain to create conjuncts by passing this to toConjuncts method.
There was a problem hiding this comment.
Thanks for the review.
I don't understand why we say "it won't affect query".
The domain does not participate in building the conjuncts as long as it is the singValue domain, this is what I want, only use the column and bind exception. It's a bit awkward to reuse the DefaultQueryBuilder, but currently the DefaultQueryBuilder not support function only accept the column handles to only get sql string.
The
DefaultQueryBuildercan change in future. Either way it does use the tuple domain to create conjuncts by passing this totoConjunctsmethod.
You are right, so after the merge got accept, the DefaultQueryBuilder needs to keep the correctness of the result that with the parameters: column & singDomain.
We need either implement a new query builder or provide/extract a new function to support the current situation to let the QueryBuilder more easily to maintain, I can have a try on it.:)
There was a problem hiding this comment.
Thanks for the explanation.
provide/extract a new function to support the current situation to let the QueryBuilder more easily to maintain, I can have a try on it.:)
I think this makes sense to do.
Description
Additional context and related issues
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.