This is an automated email from the ASF dual-hosted git repository.
zeroshade pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-go.git
The following commit(s) were added to refs/heads/main by this push:
new 2eae1935 fix(parquet/file): close file on OpenParquetFile errors
(#1053)
2eae1935 is described below
commit 2eae193511c15d73ad473057f2db3597ebb560a0
Author: Minh Vu <[email protected]>
AuthorDate: Wed Aug 5 20:03:33 2026 +0200
fix(parquet/file): close file on OpenParquetFile errors (#1053)
### Rationale for this change
`OpenParquetFile` opens a file or mmap before constructing the reader.
If initialization fails while parsing metadata, the function returned
without closing that owned source.
### What changes are included in this PR?
Require the internally opened source to implement `ReaderAt`, `Seeker`,
and `Closer`, then close it when `NewParquetReader` fails. Preserve a
simultaneous close failure with `errors.Join`.
### Are these changes tested?
Yes. A close-tracking regression test verifies that malformed input
closes the owned source, and the mmap signature cross-compiles on
Windows.
`go test ./parquet/file -run TestOpenParquetFileClosesSourceOnError`
### Are there any user-facing changes?
Invalid files still return an error, but their file descriptor or mmap
is released promptly.
---
parquet/file/file_reader.go | 17 ++++++++--
parquet/file/file_reader_mmap.go | 3 +-
parquet/file/file_reader_mmap_windows.go | 8 ++---
parquet/file/open_file_internal_test.go | 55 ++++++++++++++++++++++++++++++++
4 files changed, 73 insertions(+), 10 deletions(-)
diff --git a/parquet/file/file_reader.go b/parquet/file/file_reader.go
index 7b6df98a..5b6978ab 100644
--- a/parquet/file/file_reader.go
+++ b/parquet/file/file_reader.go
@@ -42,6 +42,11 @@ var (
errInconsistentFileMetadata = errors.New("parquet: file is smaller than
indicated metadata size")
)
+type readerAtSeekerCloser interface {
+ parquet.ReaderAtSeeker
+ io.Closer
+}
+
// Reader is the main interface for reading a parquet file
type Reader struct {
r parquet.ReaderAtSeeker
@@ -78,7 +83,7 @@ func WithMetadata(m *metadata.FileMetaData) ReadOption {
// then the default ReaderProperties will be used. The WithMetadata option can
be used to provide
// a FileMetaData object rather than reading the file metadata from the file.
func OpenParquetFile(filename string, memoryMap bool, opts ...ReadOption)
(*Reader, error) {
- var source parquet.ReaderAtSeeker
+ var source readerAtSeekerCloser
var err error
if memoryMap {
@@ -92,7 +97,15 @@ func OpenParquetFile(filename string, memoryMap bool, opts
...ReadOption) (*Read
return nil, err
}
}
- return NewParquetReader(source, opts...)
+ return newParquetReaderFromFile(source, opts...)
+}
+
+func newParquetReaderFromFile(source readerAtSeekerCloser, opts ...ReadOption)
(*Reader, error) {
+ rdr, err := NewParquetReader(source, opts...)
+ if err != nil {
+ return nil, errors.Join(err, source.Close())
+ }
+ return rdr, nil
}
// NewParquetReader returns a FileReader instance that reads a parquet file
which can be read from r.
diff --git a/parquet/file/file_reader_mmap.go b/parquet/file/file_reader_mmap.go
index 727a3879..4a4455de 100644
--- a/parquet/file/file_reader_mmap.go
+++ b/parquet/file/file_reader_mmap.go
@@ -23,11 +23,10 @@ import (
"errors"
"io"
- "github.com/apache/arrow-go/v18/parquet"
"golang.org/x/exp/mmap"
)
-func mmapOpen(filename string) (parquet.ReaderAtSeeker, error) {
+func mmapOpen(filename string) (readerAtSeekerCloser, error) {
rdr, err := mmap.Open(filename)
if err != nil {
return nil, err
diff --git a/parquet/file/file_reader_mmap_windows.go
b/parquet/file/file_reader_mmap_windows.go
index 92e9620a..5e46adbd 100644
--- a/parquet/file/file_reader_mmap_windows.go
+++ b/parquet/file/file_reader_mmap_windows.go
@@ -19,12 +19,8 @@
package file
-import (
- "errors"
+import "errors"
- "github.com/apache/arrow-go/v18/parquet"
-)
-
-func mmapOpen(_ string) (parquet.ReaderAtSeeker, error) {
+func mmapOpen(_ string) (readerAtSeekerCloser, error) {
return nil, errors.New("mmap not implemented on windows")
}
diff --git a/parquet/file/open_file_internal_test.go
b/parquet/file/open_file_internal_test.go
new file mode 100644
index 00000000..b83e5df7
--- /dev/null
+++ b/parquet/file/open_file_internal_test.go
@@ -0,0 +1,55 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package file
+
+import (
+ "bytes"
+ "errors"
+ "testing"
+
+ "github.com/stretchr/testify/require"
+)
+
+type closeTrackingReader struct {
+ *bytes.Reader
+ closed bool
+ closeErr error
+}
+
+func (r *closeTrackingReader) Close() error {
+ r.closed = true
+ return r.closeErr
+}
+
+func TestOpenParquetFileJoinsSourceCloseErrors(t *testing.T) {
+ closeErr := errors.New("close failed")
+ source := &closeTrackingReader{Reader:
bytes.NewReader([]byte("invalid")), closeErr: closeErr}
+ rdr, err := newParquetReaderFromFile(source)
+ require.Error(t, err)
+ require.Nil(t, rdr)
+ require.ErrorIs(t, err, closeErr)
+ require.True(t, source.closed)
+}
+
+func TestOpenParquetFileClosesSourceOnError(t *testing.T) {
+ source := &closeTrackingReader{Reader:
bytes.NewReader([]byte("invalid"))}
+ rdr, err := newParquetReaderFromFile(source)
+ require.Error(t, err)
+ require.Nil(t, rdr)
+ require.True(t, source.closed)
+}
