zeroshade commented on code in PR #1002:
URL: https://github.com/apache/arrow-go/pull/1002#discussion_r3660288743
##########
parquet/file/column_reader.go:
##########
@@ -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))
if err != nil {
c.err = err
return err
}
- toSkip -= int64(skipped)
+ toSkip -= rowsSkipped
Review 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.
##########
parquet/file/column_reader.go:
##########
@@ -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))
Review 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:
```go
_, err := c.curDecoder.Discard(int(valuesToSkip))
```
Alternatively declare `var err error` before the loop.
##########
parquet/file/column_reader_test.go:
##########
@@ -465,6 +465,47 @@ 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)
+ skipped, err := typed.Skip(2)
+ require.NoError(t, err)
+ assert.EqualValues(t, 2, skipped)
Review 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
