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

Fix on conflict error and add support for other on conflict queries in Postgres #34227

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
e0dda25
Replace System.lineSeparator() with \n for cross-platform consistency…
omkar-shitole Dec 6, 2024
5ec751c
Merge pull request #1 from omkar-shitole/ISSUE#33789/DATA_PIPELINE_FA…
omkar-shitole Dec 6, 2024
f170f5e
Merge branch 'apache:master' into master
omkar-shitole Dec 9, 2024
ce175f9
Merge branch 'apache:master' into master
omkar-shitole Dec 31, 2024
b455574
add support for WHERE segment with ON CONFLICT segment in INSERT stat…
omkar-shitole Dec 31, 2024
30283fc
updated RELEASE-NOTES.md
omkar-shitole Dec 31, 2024
008ba36
remove redundant commented code
omkar-shitole Jan 2, 2025
27b4042
Merge branch 'apache:master' into master
omkar-shitole Jan 2, 2025
ed4a82e
Issue #32280/ bug fix: WHERE segment with ON CONFLICT segment in INSE…
omkar-shitole Jan 2, 2025
6a72dfa
fix spotless and cehckstyle
omkar-shitole Jan 2, 2025
c985ba1
Merge branch 'apache:master' into master
omkar-shitole Jan 2, 2025
8201efd
Merge branch 'apache:master' into master
omkar-shitole Jan 7, 2025
426e9bf
modify and add new e2e test cases
omkar-shitole Jan 7, 2025
af0a37e
Merge branch 'apache:master' into Issue#32280_bugfix_on_conflict_for_…
omkar-shitole Jan 7, 2025
fee7feb
Merge remote-tracking branch 'origin/Issue#32280_bugfix_on_conflict_f…
omkar-shitole Jan 7, 2025
107044b
Merge branch 'apache:master' into master
omkar-shitole Jan 14, 2025
03a00c3
correction commit with modifications in javadoc, release-notes and On…
omkar-shitole Jan 14, 2025
8d42636
Merge branch 'apache:master' into master
omkar-shitole Jan 14, 2025
6d16aff
Merge branch 'apache:master' into master
omkar-shitole Jan 23, 2025
d0c8879
Revert "Issue #32280/ bug fix: WHERE segment with ON CONFLICT segment…
omkar-shitole Jan 24, 2025
8449442
Merge branch 'apache:master' into master
omkar-shitole Jan 24, 2025
cc421be
Merge branch 'apache:master' into master
omkar-shitole Jan 27, 2025
5e225c4
correction commit with modifications as per requested changes
omkar-shitole Jan 29, 2025
b51d95b
Merge remote-tracking branch 'upstream/master' into Issue#32280_bugfi…
omkar-shitole Jan 29, 2025
41c96cd
Merge branch 'master' into Issue#32280_bugfix_on_conflict_for_postgres
omkar-shitole Jan 29, 2025
033c582
rename varibale to result in test-class
omkar-shitole Jan 30, 2025
7c34c47
Resolved merge conflicts from upstream/master
omkar-shitole Feb 3, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE-NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Can you modify this change log to Fix on conflict error and add support for other on conflict queries in Postgres #34227?


### Change Logs

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.shardingsphere.infra.binder.context.segment.insert.values;

import com.google.common.base.Preconditions;
import lombok.Getter;
import lombok.ToString;
import org.apache.shardingsphere.infra.binder.context.type.WhereAvailable;
import org.apache.shardingsphere.sql.parser.statement.core.extractor.ColumnExtractor;
import org.apache.shardingsphere.sql.parser.statement.core.extractor.ExpressionExtractor;
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.assignment.ColumnAssignmentSegment;
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.column.ColumnSegment;
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.expr.BinaryOperationExpression;
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.expr.ExpressionSegment;
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.expr.FunctionSegment;
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.expr.simple.LiteralExpressionSegment;
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.expr.simple.ParameterMarkerExpressionSegment;
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.predicate.WhereSegment;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.LinkedList;
import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;

Copy link
Member

Choose a reason for hiding this comment

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

Please add javadoc for this class.

@Getter
@ToString
Copy link
Member

Choose a reason for hiding this comment

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

Why add @ToString annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

