Copilot commented on code in PR #6348:
URL: https://github.com/apache/incubator-seata/pull/6348#discussion_r2190331473
##########
sqlparser/seata-sqlparser-druid/src/main/java/org/apache/seata/sqlparser/druid/BaseRecognizer.java:
##########
@@ -149,4 +157,62 @@ public boolean visit(SQLInsertStatement x) {
getAst().accept(visitor);
return true;
}
+
+ public List<String> getWhereColumns(SQLExpr sqlExpr) {
+ if (sqlExpr == null) {
+ return Collections.emptyList();
+ }
+ // single condition
+ if (sqlExpr instanceof SQLBinaryOpExpr) {
+ return getWhereColumns(Collections.singletonList(sqlExpr));
+ } else {
+ // multiple conditions
+ return getWhereColumns(sqlExpr.getChildren());
+ }
+ }
+
+ public List<String> getWhereColumns(List<SQLObject> list) {
+ if (CollectionUtils.isNotEmpty(list)) {
+ List<String> columns = new ArrayList<>(list.size());
+ for (SQLObject sqlObject : list) {
+ if (sqlObject instanceof SQLIdentifierExpr) {
+ columns.add(((SQLIdentifierExpr) sqlObject).getName());
+ } else {
+ getWhereColumns(sqlObject, columns);
+ }
+ }
+ return columns;
+ }
+ return Collections.emptyList();
+ }
+
+ public void getWhereColumns(SQLObject sqlExpr, List<String> list) {
+ if (sqlExpr instanceof SQLBinaryOpExpr) {
+ SQLExpr left = ((SQLBinaryOpExpr) sqlExpr).getLeft();
+ getWhereColumn(left, list);
+ SQLExpr right = ((SQLBinaryOpExpr) sqlExpr).getRight();
+ getWhereColumn(right, list);
+ }
+ }
+
+ public void getWhereColumn(SQLExpr left, List<String> list) {
+ if (left instanceof SQLBetweenExpr) {
+ SQLExpr expr = ((SQLBetweenExpr) left).getTestExpr();
+ if (expr instanceof SQLIdentifierExpr) {
+ list.add(((SQLIdentifierExpr) expr).getName());
+ }
+ if (expr instanceof SQLPropertyExpr) {
+ list.add(((SQLPropertyExpr) expr).getName());
+ }
+ } else if (left instanceof SQLIdentifierExpr) {
+ list.add(((SQLIdentifierExpr) left).getName());
Review Comment:
Currently `getWhereColumn` handles `SQLIdentifierExpr` and `SQLBetweenExpr`
but ignores direct `SQLPropertyExpr` (qualified columns) in equality or other
expressions. Add an `else if (left instanceof SQLPropertyExpr)` branch to
capture those column names as well.
```suggestion
list.add(((SQLIdentifierExpr) left).getName());
} else if (left instanceof SQLPropertyExpr) {
list.add(((SQLPropertyExpr) left).getName());
```
##########
rm-datasource/src/main/java/org/apache/seata/rm/datasource/exec/AbstractDMLBaseExecutor.java:
##########
@@ -98,6 +100,16 @@ protected T executeAutoCommitFalse(Object[] args) throws
Exception {
try {
TableRecords beforeImage = beforeImage();
T result =
statementCallback.execute(statementProxy.getTargetStatement(), args);
+ int updateCount = statementProxy.getUpdateCount();
+ if (updateCount > 0) {
+ if (SQLType.UPDATE == sqlRecognizer.getSQLType()) {
+ if (updateCount > beforeImage.size()) {
+ String errorMsg =
+ "Before image size is not equaled to after
image size, probably because you use read committed, please retry transaction.";
Review Comment:
The check only throws when `updateCount` is greater than
`beforeImage.size()`. To ensure full consistency, consider enforcing exact
equality (`updateCount != beforeImage.size()`) so mismatches in either
direction are caught.
```suggestion
if (updateCount != beforeImage.size()) {
String errorMsg =
"Before image size is not equal to after
image size, possibly due to read committed isolation level or other issues.
Please retry the transaction.";
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@seata.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@seata.apache.org
For additional commands, e-mail: notifications-h...@seata.apache.org
