Fix on conflict error and add support for other on conflict queries in Postgres
#34227
Conversation
…ement of postgres
…or_postgres' into Issue#32280_bugfix_on_conflict_for_postgres
omkar-shitole
changed the title
on conflict error and add support for other on conflict queries in Postgres
|
Hi @terrymanu, Will you please review and proceed? |
@omkar-shitole Thank you for your contribution, I will review this pr later. |
| @@ -75,6 +75,7 @@ | |||
| 1. Encrypt: Use sql bind info in EncryptInsertPredicateColumnTokenGenerator to avoid wrong column table mapping - [#34110](https://github.com/apache/shardingsphere/pull/34110) | |||
| 1. Mode: Fixes `JDBCRepository` improper handling of H2-database in memory mode - [#33281](https://github.com/apache/shardingsphere/issues/33281) | |||
| 1. Mode: Fixes duplicate column names added when index changed in DDL - [#33982](https://github.com/apache/shardingsphere/issues/33281) | |||
| 1. SQL Binder: Fixes bug: throwing exception while using WHERE statement in ON CONFLICT with INSERT INTO in Postgres [#32280](https://github.com/apache/shardingsphere/issues/32280) | |||
There was a problem hiding this comment.
Can you modify this change log to Fix on conflict error and add support for other on conflict queries in Postgres #34227?
| import java.util.List; | ||
| import java.util.Optional; | ||
| import java.util.stream.Collectors; | ||
|
|
There was a problem hiding this comment.
Please add javadoc for this class.
| import java.util.stream.Collectors; | ||
|
|
||
| @Getter | ||
| @ToString |
There was a problem hiding this comment.
Why add @ToString annotation?
There was a problem hiding this comment.
Thank you for pointing this out. I added the annotation because, while it has not been explicitly used anywhere currently, I observed a similar annotation unused applied to the OnDuplicateUpdateContext class. I included it to maintain consistency and to ensure that if needed later.
However, if you feel this annotation is unnecessary for now, let me know will cover this in the correction commit.
|
|
||
| @Getter | ||
| @ToString | ||
| public final class OnConflictUpdateContext implements WhereAvailable { |
There was a problem hiding this comment.
Please remove WhereAvailable here, it's only for SQLStatement.
There was a problem hiding this comment.
Thank you for reviewing and pointing this out too. Adding it here was redundant and an oversight on my part as I missed removing it during cleanup. I will address it in the correction commit.
Thank you again!
There was a problem hiding this comment.
@strongduanmu
All other suggested changes/modifications are ready and added to the branch.
Will you please confirm this once?
|
|
||
| private final Collection<ColumnSegment> columnSegments; | ||
|
|
||
| private final Collection<BinaryOperationExpression> joinConditions = new LinkedList<>(); |
There was a problem hiding this comment.
Is there a join condition in On Conflict segment?
There was a problem hiding this comment.
No, It's not part of a particular segment. join can be handled outside the conflict segment in Insert statement . I'll address this in a correction commit.
https://www.postgresql.org/docs/current/sql-insert.html
Fixes #32280
Changes proposed in this pull request:
whereSegmentinonConflictKeyColumnSegmentwhich was missing and required. The current flow is more intent on theon_duplicateof MySQL thanon_conflictin Postgres which has been added in Postgres after Postgres 9.optOnConflictrule which was eventually failing to read parameter markers expressions and assignments present inwhereSegmentINSERT INTOstatement. The flow for ON DUPLICATE has not been removed or altered for now, as It may be useful for certain conditions of older versions of Postgres (and can be deprecated in future if no use case is observed for long)Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.