hcrosse opened a new pull request, #1447: URL: https://github.com/apache/arrow-go/pull/1447
### Rationale for this change Optional columns take the spaced decode path even when a batch has no nulls. With `SetReadDict` enabled, that includes building a per-value validity slice before appending dictionary indices. I measured 2M synthetic 24-byte strings in one dictionary-encoded row group, with `SetReadDict` enabled, over 10 interleaved rounds on one 8-vCPU AMD EPYC Linux amd64 VM running Go 1.26.10. These are median times for optional columns with no nulls, comparing main at `22c9f26d` with this change. | Distinct values | Rows/batch | Main → fix, ns/row | Reduction | | --- | ---: | ---: | ---: | | 1k | 8,192 | 10.19 → 6.53 | 36% | | 1k | 65,536 | 8.51 → 4.70 | 45% | | 100k | 8,192 | 180.4 → 174.5 | 3% | | 100k | 65,536 | 36.31 → 32.78 | 10% | benchstat gives `p<0.001` for three rows and `p=0.004` for 100k at 8,192 rows per batch. In that case most of the time goes to copying the dictionary into every batch, which https://github.com/apache/arrow-go/pull/1444 addresses. Required columns stayed within noise. A version of this change without the count checks ran at the same speed. I can add the benchmark as a `pqarrow` benchmark if that would help. ### What changes are included in this PR? Batches with no nulls now use dense decoding. `DefLevelsToBitmap` still writes their validity bits first, and batches with nulls keep spaced decoding. Each `ReadValuesDense` implementation also checks the decoded count. It returns `parquet: number of values / definition levels read did not match` when the decoder yields fewer values than requested. ### Are these changes tested? I added two tests. - `TestReadRecordsDenseForBatchesWithoutNulls` reads pages with nulls, without nulls, then with nulls in one call. It checks the path for each page, the values and the validity bits. - `TestReadRecordsRejectsShortPage` changes a `DELTA_BINARY_PACKED` header to claim two values instead of four. It checks that `ReadRecords` returns an error for optional and required columns. On main, the path test fails and the short-page test fails for required columns. On Linux, `go test -tags assert,test ./parquet/...` passes with the same per-package results as main. The same suite passes with `-race` on both trees on a 32 GB VM. The `prek` hooks pass, including `golangci-lint` and the Release Audit Tool. I have not tested short pages for every encoding, `noasm`, arm64 or 32-bit builds, or real-world files. I benchmarked only dictionary-encoded strings with `SetReadDict` enabled, and not columns that contain nulls. ### Are there any user-facing changes? Optional columns with no nulls read faster in these benchmarks. There is no API change. Required columns now return an error when a page's decoder yields fewer values than requested. On main they returned no error. Optional columns already returned an error in this case on the spaced path, and batches that now take the dense path keep it, although the message can differ slightly from the decoder's own. Closes https://github.com/apache/arrow-go/issues/1445. -- 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]
