vinooganesh commented on PR #3397: URL: https://github.com/apache/parquet-java/pull/3397#issuecomment-5559330579
@wgtmac thanks again for the review, it was a genuinely useful one. I've gone through all of your comments and replied to each thread individually. A summary of where things landed: The three reader issues are fixed in 338fb99c6. You were right that numElements could be forged up to Integer.MAX_VALUE, and while fixing it I found the allocation was reachable rather than theoretical, since MultiBufferInputStream.slice allocates before its EOF check. The vector count is bounded against the bytes actually present now, the offset array is validated up front, each vector's reads are confined by the following offset, and skip no longer overflows. The sampling problem you spotted was the most valuable comment in the review. Your arithmetic was exactly right: reset() ran at every page boundary and cleared the sample state, so the threshold was never reached and the preset cache was never built. Fixed in eb0e5941d, with a regression test that fails without it. The two test comments were both fair. The allocation test was inflating valuesCount rather than forging num_elements so it never reached the path it claimed to, and catchAny was swallowing Errors so it could not really fail. Both fixed in 338fb99c6. Beyond that: ALP is configured through a single AlpConfig now with withAlp (99ae547c5), AlpConstants is package private with the constants moved to whichever class actually uses them, AlpEncoderDecoder is renamed to AlpCodec, the wildcard imports are gone and the javadoc box is fixed (78c1d182a), ALP is declared directly in parquet.thrift with the perl patch deleted (229e0d99b), and the double encoding is gone (8e743c60a). There are three places where I would like your opinion rather than assuming I picked right: - To make withAlp work without breaking existing callers I added getDefaultValue and getValue to ColumnProperty.Builder. It is package private so not public API, but it is a shared file rather than something ALP specific. - dev/update-parquet-thrift.sh overwrites parquet.thrift from upstream and will silently drop the ALP entry. I put a marked notice in the enum so the loss at least shows up in a diff, but a guard in the script might be better. - On the nested writer classes, I followed the ByteStreamSplit layout rather than choosing it deliberately. Happy to split them if you would rather ALP be internally consistent. The one thing I cannot fix on my own is the interop test. The fixtures are proposed in apache/parquet-testing#100, which is still open, so the test skips in CI today. If you have any pull there it would help, since cross language read back matters for the vote more than for that test alone. parquet-column is at 856 tests with no failures. Whenever you have time for another look, I would appreciate it. -- 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]
