Skip to content

[common] Fix double position advance in DataOutputSerializer.writeBytes - #9520

Open
LuciferYang wants to merge 1 commit into
apache:masterfrom
LuciferYang:fix/data-output-writebytes-position
Open

[common] Fix double position advance in DataOutputSerializer.writeBytes#9520
LuciferYang wants to merge 1 commit into
apache:masterfrom
LuciferYang:fix/data-output-writebytes-position

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

Purpose

close #9519

DataOutputSerializer.writeBytes(String) wrote each byte through writeByte, which goes to write(int) and does this.buffer[this.position++] = ..., and then advanced the position by the string length a second time. After the call length() reported twice the bytes written, and the next write landed past a gap of untouched buffer, so the serialized output was longer than the data and carried whatever the array happened to hold. Removing the extra advance is the whole change.

The sibling writeChars(String) has the same shape, a resize followed by a loop of writeChar, and never had that line, which is the form this method should have had.

I audited every position update in the class while checking this: write(byte[], int, int), write(MemorySegment, int, int), writeChar, writeShort, writeInt, writeLong, writeUTF, skipBytesToWrite and write(DataInputView, int) each advance by exactly the bytes they wrote. writeBytes was the only one that did not.

Nothing in the repository calls this overload, so no Paimon code path changes behavior. It is part of the java.io.DataOutput contract that DataOutputView exposes, so the defect was visible to callers outside the repository.

Tests

DataOutputSerializerTest is new; the class had no test before. All three assert on getCopyOfBuffer(), which is cut at length(), so each one pins the length and the content together.

  • testWriteBytesAdvancesPositionOnce: writeBytes("abc") leaves exactly a b c.
  • testWriteBytesLeavesTheNextWriteInPlace: writeBytes("abc") then writeInt(42) leaves a b c 0 0 0 42, which is the assertion that catches a gap rather than just a wrong length.
  • testWriteBytesAcrossAResize: starts from a two-byte buffer and writes six bytes, so the growth path runs during the call.

All three fail against the pre-fix code.

mvn -pl paimon-common test on JDK 8: 12467 tests, 0 failures, 0 errors. checkstyle, spotless, enforcer and rat run clean.

writeBytes(String) wrote each byte through writeByte, which goes to write(int)
and advances position itself, and then advanced position by the string length a
second time. After the call length() reported twice the bytes written and the
next write landed past a gap of untouched bytes. The sibling writeChars has the
same shape and never had that line.

Nothing in the repository calls this overload; it is part of the java.io.DataOutput
contract that DataOutputView exposes, so the defect is visible to callers outside
the repository.

Assisted-by: GLM-5.3
@LuciferYang

Copy link
Copy Markdown
Contributor Author

The two UTCase and ITCase Spark 3.x jobs failed on a Maven Central transport error rather than on this change: the 2.12 job died in the Build Spark 3 with 2.12 step, before any test ran, on Could not transfer artifact org.apache.spark:spark-sql_2.12:jar:tests:3.5.8 from/to central: Connection reset, and the 2.13 job was cancelled by the matrix fail-fast. A re-run should clear both. This PR touches only paimon-common; locally mvn -pl paimon-common clean test is green on JDK 8: 12467 tests, 0 failures.

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.

[Bug] DataOutputSerializer.writeBytes advances the write position twice

1 participant