fix(parquet/file): count repeated skips in logical rows - #1002
fix(parquet/file): count repeated skips in logical rows#1002fallintoplace wants to merge 1 commit into
Conversation
zeroshade
left a comment
There was a problem hiding this comment.
The bug you identified here is real and worth fixing, but the patch as submitted does not build, so I can't merge it yet.
I traced the defect on main and confirmed it: in the repetition-level branch of skipRows, toSkip is denominated in logical rows but was being decremented by skipped, which is a count of physical values. Those are different units as soon as a column is repeated or contains nulls. Concretely, with rep levels {0,0,0} and def levels {0,0,1} (two empty rows then one value), valuesToSkip resolves to 0, so toSkip never decreases and Skip walks past the requested row. Your toSkip -= rowsSkipped is the correct fix.
Two things need addressing:
- The compile error (inline below) — this is the blocker.
- A tautological assertion in the test (inline below).
Once those are sorted I'm happy to merge this. Please also rebase, since main has moved a fair bit today.
| } | ||
|
|
||
| skipped, err := c.curDecoder.Discard(int(valuesToSkip)) | ||
| _, err = c.curDecoder.Discard(int(valuesToSkip)) |
There was a problem hiding this comment.
This does not compile. Changing skipped, err := ... to _, err = ... removes the only declaration of err in this scope — there is no err in any enclosing scope (skipRows has no named return, and the sibling branch above declares its own via :=). I reproduced this:
vet: undefined: err
Easiest fix is to keep the short variable declaration, which is legal here because err is still a new variable even with _ on the left:
_, err := c.curDecoder.Discard(int(valuesToSkip))Alternatively declare var err error before the loop.
| } | ||
|
|
||
| toSkip -= int64(skipped) | ||
| toSkip -= rowsSkipped |
There was a problem hiding this comment.
For the record, this line is the actual fix and it is correct. rowsSkipped is counted in logical rows in the loop above, matching the units of toSkip, whereas the previous int64(skipped) was a physical value count. Note that in the found-enough-rows branch the loop breaks when rowsSkipped == toSkip, so this correctly drives toSkip to zero.
| typed := col.(*file.Int32ColumnChunkReader) | ||
| skipped, err := typed.Skip(2) | ||
| require.NoError(t, err) | ||
| assert.EqualValues(t, 2, skipped) |
There was a problem hiding this comment.
This assertion can never fail, so it gives no protection. Int32ColumnChunkReader.Skip (in column_reader_types.gen.go) returns its nvalues argument unconditionally, regardless of what skipRows actually did — so skipped is always 2 here even with the bug present.
The ReadBatch assertions below are the meaningful part of this test and they do exercise the fix. Please either drop this line or, better, assert on reader state that actually reflects the skip (for example that the remaining value count decreased by 2).
Repeated-column skipping tracks logical rows and physical values separately, but the remaining skip count was being reduced by physical values. Empty repeated rows could therefore make
Skipmove past the requested row.This keeps the remaining count in logical-row units and adds a regression with two empty rows followed by a non-null value.
Tests:
go test ./parquet/file -run TestSkipEmptyRepeatedRows\|TestPrimitiveReader -count=1