public final class OnConflictUpdateContext implements WhereAvailable {
Copy link
Member

Choose a reason for hiding this comment

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

Please remove WhereAvailable here, it's only for SQLStatement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@strongduanmu
All other suggested changes/modifications are ready and added to the branch.
Will you please confirm this once?


private final int parameterCount;

private final List<ExpressionSegment> valueExpressions;

private final Collection<WhereSegment> whereSegments = new LinkedList<>();

private final Collection<ColumnSegment> columnSegments;

private final Collection<BinaryOperationExpression> joinConditions = new LinkedList<>();
Copy link
Member

Choose a reason for hiding this comment

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

Is there a join condition in On Conflict segment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


private final List<ParameterMarkerExpressionSegment> parameterMarkerExpressions;

private final List<Object> parameters;

public OnConflictUpdateContext(final Collection<ColumnAssignmentSegment> assignments, final List<Object> params, final int parametersOffset, final Optional<WhereSegment> segment) {
List<ExpressionSegment> expressionSegments = assignments.stream().map(ColumnAssignmentSegment::getValue).collect(Collectors.toList());
segment.ifPresent(whereSegments::add);
for (WhereSegment whereSegment : whereSegments) {
Copy link
Member

Choose a reason for hiding this comment

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

Please rename whereSegment to each

expressionSegments.add(whereSegment.getExpr());
}
columnSegments = assignments.stream().map(each -> each.getColumns().get(0)).collect(Collectors.toList());
Copy link
Member

Choose a reason for hiding this comment

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

Why only call each.getColumns().get(0) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Postgres ON CONFLICT clause only updates the specified conflicting column, and each assignment logically corresponds to a single column in this context.
In Mysql, the same provision has been made considering only one column will be assigned per assignment.

ColumnExtractor.extractColumnSegments(columnSegments, whereSegments);
ExpressionExtractor.extractJoinConditions(joinConditions, whereSegments);
valueExpressions = getValueExpressions(expressionSegments);
parameterMarkerExpressions = ExpressionExtractor.getParameterMarkerExpressions(expressionSegments);
parameterCount = parameterMarkerExpressions.size();
parameters = getParameters(params, parametersOffset);
}

private List<ExpressionSegment> getValueExpressions(final Collection<ExpressionSegment> assignments) {
List<ExpressionSegment> result = new ArrayList<>(assignments.size());
Copy link
Member

Choose a reason for hiding this comment

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

Only for cast Collection to List? If so, please modify the origianl assignments to ArrayList.

result.addAll(assignments);
return result;
}

private List<Object> getParameters(final List<Object> params, final int paramsOffset) {
if (params.isEmpty() || 0 == parameterCount) {
return Collections.emptyList();
}
List<Object> result = new ArrayList<>(parameterCount);
result.addAll(params.subList(paramsOffset, paramsOffset + parameterCount));
return result;
}

/**
* Get value.
*
* @param index index
* @return value
*/
public Object getValue(final int index) {
ExpressionSegment valueExpression = valueExpressions.get(index);
if (valueExpression instanceof ParameterMarkerExpressionSegment) {
return parameters.get(getParameterIndex((ParameterMarkerExpressionSegment) valueExpression));
}
if (valueExpression instanceof FunctionSegment) {
return valueExpression;
}
return ((LiteralExpressionSegment) valueExpression).getLiterals();
}

private int getParameterIndex(final ParameterMarkerExpressionSegment paramMarkerExpression) {
int result = parameterMarkerExpressions.indexOf(paramMarkerExpression);
Preconditions.checkArgument(result >= 0, "Can not get parameter index.");
return result;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.apache.shardingsphere.infra.binder.context.segment.insert.keygen.engine.GeneratedKeyContextEngine;
import org.apache.shardingsphere.infra.binder.context.segment.insert.values.InsertSelectContext;
import org.apache.shardingsphere.infra.binder.context.segment.insert.values.InsertValueContext;
import org.apache.shardingsphere.infra.binder.context.segment.insert.values.OnConflictUpdateContext;
import org.apache.shardingsphere.infra.binder.context.segment.insert.values.OnDuplicateUpdateContext;
import org.apache.shardingsphere.infra.binder.context.segment.table.TablesContext;
import org.apache.shardingsphere.infra.binder.context.statement.CommonSQLStatementContext;
Expand All @@ -43,6 +44,7 @@
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.assignment.InsertValuesSegment;
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.assignment.SetAssignmentSegment;
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.column.ColumnSegment;
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.column.OnConflictKeyColumnsSegment;
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.column.OnDuplicateKeyColumnsSegment;
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.combine.CombineSegment;
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.expr.BinaryOperationExpression;
Expand Down Expand Up @@ -92,6 +94,9 @@ public final class InsertStatementContext extends CommonSQLStatementContext impl
@Getter
private OnDuplicateUpdateContext onDuplicateKeyUpdateValueContext;

@Getter
private OnConflictUpdateContext onConflictKeyUpdateValueContext;

private GeneratedKeyContext generatedKeyContext;

public InsertStatementContext(final ShardingSphereMetaData metaData, final List<Object> params, final InsertStatement sqlStatement, final String currentDatabaseName) {
Expand All @@ -103,6 +108,7 @@ public InsertStatementContext(final ShardingSphereMetaData metaData, final List<
insertValueContexts = getInsertValueContexts(params, parametersOffset, valueExpressions);
insertSelectContext = getInsertSelectContext(metaData, params, parametersOffset, currentDatabaseName).orElse(null);
onDuplicateKeyUpdateValueContext = getOnDuplicateKeyUpdateValueContext(params, parametersOffset).orElse(null);
onConflictKeyUpdateValueContext = getOnConflictKeyUpdateValueContext(params, parametersOffset).orElse(null);
tablesContext = new TablesContext(getAllSimpleTableSegments());
List<String> insertColumnNames = getInsertColumnNames();
ShardingSphereSchema schema = getSchema(metaData, currentDatabaseName);
Expand Down Expand Up @@ -179,6 +185,18 @@ private Optional<OnDuplicateUpdateContext> getOnDuplicateKeyUpdateValueContext(f
return Optional.of(onDuplicateUpdateContext);
}

private Optional<OnConflictUpdateContext> getOnConflictKeyUpdateValueContext(final List<Object> params, final AtomicInteger parametersOffset) {
Optional<OnConflictKeyColumnsSegment> onConflictKeyColumnsSegment = getSqlStatement().getOnConflictKeyColumns();
if (!onConflictKeyColumnsSegment.isPresent()) {
return Optional.empty();
}
Collection<ColumnAssignmentSegment> onConflictKeyColumns = onConflictKeyColumnsSegment.get().getColumns();
Optional<WhereSegment> whereSegment = getSqlStatement().getOnConflictKeyColumns().flatMap(OnConflictKeyColumnsSegment::getWhere);
OnConflictUpdateContext onConflictUpdateContext = new OnConflictUpdateContext(onConflictKeyColumns, params, parametersOffset.get(), whereSegment);
parametersOffset.addAndGet(onConflictUpdateContext.getParameterCount());
return Optional.of(onConflictUpdateContext);
}

private Collection<SimpleTableSegment> getAllSimpleTableSegments() {
TableExtractor tableExtractor = new TableExtractor();
tableExtractor.extractTablesFromInsert(getSqlStatement());
Expand Down Expand Up @@ -228,6 +246,15 @@ public List<Object> getOnDuplicateKeyUpdateParameters() {
return null == onDuplicateKeyUpdateValueContext ? new ArrayList<>() : onDuplicateKeyUpdateValueContext.getParameters();
}

/**
* Get on duplicate key update parameters.
*
* @return on duplicate key update parameters
*/
public List<Object> getOnConflictKeyUpdateParameters() {
return null == onConflictKeyUpdateValueContext ? new ArrayList<>() : onConflictKeyUpdateValueContext.getParameters();
}

/**
* Get generated key context.
*
Expand Down Expand Up @@ -315,6 +342,7 @@ public void setUpParameters(final List<Object> params) {
insertValueContexts = getInsertValueContexts(params, parametersOffset, valueExpressions);
insertSelectContext = getInsertSelectContext(metaData, params, parametersOffset, currentDatabaseName).orElse(null);
onDuplicateKeyUpdateValueContext = getOnDuplicateKeyUpdateValueContext(params, parametersOffset).orElse(null);
onConflictKeyUpdateValueContext = getOnConflictKeyUpdateValueContext(params, parametersOffset).orElse(null);
ShardingSphereSchema schema = getSchema(metaData, currentDatabaseName);
generatedKeyContext = new GeneratedKeyContextEngine(getSqlStatement(), schema).createGenerateKeyContext(insertColumnNamesAndIndexes, insertValueContexts, params).orElse(null);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.item.ColumnProjectionSegment;
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.item.ProjectionSegment;
import org.apache.shardingsphere.sql.parser.statement.core.statement.dml.InsertStatement;
import org.apache.shardingsphere.sql.parser.statement.postgresql.dml.PostgreSQLInsertStatement;

import java.util.Collection;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -63,6 +64,12 @@ private InsertStatement copy(final InsertStatement sqlStatement) {
result.getValues().addAll(sqlStatement.getValues());
sqlStatement.getSetAssignment().ifPresent(result::setSetAssignment);
sqlStatement.getOnDuplicateKeyColumns().ifPresent(result::setOnDuplicateKeyColumns);
sqlStatement.getOnConflictKeyColumns().ifPresent(segment -> {
Copy link
Member

Choose a reason for hiding this comment

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

Please do not use PostgreSQLInsertStatement in SQLBinder. If you need call setOnConflictKeyColumnsSegment method, please add it to InsertStatement, and implement setOnConflictKeyColumnsSegment in PostgreSQLInsertStatement.

if (result instanceof PostgreSQLInsertStatement) {
((PostgreSQLInsertStatement) result).setOnConflictKeyColumnsSegment(segment);
}
});
sqlStatement.getWithSegment().ifPresent(result::setWithSegment);
sqlStatement.getOutputSegment().ifPresent(result::setOutputSegment);
sqlStatement.getMultiTableInsertType().ifPresent(result::setMultiTableInsertType);
sqlStatement.getMultiTableInsertIntoSegment().ifPresent(result::setMultiTableInsertIntoSegment);
Expand Down
Loading
Loading