Skip to content

perf(parquet/pqarrow): use synchronous path for serial reads - #1235

Open
fallintoplace wants to merge 2 commits into
apache:mainfrom
fallintoplace:perf/parquet-pqarrow-sync
Open

perf(parquet/pqarrow): use synchronous path for serial reads#1235
fallintoplace wants to merge 2 commits into
apache:mainfrom
fallintoplace:perf/parquet-pqarrow-sync

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

  • ArrowReadProperties.Parallel is false by default.
  • GetFieldReaders now uses a direct loop for serial reads.
  • ReadRowGroups now reads columns directly for serial reads.
  • The parallel path is unchanged.
  • Added a benchmark for 1, 8, 32, and 128 columns with small row groups.

Benchmark

Apple M1 Pro. Median of 5 runs. The benchmark uses Parallel=false.

columns upstream main this PR time change allocs/op
1 156.9 us 128.8 us -18% 1124 -> 1106
8 1.05 ms 967 us -8% 7848 -> 7810
32 3.38 ms 3.24 ms -4% 31413 -> 31296
128 11.92 ms 11.18 ms -6% 124896 -> 124436

Command:

go test ./parquet/pqarrow -run '^$' -bench '^BenchmarkReadTableSerial$' -benchmem -benchtime=1s -count=5

Tests

  • go test ./parquet/pqarrow -count=1
  • go test -race ./parquet/pqarrow -run '^(TestGetFieldReadersReleasesPartialReadersOnError|TestRecordReaderSerial|TestRecordReaderSeekToRow|TestRecordReaderParallel)$' -count=1
  • go vet ./parquet/pqarrow

@zeroshade zeroshade left a comment

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.

The serial-path performance and allocation improvements are reproducible, and the standard tests pass. However, the synchronous path removes an existing panic-recovery boundary, causing malformed decoder or extension-array input to panic callers rather than return an error.

Comment thread parquet/pqarrow/file_reader.go Outdated
return nil, err
}

data, err := fr.ReadColumn(rowGroups, rdr)

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.

ReadColumn previously ran inside the worker’s deferred recovery boundary, which converted decoder panics into panic while reading: … errors. Calling it directly here lets those panics escape from ReadTable.

I reproduced this through the public FileReader flow with a malformed extension implementation: the base returns the recovered error, while this head panics. Please preserve the recovery boundary for synchronous reads—ideally through a shared guarded helper—and add regression coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants