Skip to content

feat: add custom map iteration naming - #585

Draft
wangyb-A wants to merge 2 commits into
mainfrom
feature/map-item-namer-fresh
Draft

feat: add custom map iteration naming#585
wangyb-A wants to merge 2 commits into
mainfrom
feature/map-item-namer-fresh

Conversation

@wangyb-A

Copy link
Copy Markdown
Contributor

Summary

  • add MapConfig.itemNamer for custom nested map-iteration names
  • preserve a null namer result as an unnamed iteration
  • resolve and validate names before allocating the map operation ID
  • reject itemNamer with NestingType.FLAT, whose virtual iterations have no context operation to name
  • detect changed iteration names when replaying STARTED and cached SUCCEEDED maps
  • implement Java conformance requirement 9-13

Compatibility

  • MapConfig remains non-generic
  • existing DurableContext method signatures are unchanged
  • default map iteration naming is unchanged when no item namer is configured

Validation

  • mvn -pl sdk test — 1102 passed
  • mvn -pl sdk-integration-tests test — 391 passed
  • mvn -pl conformance-tests package -DskipTests — passed
  • real deployed map conformance run is pending

Known baseline issue

Project-wide mvn spotless:check reports six pre-existing formatting violations under conformance-tests/src/main/java/plugin/. Those unrelated files are intentionally excluded from this PR.

Closes #528

@wangyb-A
wangyb-A temporarily deployed to ai-pr-review-runtime July 31, 2026 23:58 — with GitHub Actions Inactive
@wangyb-A
wangyb-A had a problem deploying to ai-pr-review-runtime July 31, 2026 23:58 — with GitHub Actions Failure
@github-actions

This comment has been minimized.

@wangyb-A
wangyb-A force-pushed the feature/map-item-namer-fresh branch from 45b16bf to 862cddc Compare August 1, 2026 00:15
@wangyb-A
wangyb-A had a problem deploying to ai-pr-review-runtime August 1, 2026 00:22 — with GitHub Actions Failure
@wangyb-A
wangyb-A had a problem deploying to ai-pr-review-runtime August 1, 2026 00:22 — with GitHub Actions Failure
var names = new java.util.ArrayList<String>(items.size());
for (int i = 0; i < items.size(); i++) {
if (namer == null) {
names.add(branchPrefix + i);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Style (low): Fully-qualified class name java.util.ArrayList. AGENTS.md marks this as a MUST-follow rule ("ALWAYS use proper imports, NEVER use fully qualified class names in code"), and spotless:apply won't rewrite it. Add import java.util.ArrayList; and use the simple name.

Suggested change
names.add(branchPrefix + i);
var names = new ArrayList<String>(items.size());

@wangyb-A
wangyb-A temporarily deployed to ai-pr-review-runtime August 1, 2026 00:31 — with GitHub Actions Inactive
@wangyb-A
wangyb-A temporarily deployed to ai-pr-review-runtime August 1, 2026 00:31 — with GitHub Actions Inactive
@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown

Codex AI review

[P2] Preserve map item typing in itemNamersdk/src/main/java/software/amazon/lambda/durable/config/MapConfig.java:138

Using BiFunction<Object, Integer, String> means typed functions such as BiFunction<Order, Integer, String> are rejected, and inline lambdas see the item only as Object, forcing unsafe casts. Make the config/builder item-type-aware and accept BiFunction<? super I, Integer, String> while preserving erased API compatibility; add a compilation test using an item-specific method.

Reviewed commit 99c7b9c51d050ae95a6d7cdd1fcccc8107f14e70. Workflow run


// Convert to List for deterministic index-based access
var itemList = List.copyOf(items);
var iterationNames = resolveMapIterationNames(name, itemList, config);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Low / maintainability: DurableContextImpl.resolveMapIterationNames is a byte-for-byte duplicate of MapOperation.resolveIterationNames (branch-prefix construction, null-namer default naming, and ParameterValidator.validateOperationName invocation). Because mapAsync always resolves names here and passes them to the 7-arg MapOperation constructor, the copy inside the legacy 6-arg constructor is only reachable via that retained-for-compatibility constructor — yet both must stay in lock-step. Any future change to naming or validation (e.g. length limits, null handling) applied to one copy but not the other would silently diverge the two construction paths.

Fix: extract a single shared helper (e.g. a package-visible static method on MapOperation) and call it from both mapAsync (before nextOperationId(), preserving the "validate before allocating the operation ID" ordering) and the 6-arg constructor, deleting one of the copies.

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown

Claude AI review

Summary

This PR adds MapConfig.itemNamer for custom nested map-iteration names, with validation-before-ID-allocation, FLAT-nesting rejection, and replay name-mismatch detection. The core semantics are correct and well-tested:

  • Ordering is right: names are resolved and validated in DurableContextImpl.mapAsync before nextOperationId(), so a rejected namer does not consume an operation ID (testInvalidItemNameDoesNotConsumeOperationId).
  • Replay safety: validateIterationNamesAgainstCheckpoint() runs before any child re-execution in both the SUCCEEDED (small and large/replayChildren) and STARTED paths, so a changed/removed namer fails deterministically with a NonDeterministicExecutionException and no replayed side effects. Covered by cached-replay, started-replay, and removed-namer tests.
  • Edge cases: empty maps never invoke the namer; a null namer result is preserved as an unnamed iteration; the 7-arg constructor enforces iterationNames.size() == items.size().
  • Compatibility: MapConfig stays non-generic (BiFunction<Object,Integer,String>), toBuilder() propagates itemNamer, and the legacy 6-arg MapOperation constructor is retained and reflectively pinned by MapOperationCompatibilityTest.

Findings

Low — maintainability (duplicated name-resolution logic)sdk/src/main/java/software/amazon/lambda/durable/context/DurableContextImpl.java:271 (and the twin at sdk/src/main/java/software/amazon/lambda/durable/operation/MapOperation.java resolveIterationNames). resolveMapIterationNames and MapOperation.resolveIterationNames are identical (prefix construction, null-namer default, and validateOperationName). The MapOperation copy is only reachable through the retained 6-arg constructor, but both must stay in lock-step; a future naming/validation change applied to one copy only would silently diverge the two construction paths. Fix: extract one shared helper and call it from both sites, preserving the validate-before-nextOperationId() ordering.

Residual test risk

  • No unit-level regression asserts that MapOperation's legacy 6-arg constructor and the mapAsync 7-arg path produce identical iteration names for the same input — this is exactly the divergence the duplication above could introduce, and no test would catch it.
  • The PR description notes the real deployed map conformance run for requirement 9-13 is still pending; only local/integration runs are reported.

Reviewed commit 99c7b9c51d050ae95a6d7cdd1fcccc8107f14e70. Workflow run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Item namer for map operation

1 participant