Skip to content

fix: format NativeMemoryConsumer id in toString - #5398

Merged
sunchao merged 1 commit into
apache:mainfrom
ywskycn:dev/codex/comet-native-memory-consumer-to-string
Aug 21, 2026
Merged

fix: format NativeMemoryConsumer id in toString#5398
sunchao merged 1 commit into
apache:mainfrom
ywskycn:dev/codex/comet-native-memory-consumer-to-string

Conversation

@ywskycn

@ywskycn ywskycn commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Fixing the sub issue 4 in #5212

Rationale for this change

What changes are included in this PR?

How are these changes tested?

sunchao
sunchao previously approved these changes Aug 20, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Summary

Reviewed the complete diff from 7e0e5d2a26928b05b936c3f2a9abd375d6efc41f to 2b118654f39d14601dfd983a420bb1a82b91a424 across five independent scopes. The one-line fix addresses item 4 of #5212, and I found no correctness or compatibility issue that should block it. Approving.

Prior state and problem

NativeMemoryConsumer.toString() used NativeMemoryConsumer(id=%), so Java interpreted %) as an invalid conversion and threw UnknownFormatConversionException. Spark stringifies memory consumers in allocation, release, spill, and diagnostic logging paths; the malformed string therefore prevented those paths from reliably identifying the native consumer.

Design approach

Replace the invalid conversion with %d, using the existing immutable long plan ID. This keeps the change confined to the diagnostic representation and preserves the surrounding class, ownership, and memory-management behavior.

Correctness / compatibility analysis

%d accepts the boxed Long passed by Java varargs, including negative values and both signed-long limits. The Java and Scala signatures, JNI descriptors, allocation/release calls, and spill callback are unchanged. The string is used for diagnostics; I found no repository parser or protocol that depends on an ASCII-only representation.

A focused harness compiled the exact base and head classes against Spark 3.5.9/JDK 11, Spark 4.0.4/JDK 17, and Spark 4.1.3/JDK 17. All 61,658 assertions passed, covering the old exception, corrected output, signed-long boundaries, 1,000 deterministic random IDs, available formatting locales, and modeled partial/zero/full memory grants. This was not a full Spark/native integration run.

Key design decisions

Keep the numeric ID unchanged and repair only its format specifier. There is no new state, synchronization, dependency, or public API. The separate consumer-accounting and spilling issues tracked by #5212 remain outside this patch.

Implementation sketch

The only production change is in CometTaskMemoryManager.NativeMemoryConsumer.toString(): String.format("NativeMemoryConsumer(id=%d)", id). Existing iterator construction, the AtomicLong-backed ID source, and native acquire/release integration continue to use the same numeric values and method signatures.

Behavioral changes worth calling out

Stringifying this consumer now yields a useful diagnostic instead of an invalid-format exception. Allocation amounts, release accounting, off-heap mode, and the no-op spill callback are unchanged.

The exact-head Java lint/package matrix and Spark 4.1/JDK 17 compile-only check passed. The main CI run is still in progress; its Rust job failed on Clippy lints in spark_bit_array.rs and mersenne.rs. Both files and the relevant CI configuration are identical at the reviewed base and head, so this is not a defect introduced by this Java change.

Suggested improvements

A small regression test that directly stringifies the consumer would make this failure harder to reintroduce. Including a value beyond the 32-bit range would also document that the identifier is a long. This is a non-blocking test-coverage suggestion; no additional change is required for this fix.

@ywskycn
ywskycn force-pushed the dev/codex/comet-native-memory-consumer-to-string branch from 2b11865 to 026c74f Compare August 21, 2026 04:45
@sunchao
sunchao dismissed their stale review August 21, 2026 05:09

Submitted in error by an automated review task. This monitor is configured for non-blocking COMMENT reviews only and must never approve.

@sunchao
sunchao merged commit 1bddcff into apache:main Aug 21, 2026
71 checks passed
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.

2 participants