viirya commented on PR #59118: URL: https://github.com/apache/spark/pull/59118#issuecomment-5917360965
Thanks @dongjoon-hyun. Updated in 01e36736eca, rebased onto master: - Added a test to `ParquetVectorizedSuite` that writes a required BYTE_STREAM_SPLIT page with 3 encoded values and a value count of 4 to a `MemPageStore`, with v1 and v2 pages, and reads it through `VectorizedParquetRecordReader` with a batch size of 2. It asserts the init message. Passing `false` for the exact check in `VectorizedColumnReader` fails it, since no exception is thrown for the first batch. - The `ParquetColumnIndexSuite` test now asserts that `_2` to `_5` are BYTE_STREAM_SPLIT, that `_1` has more than one page, and that a page boundary of `_1` falls inside a page of each column expected to be unaligned. You're right about v1: from the offset indexes, `_1`, `_3` and `_5` all get 200-row pages and `_2`/`_4` 300-row pages. The comment now says that only the FLOAT columns are unaligned with v1 pages. The round trip in `ParquetEncodingSuite` also asserts more than one page per column and builds its records from `expected`. - E > D: agreed, I'd like to do it as a follow-up in SPARK-59902. I prototyped the end-of-page check you described (after the `defColumn.readBatch` / `repColumn.readBatchRepeated` calls, when `valuesToReadInPage == 0`), and it ran on 1454 pages of the BYTE_STREAM_SPLIT tests in the Parquet suites, including the array, nullable and column index cases with both page versions, without a false positive. Since a false positive would reject valid files, I'd like to cover more layouts, such as nested structs and maps and row ranges on nested columns, before adding it. - `initFromPage` computes the encoded value count in a local and assigns it after the checks, the upper bound check says `exceed` again, the length message says `page data length`, and the field Javadoc says it counts encoded values. `readBinary(int len)` rejects a `len` other than the value width with an `IllegalArgumentException` before consuming a value. - Applied the test nits. I kept `toBytes`, so that the expected bytes don't depend on the parquet writer. - Re-ran the A/B microbenchmark on this commit, master's reader against this one: per-value `readInteger`/`readLong`/`readDouble`/`readBinary(12)` and batch `readIntegers`/`readDoubles` show no measurable difference. - The missing column name in the messages also applies to the sibling readers, so I'll leave it for a separate change. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
