Skip to content

[common] Make BinaryRow.anyNull respect the row offset - #9525

Open
LuciferYang wants to merge 1 commit into
apache:masterfrom
LuciferYang:fix/binaryrow-anynull-offset
Open

[common] Make BinaryRow.anyNull respect the row offset#9525
LuciferYang wants to merge 1 commit into
apache:masterfrom
LuciferYang:fix/binaryrow-anynull-offset

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

Purpose

close #9524

BinaryRow.anyNull() read the header word and the null-bit words from segments[0] at absolute positions 0 and i, while every other accessor in the class reads through the row's offset: isNullAt goes through bitGet(segments[0], offset, ...), the field getters through getFieldOffset(pos), and getRowKind() reads segments[0].get(offset). A row pointed at a non-zero offset therefore answered from whatever bytes sat at the start of the segment, so anyNull() and isNullAt() could disagree about the same row. Both reads now add the offset.

The four in-repo callers, in FieldNestedUpdateAgg and FieldNestedPartialUpdateAgg, each pass a row from keyProjection.apply(row).copy(), and BinaryRow.copy() re-points the copy at offset 0, so no current code path is affected. Non-zero offsets do occur: BinaryRowSerializer.pointTo gives a row the position after a length prefix when mapFromPages walks a page, so the first row in a page sits at offset 4.

anyNull(int[] fields) needed no change; it goes through isNullAt. BinaryArray.anyNull() already reads from offset + 4, so this was the last member of that family reading at an absolute index.

Tests

Two cases next to the existing anyNullTest in BinaryRowTest, one per code path in the method, both laying two rows into one segment with a shared concat helper and pointing the second one at a non-zero offset.

  • testAnyNullWithNonZeroOffset: arity 1, so only the header read runs. The row starts at offset 20, four bytes of padding plus a 16-byte row, so the offset is not a multiple of eight either, which is the alignment BinaryRowSerializer actually produces.
  • testAnyNullHighFieldWithNonZeroOffset: arity 60 with the null at field 59, which puts the null bit in the second 8-byte word, so the loop is what has to honor the offset. It also asserts isNullAt(59) first, so a failure cannot be blamed on the fixture.

In both cases the row without nulls goes first, so a read that ignores the offset lands on it and reports no null. Both 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.

anyNull() read the header word and the null-bit words from segments[0] at
absolute positions 0 and i, while every other accessor in the class reads
through the row's offset. A row pointed at a non-zero offset therefore answered
from another row's bytes, and anyNull() could disagree with isNullAt() about the
same row. Both reads now add the offset.

The four in-repo callers pass rows from BinaryRow.copy(), which re-points the
copy at offset 0, so no current code path is affected. Non-zero offsets do occur,
from BinaryRowSerializer.pointTo when mapFromPages points a row at the bytes
after a length prefix.

Assisted-by: GLM-5.3
@LuciferYang

Copy link
Copy Markdown
Contributor Author

The UTCase and ITCase Flink 1.x Others failure is a Maven Central transport error, not something this change causes: the run died in the Build step, before any test ran, on Could not transfer artifact org.apache.paimon:paimon-vector-index-java:pom:0.4.0 from/to central: Connection reset while resolving dependencies for paimon-vector. A re-run should clear it. 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] BinaryRow.anyNull ignores the row offset

1 participant