Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"type": "feature",
"category": "Amazon DynamoDB Enhanced Client",
"contributor": "",
"description": "Improved performance of UpdateExpression conversion by replacing Stream.concat chains and String.format with direct iteration and StringJoiner."
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.StringJoiner;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import software.amazon.awssdk.annotations.SdkInternalApi;
Expand Down Expand Up @@ -113,53 +115,89 @@ public static List<String> findAttributeNames(UpdateExpression updateExpression)
private static List<String> groupExpressions(UpdateExpression expression) {
List<String> groupExpressions = new ArrayList<>();
if (!expression.setActions().isEmpty()) {
groupExpressions.add(SET + expression.setActions().stream()
.map(a -> String.format("%s = %s", a.path(), a.value()))
.collect(Collectors.joining(ACTION_SEPARATOR)));
StringJoiner joiner = new StringJoiner(ACTION_SEPARATOR, SET, "");
expression.setActions().forEach(a -> joiner.add(a.path() + " = " + a.value()));
groupExpressions.add(joiner.toString());
}
if (!expression.removeActions().isEmpty()) {
groupExpressions.add(REMOVE + expression.removeActions().stream()
.map(RemoveAction::path)
.collect(Collectors.joining(ACTION_SEPARATOR)));
StringJoiner joiner = new StringJoiner(ACTION_SEPARATOR, REMOVE, "");
expression.removeActions().forEach(a -> joiner.add(a.path()));
groupExpressions.add(joiner.toString());
}
if (!expression.deleteActions().isEmpty()) {
groupExpressions.add(DELETE + expression.deleteActions().stream()
.map(a -> String.format("%s %s", a.path(), a.value()))
.collect(Collectors.joining(ACTION_SEPARATOR)));
StringJoiner joiner = new StringJoiner(ACTION_SEPARATOR, DELETE, "");
expression.deleteActions().forEach(a -> joiner.add(a.path() + " " + a.value()));
groupExpressions.add(joiner.toString());
}
if (!expression.addActions().isEmpty()) {
groupExpressions.add(ADD + expression.addActions().stream()
.map(a -> String.format("%s %s", a.path(), a.value()))
.collect(Collectors.joining(ACTION_SEPARATOR)));
StringJoiner joiner = new StringJoiner(ACTION_SEPARATOR, ADD, "");
expression.addActions().forEach(a -> joiner.add(a.path() + " " + a.value()));
groupExpressions.add(joiner.toString());
}
return groupExpressions;
}

private static Stream<Map<String, String>> streamOfExpressionNames(UpdateExpression expression) {
return Stream.concat(expression.setActions().stream().map(SetAction::expressionNames),
Stream.concat(expression.removeActions().stream().map(RemoveAction::expressionNames),
Stream.concat(expression.deleteActions().stream()
.map(DeleteAction::expressionNames),
expression.addActions().stream()
.map(AddAction::expressionNames))));
private static Map<String, AttributeValue> mergeExpressionValues(UpdateExpression expression) {
Map<String, AttributeValue> merged = new HashMap<>();

for (SetAction action : expression.setActions()) {
mergeValuesInto(merged, action.expressionValues());
}
for (DeleteAction action : expression.deleteActions()) {
mergeValuesInto(merged, action.expressionValues());
}
for (AddAction action : expression.addActions()) {
mergeValuesInto(merged, action.expressionValues());
}

return merged.isEmpty() ? Collections.emptyMap() : Collections.unmodifiableMap(merged);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? Why not just returned Collections.unmodifiableMap(merged) unconditionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not necessary but we can avoid wrapping an empty map with UnmodifiableMap this way.

}

private static Map<String, AttributeValue> mergeExpressionValues(UpdateExpression expression) {
return streamOfExpressionValues(expression)
.reduce(Expression::joinValues)
.orElseGet(Collections::emptyMap);
private static Map<String, String> mergeExpressionNames(UpdateExpression expression) {
Map<String, String> merged = new HashMap<>();

for (SetAction action : expression.setActions()) {
mergeNamesInto(merged, action.expressionNames());
}
for (RemoveAction action : expression.removeActions()) {
mergeNamesInto(merged, action.expressionNames());
}
for (DeleteAction action : expression.deleteActions()) {
mergeNamesInto(merged, action.expressionNames());
}
for (AddAction action : expression.addActions()) {
mergeNamesInto(merged, action.expressionNames());
}

return merged.isEmpty() ? Collections.emptyMap() : Collections.unmodifiableMap(merged);
}

private static Stream<Map<String, AttributeValue>> streamOfExpressionValues(UpdateExpression expression) {
return Stream.concat(expression.setActions().stream().map(SetAction::expressionValues),
Stream.concat(expression.deleteActions().stream().map(DeleteAction::expressionValues),
expression.addActions().stream().map(AddAction::expressionValues)));
private static void mergeNamesInto(Map<String, String> target, Map<String, String> source) {
if (source == null || source.isEmpty()) {
return;
}
source.forEach((key, value) -> {
String oldValue = target.put(key, value);
if (oldValue != null && !oldValue.equals(value)) {
throw new IllegalArgumentException(
String.format("Attempt to coalesce two expressions with conflicting expression names. "
+ "Expression name key = '%s'", key));
Comment on lines +183 to +184
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this new behavior?

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, but I can see why you'd think that. It's a bit hard to read the diff.

In the current code streamOfExpressionNames calls joinNames() which has the same validation.

throw new IllegalArgumentException(
String.format("Attempt to coalesce two expressions with conflicting expression names. "
+ "Expression name key = '%s'", key));

In the new code I just moved the validation here.

We also have an existing test covering this specific codepath

void convert_removeActions_uniqueAttributes_duplicateNameTokens_error() {
UpdateExpression updateExpression = createUpdateExpression(
RemoveAction.builder().path("attribute_ref").putExpressionName("attribute_ref", "attribute1").build(),
RemoveAction.builder().path("attribute_ref").putExpressionName("attribute_ref", "attribute2").build());
assertThatThrownBy(() -> UpdateExpressionConverter.toExpression(updateExpression))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Attempt to coalesce two expressions with conflicting expression names")
.hasMessageContaining("attribute_ref");
}

}
});
}

private static Map<String, String> mergeExpressionNames(UpdateExpression expression) {
return streamOfExpressionNames(expression)
.reduce(Expression::joinNames)
.orElseGet(Collections::emptyMap);
private static void mergeValuesInto(Map<String, AttributeValue> target, Map<String, AttributeValue> source) {
if (source == null || source.isEmpty()) {
return;
}
source.forEach((key, value) -> {
AttributeValue oldValue = target.put(key, value);
if (oldValue != null && !oldValue.equals(value)) {
throw new IllegalArgumentException(
String.format("Attempt to coalesce two expressions with conflicting expression values. "
+ "Expression value key = '%s'", key));
}
});
}

private static List<String> listPathsWithoutTokens(UpdateExpression expression) {
Expand Down
Loading