Skip to content

GH-34807: [Go] Handle io.EOF when reading parquet footer size and magic bytes #34808

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 5 commits into from
Apr 10, 2023
Merged

GH-34807: [Go] Handle io.EOF when reading parquet footer size and magic bytes #34808

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fb40210
Properly handle io.EOF when reading footer size and magic bytes
agchang Mar 30, 2023
6b0cae6
Add a test
agchang Apr 5, 2023
951a3c0
Simplify test
agchang Apr 5, 2023
65ed4f6
Run goimports
agchang Apr 9, 2023
b5c77d8
fix import formatting
zeroshade Apr 10, 2023
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
2 changes: 1 addition & 1 deletion go/parquet/file/file_reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ func (f *Reader) parseMetaData() error {
buf := make([]byte, footerSize)
// backup 8 bytes to read the footer size (first four bytes) and the magic bytes (last 4 bytes)
n, err := f.r.ReadAt(buf, f.footerOffset-int64(footerSize))
if err != nil {
if err != nil && err != io.EOF {
return fmt.Errorf("parquet: could not read footer: %w", err)
}
if n != len(buf) {
Expand Down
30 changes: 30 additions & 0 deletions go/parquet/file/file_reader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ package file_test
import (
"bytes"
"encoding/binary"
"github.com/apache/arrow/go/v12/parquet"
"github.com/apache/arrow/go/v12/parquet/schema"
"github.com/stretchr/testify/require"

@zeroshade zeroshade Apr 6, 2023

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.

can you run go fmt on this / goimports so it reorders the imports sorting them appropriately? that's likely what's causing the failing dev script CI tests, these are getting ordered differently resulting in the diffs not being what is expected.

@zeroshade zeroshade Apr 6, 2023

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.

I should really add a workflow for the bot to auto-run the formatting like we have for C++ and clang-format...

@agchang agchang Apr 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Thanks for the pointer.

I should really add a workflow for the bot to auto-run the formatting like we have for C++ and clang-format...

I can take a look if you guys need help on the go front :-)

@agchang agchang Apr 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, still have failing checks, I ran:
go fmt -d file_reader.go file_reader_test.go and see no diffs 🤔

@zeroshade zeroshade Apr 10, 2023

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.

you needed to remove the empty line and then use goimports to sort the imports. I've done so and pushed the fix.

@agchang agchang Apr 10, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for fixing.

"io"
"math/rand"
"testing"
Expand Down Expand Up @@ -65,6 +68,19 @@ func checkStatistics(t *testing.T, stats format.Statistics, actual metadata.Enco
}
}

type testReader struct {
*bytes.Reader
}

// ReadAt for testReader returns io.EOF when off + len(b) is exactly the length of the underlying input source.
func (tr testReader) ReadAt(b []byte, off int64) (int, error) {
n, err := tr.Reader.ReadAt(b, off)
if err == nil && (int64(n)+off == tr.Size()) {
return n, io.EOF
}
return n, err
}

type PageSerdeSuite struct {
suite.Suite

Expand Down Expand Up @@ -271,6 +287,20 @@ func (p *PageSerdeSuite) TestCompression() {
}
}

func TestWithEOFReader(t *testing.T) {
root, _ := schema.NewGroupNode("schema", parquet.Repetitions.Repeated, schema.FieldList{
schema.NewInt32Node("int_col", parquet.Repetitions.Required, -1)}, -1)
props := parquet.NewWriterProperties(parquet.WithVersion(parquet.V2_LATEST))

var buf bytes.Buffer
wr := file.NewParquetWriter(&buf, root, file.WithWriterProps(props))
require.NoError(t, wr.Close())

r := bytes.NewReader(buf.Bytes())
_, err := file.NewParquetReader(testReader{Reader: r})
assert.NoError(t, err)
}

func TestInvalidHeaders(t *testing.T) {
badHeader := []byte("PAR2")
_, err := file.NewParquetReader(bytes.NewReader(badHeader))
Expand Down