Skip to content

MR: Keep computed block locations in IcebergSplit.getLocations - #18036

Open
thswlsqls wants to merge 1 commit into
apache:mainfrom
thswlsqls:fix/mr-split-locations-cache-overwrite
Open

MR: Keep computed block locations in IcebergSplit.getLocations#18036
thswlsqls wants to merge 1 commit into
apache:mainfrom
thswlsqls:fix/mr-split-locations-cache-overwrite

Conversation

@thswlsqls

Copy link
Copy Markdown
Contributor

Closes #18032

Summary

  • IcebergSplit declares transient String[] locations and checks locations == null, so the value is meant to be computed once and cached. The else branch overwrites it with ANYWHERE on every later call, so two calls on an unchanged split return different values — Hadoop's InputSplit.getLocations() contract expects them to match.
  • The else branch came in with #1582, which fixed an NPE when getLocations() runs on a worker node whose deserialized split has a null conf. The original implementation, #1192, had if (locations == null) alone. This change restores that shape and keeps the guard by folding the null check into localityPreferred, so a null conf still resolves to ANYWHERE and never calls Util.blockLocations.
  • No production failure is claimed here: plain MapReduce submission calls getLocations() once per split, so the case rests on the contract and on the caching the class already declares.

Testing done

  • Extended TestIcebergInputFormats#testLocality to call getLocations() twice on the same split and assert both calls return the computed locations. It fails before the fix — all 6 parameterized variants return ["*"] on the second call — and passes after.
  • ./gradlew :iceberg-mr:check — 158 tests, 0 failures.

AI Disclosure

  • Model: Claude Opus 5
  • Platform/Tool: Claude Code
  • Human Oversight: [unknown - human to fill in]
  • Prompt Summary: Fix IcebergSplit.getLocations() so a cache hit returns the previously computed block locations instead of overwriting them with ANYWHERE, keep the null-conf NPE guard added in MR: Fix NPE when InputSplit.getLocations is called on mappers #1582, and extend testLocality to cover repeated calls.

The transient locations field and the locations == null check make
getLocations() a one-time computation that is cached on the split, but
the else branch reassigned locations to ANYWHERE on every later call.
Calling getLocations() twice on the same split therefore returned the
computed block locations first and "*" afterwards, which breaks the
Hadoop InputSplit contract that repeated calls return the same value.

The else branch was added in apache#1582 to avoid an NPE when getLocations()
runs on a worker node, where the deserialized split has a null conf.
Folding the conf null check into localityPreferred keeps that guard:
with a null conf the split still resolves to ANYWHERE and never calls
Util.blockLocations. Extend testLocality to assert that a second call
returns the same locations.

Generated-by: Claude Code
@github-actions github-actions Bot added the MR label Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MR: IcebergSplit.getLocations discards cached block locations on repeated calls

2 participants