Skip to content

fix(parquet/file): count repeated skips in logical rows #1002

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zeroshade merged 1 commit into from
Jul 27, 2026
+42 −2
Merged

fix(parquet/file): count repeated skips in logical rows #1002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions parquet/file/column_reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,13 +708,13 @@ func (c *columnChunkReader) skipRows(nrows int64) error {
valuesToSkip = int64(levelsToSkip)
}

skipped, err := c.curDecoder.Discard(int(valuesToSkip))
_, err = c.curDecoder.Discard(int(valuesToSkip))

@zeroshade zeroshade Jul 27, 2026

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.

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.

if err != nil {
c.err = err
return err
}

toSkip -= int64(skipped)
toSkip -= rowsSkipped

@zeroshade zeroshade Jul 27, 2026

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.

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.

c.consumeBufferedValues(int64(levelsToSkip))
}
}
Expand Down
40 changes: 40 additions & 0 deletions parquet/file/column_reader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,46 @@ func (p *PrimitiveReaderSuite) TestInt32FlatRepeated() {
p.testDict(npages, levelsPerPage, d, reflect.TypeOf(int32(0)))
}

func TestSkipEmptyRepeatedRows(t *testing.T) {
sc := schema.NewSchema(schema.MustGroup(schema.NewGroupNode("schema", parquet.Repetitions.Required, schema.FieldList{
schema.NewInt32Node("values", parquet.Repetitions.Repeated, -1),
}, -1)))

var out bytes.Buffer
w := file.NewParquetWriter(&out, sc.Root(), file.WithWriterProps(
parquet.NewWriterProperties(parquet.WithDictionaryDefault(false))))
rg := w.AppendRowGroup()
cw, err := rg.NextColumn()
require.NoError(t, err)
_, err = cw.(*file.Int32ColumnChunkWriter).WriteBatch(
[]int32{42}, []int16{0, 0, 1}, []int16{0, 0, 0})
require.NoError(t, err)
require.NoError(t, cw.Close())
require.NoError(t, rg.Close())
require.NoError(t, w.Close())

r, err := file.NewParquetReader(bytes.NewReader(out.Bytes()))
require.NoError(t, err)
defer r.Close()

col, err := r.RowGroup(0).Column(0)
require.NoError(t, err)
typed := col.(*file.Int32ColumnChunkReader)
_, err = typed.Skip(2)
require.NoError(t, err)

values := make([]int32, 1)
defs := make([]int16, 1)
reps := make([]int16, 1)
levelsRead, valuesRead, err := typed.ReadBatch(1, values, defs, reps)
require.NoError(t, err)
assert.EqualValues(t, 1, levelsRead)
assert.Equal(t, 1, valuesRead)
assert.Equal(t, []int32{42}, values)
assert.Equal(t, []int16{1}, defs)
assert.Equal(t, []int16{0}, reps)
}

func (p *PrimitiveReaderSuite) TestReadBatchMultiPage() {
const (
levelsPerPage int = 100
Expand Down
Loading