fix(parquet/file): count repeated skips in logical rows - #1002
Conversation
fallintoplace
changed the title
fallintoplace
changed the title
fallintoplace
changed the title
zeroshade
left a comment
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).
374f18a to
1d06e13
Compare
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Re-reviewed at 1d06e13c. Both of my concerns are resolved and CI is green (22/22), so this is good to go.
On the compile error: interesting outcome — the production hunk here is byte-identical to what I reviewed before, but it now compiles. The reason is that main moved underneath it: skipRows now reads nreps, _, err := c.repetitionDecoder.Decode(repLvls), which declares err in that else block (it was nreps, _ := when I first looked). So _, err = c.curDecoder.Discard(...) resolves against that declaration and the rebase alone fixed it. Worth noting for the record that this line's compilation depends on a declaration several statements earlier rather than being locally self-evident, but it is correct and idiomatic as written.
On the test: the tautological assert.EqualValues(t, 2, skipped) is gone, replaced with _, err = typed.Skip(2). Good — the remaining ReadBatch assertions are the ones that actually exercise the fix, verifying the reader lands on the real value (42, def=1, rep=0) rather than skipping past it.
Re-confirming the substance: toSkip is denominated in logical rows but was decremented by a physical value count, so with rep {0,0,0} / def {0,0,1} the two empty rows contribute zero physical values and toSkip never decreased. toSkip -= rowsSkipped is the correct unit. LGTM.
2 participants
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