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 8 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: Fix on conflict error and add support other on conflict queries in Postgres [#32280](https://github.com/apache/shardingsphere/issues/32280)

### Change Logs

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
/*
* 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 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.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.

/**
* On conflict context.
*/
@Getter
public final class OnConflictUpdateContext {

private final int parameterCount;

private final List<ExpressionSegment> valueExpressions;

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

private final Collection<ColumnSegment> columnSegments;

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);
valueExpressions = getValueExpressions(expressionSegments);
parameterMarkerExpressions = ExpressionExtractor.getParameterMarkerExpressions(expressionSegments);
parameterCount = parameterMarkerExpressions.size();
parameters = getParameters(params, parametersOffset);
}

/**
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 javadoc for private method.

* get value expressions from expression segments.
*
* @param assignments Collection of expression segments
* @return List of value expressions
*/
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;
}

/**
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 javadoc for private method.

* get list of parameters.
*
* @param params List of parameters
* @param paramsOffset Offset in the parameter list
* @return List of parameters
*/
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();
}

/**
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 javadoc for private method.

* get index of a parameter.
*
* @param paramMarkerExpression Parameter marker expression.
* @return Index of the parameter in the parameter list.
*/
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