vinooganesh commented on PR #3397: URL: https://github.com/apache/parquet-java/pull/3397#issuecomment-5650398149
@RussellSpitzer thanks for the review and the approval. Three of the four nits are done, and I am still working through the thrift one. **Java docs** — f7f7273aa. The one you spotted described the build-time patch that was deleted in 229e0d99b, so I dropped the detail rather than updating it. I grepped for anything else mentioning the patch or a local addition and that was the only one left. **Skips** — the `testEnumEquivalence` skip is gone in 0fcd73e98, so ALP round trips like every other encoding now. One judgement call for you: `TestInterOpReadAlp` also skips when pyarrow reports "Unknown encoding type". That is a skip about ALP not existing, but in pyarrow rather than here, and removing it makes the test fail on most machines. Happy to pull it if you would rather it fail loudly. **`withoutAlp(column)`** — a8c584f64. It was missing because ColumnProperty treated a per-column null as unset and fell back to the default, so it checks containsKey now and an explicit null overrides; every other per-column property either cannot hold a null or already defaults to one, so nothing else changes. Two fixes had to come with it: the schema check was reading "named in the ALP property" as "enabled", so disabling ALP on an int column failed with a message claiming ALP was enabled on it, and toString rendered a disabled column as null, which undid your other nit. **Thrift** — still looking into it. There is a wrinkle in doing it as a precursor PR that I want to understand properly before I propose something, so I will follow up on that thread. Your approval is against e660c6cec, so it misses the four commits since. Another look whenever you get a chance would be great. -- 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]
