Repository: parquet-go/parquet-go (go)

Issue:
Optimistically reading file footer breaks Tempo caching logic
I like [this recent PR](https://github.com/parquet-go/parquet-go/pull/192) and believe it will help in the general case, but Tempo has logic to [immediately return the footer length and cache the footer](https://github.com/grafana/tempo/blob/e61733554c09fb59d6990eff48dc9c511b5c22cf/tempodb/encoding/vparquet4/readers.go#L98-L114) in an off process cache. This PR breaks that behavior by changing the ranges it attempts to read.

Are we good on a PR to make this new behavior configurable? I'd be glad to submit it.







Pull request description:
add optimistic read file option
Fixes #193 

Patch:
diff --git a/config.go b/config.go
index 6d7025f..45ff8ba 100644
--- a/config.go
+++ b/config.go
@@ -94,6 +94,7 @@ type FileConfig struct {
 	SkipMagicBytes   bool
 	SkipPageIndex    bool
 	SkipBloomFilters bool
+	OptimisticRead   bool
 	ReadBufferSize   int
 	ReadMode         ReadMode
 	Schema           *Schema
@@ -467,6 +468,17 @@ func SkipBloomFilters(skip bool) FileOption {
 	return fileOption(func(config *FileConfig) { config.SkipBloomFilters = skip })
 }
 
+// OptimisticRead configures a file to optimistically perform larger buffered
+// reads to improve performance. This is useful when reading from remote storage
+// and amortize the cost of network round trips.
+//
+// This is an option instead of enabled by default because dependents of this
+// package have historically relied on the read patterns to provide external
+// caches and achieve similar results (e.g., Tempo).
+func OptimisticRead(enabled bool) FileOption {
+	return fileOption(func(config *FileConfig) { config.OptimisticRead = enabled })
+}
+
 // FileReadMode is a file configuration option which controls the way pages
 // are read. Currently the only two options are ReadModeAsync and ReadModeSync
 // which control whether or not pages are loaded asynchronously. It can be
diff --git a/file.go b/file.go
index aff3b6e..f9bc9bb 100644
--- a/file.go
+++ b/file.go
@@ -63,7 +63,10 @@ func OpenFile(r io.ReaderAt, size int64, options ...FileOption) (*File, error) {
 		cast.SetMagicFooterSection(size-8, 8)
 	}
 
-	optimisticFooterSize := max(8, min(int64(c.ReadBufferSize), size))
+	optimisticFooterSize := min(int64(c.ReadBufferSize), size)
+	if !c.OptimisticRead || optimisticFooterSize < 8 {
+		optimisticFooterSize = 8
+	}
 	optimisticFooterData := make([]byte, optimisticFooterSize)
 
 	if n, err := r.ReadAt(optimisticFooterData, size-optimisticFooterSize); n != len(optimisticFooterData) {


Test patch:
diff --git a/file_test.go b/file_test.go
index 10d4e64..f1a5b52 100644
--- a/file_test.go
+++ b/file_test.go
@@ -34,7 +34,8 @@ func TestOpenFile(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			p, err := parquet.OpenFile(f, s.Size())
+			p, err := parquet.OpenFile(f, s.Size(),
+				parquet.OptimisticRead(true))
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -71,7 +72,8 @@ func TestOpenFileWithoutPageIndex(t *testing.T) {
 			if err != nil {
 				t.Fatal(err)
 			}
-			fileWithoutIndex, err := parquet.OpenFile(f, s.Size(), parquet.SkipPageIndex(true))
+			fileWithoutIndex, err := parquet.OpenFile(f, s.Size(),
+				parquet.SkipPageIndex(true))
 			if err != nil {
 				t.Fatal(err)
 			}