From 4b29820ca170282ce1fb2820b24f21cae27eb870 Mon Sep 17 00:00:00 2001 From: Palash Chauhan Date: Wed, 4 Feb 2026 16:05:03 -0800 Subject: [PATCH] @W-21080170 : Fix update expression parsing for SET with IF_NOT_EXISTS and arithmetic ops (#192) --- .../phoenix/ddb/UpdateItemBaseTests.java | 42 ++++++++- .../ddb/bson/UpdateExpressionDdbToBson.java | 93 ++++++++++--------- 2 files changed, 91 insertions(+), 44 deletions(-) diff --git a/phoenix-ddb-rest/src/test/java/org/apache/phoenix/ddb/UpdateItemBaseTests.java b/phoenix-ddb-rest/src/test/java/org/apache/phoenix/ddb/UpdateItemBaseTests.java index c3c1689..75de539 100644 --- a/phoenix-ddb-rest/src/test/java/org/apache/phoenix/ddb/UpdateItemBaseTests.java +++ b/phoenix-ddb-rest/src/test/java/org/apache/phoenix/ddb/UpdateItemBaseTests.java @@ -143,7 +143,7 @@ public UpdateItemBaseTests(boolean isSortKeyPresent) { * attribute that is of type Number. */ @Test(timeout = 120000) - public void testSet() { + public void testSet1() { final String tableName = testName.getMethodName().replaceAll("[\\[\\]]", ""); createTableAndPutItem(tableName, true); @@ -171,6 +171,45 @@ public void testSet() { validateItem(tableName, key); } + /** + * SET: test arithmetic operations with if_not_exists function. + */ + @Test(timeout = 120000) + public void testSet2() { + final String tableName = testName.getMethodName().replaceAll("[\\[\\]]", ""); + createTableAndPutItem(tableName, true); + + // update item + Map key = getKey(); + UpdateItemRequest.Builder uir = UpdateItemRequest.builder().tableName(tableName).key(key); + uir.updateExpression("SET #1 = if_not_exists(#1, :v100) + :v1, " + + "#4 = :v100 - if_not_exists( #4, :v100), " + + "#n1 = if_not_exists(#n1, :v0) + :v100, " + + "#n2 = if_not_exists(#n2, :v110)- :v10, " + + "#n5 = if_not_exists(#n5, :v10) + if_not_exists(#5, :v0), " + + "NEWCOL6 = if_not_exists(NEWCOL6, :v10)"); + + Map exprAttrNames = new HashMap<>(); + exprAttrNames.put("#1", "COL1"); + exprAttrNames.put("#4", "COL4"); + exprAttrNames.put("#n1", "NEWCOL1"); + exprAttrNames.put("#n2", "NEWCOL2"); + exprAttrNames.put("#5", "COL5"); + exprAttrNames.put("#n5", "NEWCOL5"); + uir.expressionAttributeNames(exprAttrNames); + Map exprAttrVal = new HashMap<>(); + exprAttrVal.put(":v1", AttributeValue.builder().n("1").build()); + exprAttrVal.put(":v10", AttributeValue.builder().n("10").build()); + exprAttrVal.put(":v100", AttributeValue.builder().n("100").build()); + exprAttrVal.put(":v110", AttributeValue.builder().n("110").build()); + exprAttrVal.put(":v0", AttributeValue.builder().n("0").build()); + uir.expressionAttributeValues(exprAttrVal); + dynamoDbClient.updateItem(uir.build()); + phoenixDBClientV2.updateItem(uir.build()); + + validateItem(tableName, key); + } + /** * REMOVE - Removes one or more attributes from an item. */ @@ -404,6 +443,7 @@ protected Map getItem1() { item.put("COL2", AttributeValue.builder().s("Title1").build()); item.put("COL3", AttributeValue.builder().s("Description1").build()); item.put("COL4", AttributeValue.builder().n("34").build()); + item.put("COL5", AttributeValue.builder().n("67").build()); item.put("TopLevelSet", AttributeValue.builder().ss("setMember1").build()); Map reviewMap1 = new HashMap<>(); reviewMap1.put("reviewer", AttributeValue.builder().s("Alice").build()); diff --git a/phoenix-ddb-utils/src/main/java/org/apache/phoenix/ddb/bson/UpdateExpressionDdbToBson.java b/phoenix-ddb-utils/src/main/java/org/apache/phoenix/ddb/bson/UpdateExpressionDdbToBson.java index 1855c11..5db52d6 100644 --- a/phoenix-ddb-utils/src/main/java/org/apache/phoenix/ddb/bson/UpdateExpressionDdbToBson.java +++ b/phoenix-ddb-utils/src/main/java/org/apache/phoenix/ddb/bson/UpdateExpressionDdbToBson.java @@ -21,14 +21,12 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; +import org.bson.BsonArray; import org.bson.BsonDocument; import org.bson.BsonNull; -import org.bson.BsonNumber; import org.bson.BsonString; import org.bson.BsonValue; -import org.apache.phoenix.expression.util.bson.UpdateExpressionUtils; - /** * Utility to convert DynamoDB UpdateExpression into BSON Update Expression. */ @@ -38,10 +36,12 @@ public class UpdateExpressionDdbToBson { private static final String removeRegExPattern = "REMOVE\\s+(.+?)(?=\\s+(SET|ADD|DELETE)\\b|$)"; private static final String addRegExPattern = "ADD\\s+(.+?)(?=\\s+(SET|REMOVE|DELETE)\\b|$)"; private static final String deleteRegExPattern = "DELETE\\s+(.+?)(?=\\s+(SET|REMOVE|ADD)\\b|$)"; + private static final String ifNotExistsPattern = "if_not_exists\\s*\\(\\s*([^,]+)\\s*,\\s*([^)]+)\\s*\\)"; private static final Pattern SET_PATTERN = Pattern.compile(setRegExPattern); private static final Pattern REMOVE_PATTERN = Pattern.compile(removeRegExPattern); private static final Pattern ADD_PATTERN = Pattern.compile(addRegExPattern); private static final Pattern DELETE_PATTERN = Pattern.compile(deleteRegExPattern); + private static final Pattern IF_NOT_EXISTS_PATTERN = Pattern.compile(ifNotExistsPattern); public static BsonDocument getBsonDocumentForUpdateExpression( final String updateExpression, @@ -75,7 +75,8 @@ public static BsonDocument getBsonDocumentForUpdateExpression( BsonDocument bsonDocument = new BsonDocument(); if (!setString.isEmpty()) { BsonDocument setBsonDoc = new BsonDocument(); - String[] setExpressions = setString.split(","); + // split by comma only if comma is not within () + String[] setExpressions = setString.split(",(?![^()]*+\\))"); for (int i = 0; i < setExpressions.length; i++) { String setExpression = setExpressions[i].trim(); String[] keyVal = setExpression.split("\\s*=\\s*"); @@ -83,19 +84,9 @@ public static BsonDocument getBsonDocumentForUpdateExpression( String attributeKey = keyVal[0].trim(); String attributeVal = keyVal[1].trim(); if (attributeVal.contains("+") || attributeVal.contains("-")) { - setBsonDoc.put(attributeKey, getArithmeticExpVal(attributeVal, comparisonValue)); + setBsonDoc.put(attributeKey, getArithmeticDoc(attributeVal, comparisonValue)); } else if (attributeVal.startsWith("if_not_exists")) { - // attributeVal --> if_not_exists ( path - String ifNotExistsPath = attributeVal.split("\\(")[1].trim(); - // next setExpression --> value) - String fallBackValue = setExpressions[i+1].split("\\)")[0].trim(); - BsonValue fallBackValueBson = comparisonValue.get(fallBackValue); - BsonDocument fallBackDoc = new BsonDocument(); - fallBackDoc.put(ifNotExistsPath, fallBackValueBson); - BsonDocument ifNotExistsDoc = new BsonDocument(); - ifNotExistsDoc.put("$IF_NOT_EXISTS", fallBackDoc); - setBsonDoc.put(attributeKey, ifNotExistsDoc); - i++; + setBsonDoc.put(attributeKey, getIfNotExistsDoc(attributeVal, comparisonValue)); } else { setBsonDoc.put(attributeKey, comparisonValue.get(attributeVal)); } @@ -153,36 +144,52 @@ public static BsonDocument getBsonDocumentForUpdateExpression( return bsonDocument; } - private static BsonString getArithmeticExpVal(String attributeVal, - BsonDocument comparisonValuesDocument) { - String[] tokens = attributeVal.split("\\s+"); - Pattern pattern = Pattern.compile("[#:$]?[^\\s\\n]+"); - StringBuilder val = new StringBuilder(); - for (String token : tokens) { - if (token.equals("+")) { - val.append(" + "); - continue; - } else if (token.equals("-")) { - val.append(" - "); - continue; + private static BsonDocument getArithmeticDoc(String expr, BsonDocument comparisonValue) { + BsonDocument arithmeticDoc = new BsonDocument(); + String op; + String[] operands; + if (expr.contains("+")) { + op = "$ADD"; + operands = expr.split("\\+"); + } else if (expr.contains("-")) { + op = "$SUBTRACT"; + operands = expr.split("-"); + } else { + throw new IllegalArgumentException("Unsupported arithmetic operator for SET"); } - Matcher matcher = pattern.matcher(token); - if (matcher.find()) { - String operand = matcher.group(); - if (operand.startsWith(":") || operand.startsWith("$") || operand.startsWith("#")) { - BsonValue bsonValue = comparisonValuesDocument.get(operand); - if (!bsonValue.isNumber() && !bsonValue.isDecimal128()) { - throw new IllegalArgumentException( - "Operand " + operand + " is not provided as number type"); + BsonArray bsonOperands = new BsonArray(); + for (String operand : operands) { + operand = operand.trim(); + if (operand.startsWith("if_not_exists")) { + bsonOperands.add(getIfNotExistsDoc(operand, comparisonValue)); + } else if (operand.startsWith(":") || operand.startsWith("$") || operand.startsWith("#")) { + BsonValue bsonValue = comparisonValue.get(operand); + if (!bsonValue.isNumber() && !bsonValue.isDecimal128()) { + throw new IllegalArgumentException( + "Operand " + operand + " is not provided as number type"); + } + bsonOperands.add(bsonValue); + } else { + bsonOperands.add(new BsonString(operand)); } - Number numVal = UpdateExpressionUtils.getNumberFromBsonNumber((BsonNumber) bsonValue); - val.append(numVal); - } else { - val.append(operand); - } } - } - return new BsonString(val.toString()); + arithmeticDoc.put(op, bsonOperands); + return arithmeticDoc; } + private static BsonDocument getIfNotExistsDoc(String expr, BsonDocument comparisonValue) { + Matcher m = IF_NOT_EXISTS_PATTERN.matcher(expr); + if (m.find()) { + String ifNotExistsPath = m.group(1).trim(); + String fallBackValue = m.group(2).trim(); + BsonValue fallBackValueBson = comparisonValue.get(fallBackValue); + BsonDocument fallBackDoc = new BsonDocument(); + fallBackDoc.put(ifNotExistsPath, fallBackValueBson); + BsonDocument ifNotExistsDoc = new BsonDocument(); + ifNotExistsDoc.put("$IF_NOT_EXISTS", fallBackDoc); + return ifNotExistsDoc; + } else { + throw new RuntimeException("Invalid format for if_not_exists(path, value)"); + } + } }