You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jp0317 (via GitHub)" <gi...@apache.org> on 2024/08/13 04:56:33 UTC
[PR] Check the number of rep levels in Parquet read_records [arrow-rs]
jp0317 opened a new pull request, #6232:
URL: https://github.com/apache/arrow-rs/pull/6232
# Which issue does this PR close?
This fixes one case of #6228.
# Rationale for this change
In read_records, the number of rep levels read from page can be 0 if the file is corrupted. But currently we don't check that and in that case the execution will stuck in the read_records, keeping reading but reading nothing.
# What changes are included in this PR?
Error out if reading 0 rep level.
# Are there any user-facing changes?
not really.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Check the number of rep levels in Parquet read_records [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6232:
URL: https://github.com/apache/arrow-rs/pull/6232#issuecomment-2292360662
> I've merge that in parquet-testing, feel free to check!
Thank you @mapleFU
I double checked this PR against the checks in https://github.com/apache/arrow-rs/pull/6262 and confirmed that prior to the PR the file loops indefinitely and after this PR it generates an error 👍
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Check the number of rep levels in Parquet read_records [arrow-rs]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #6232:
URL: https://github.com/apache/arrow-rs/pull/6232#issuecomment-2291044430
I've merge that in parquet-testing, feel free to check!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Check the number of rep levels in Parquet read_records [arrow-rs]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #6232:
URL: https://github.com/apache/arrow-rs/pull/6232#issuecomment-2286610834
Yeah pitrou is not here perhaps I can merge that pr first and just testing them 🤔
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Avoid infinite loop in bad parquet by checking the number of rep levels [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6232:
URL: https://github.com/apache/arrow-rs/pull/6232
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Check the number of rep levels in Parquet read_records [arrow-rs]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #6232:
URL: https://github.com/apache/arrow-rs/pull/6232#issuecomment-2285906751
BTW @alamb @jp0317 Would you mind also check https://github.com/apache/parquet-testing/pull/48#pullrequestreview-2235162383 ?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Check the number of rep levels in Parquet read_records [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6232:
URL: https://github.com/apache/arrow-rs/pull/6232#issuecomment-2288699319
> Thanks @alamb ! It's from [this file](https://github.com/jp0317/parquet-testing/commit/d6ad56b337166cc8c36eda2496812a05beca1368#diff-ffa316b21b9b73ab50137c50b30d8df9933f2da2af01455d08af0917074972b6) which causes infinite loop. I'll send to PR for parquet-testing then add a test correspondingly.
Thank you 🙏
> Besides, it seems the current tests did not use anything from the parquet-testing/bad_data so the test coverage on irregular inputs seems to be near zero?
Yes, I think this is accurate. I will file a ticket to track adding additional negative test coverage
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Check the number of rep levels in Parquet read_records [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6232:
URL: https://github.com/apache/arrow-rs/pull/6232#discussion_r1715028692
##########
parquet/src/column/reader.rs:
##########
@@ -240,6 +240,12 @@ where
let (mut records_read, levels_read) =
reader.read_rep_levels(out, remaining_records, remaining_levels)?;
+ if levels_read == 0 {
+ // Tha fact that we're still looping implies there must be some levels to read.
Review Comment:
```suggestion
// The fact that we're still looping implies there must be some levels to read.
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Check the number of rep levels in Parquet read_records [arrow-rs]
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on PR #6232:
URL: https://github.com/apache/arrow-rs/pull/6232#issuecomment-2286831544
Hi @alamb , I updated the check to also involve `records_read` as it seems valid to have `(records_read, levels_read)` as `(1, 0)`. PTAL! thanks.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Check the number of rep levels in Parquet read_records [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6232:
URL: https://github.com/apache/arrow-rs/pull/6232#issuecomment-2292360958
Thanks again @jp0317 -- a very nice contribution indeed!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Check the number of rep levels in Parquet read_records [arrow-rs]
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on PR #6232:
URL: https://github.com/apache/arrow-rs/pull/6232#issuecomment-2287378207
Thanks @alamb ! It's from [this file](https://github.com/jp0317/parquet-testing/commit/d6ad56b337166cc8c36eda2496812a05beca1368#diff-ffa316b21b9b73ab50137c50b30d8df9933f2da2af01455d08af0917074972b6) which causes infinite loop. I'll send to PR for `parquet-testing` then add a test correspondingly.
Besides, it seems the current tests did not use anything from the `parquet-testing/bad_data` so the test coverage on irregular inputs seems to be near zero?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Check the number of rep levels in Parquet read_records [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6232:
URL: https://github.com/apache/arrow-rs/pull/6232#issuecomment-2286585837
> BTW @alamb @jp0317 Would you mind also check [apache/parquet-testing#48](https://github.com/apache/parquet-testing/pull/48) ?
Looks like a good PR.
@mapleFU - are you thinking we should add some tests for reading that bad data from the parquet-testing repo and verify that the parquet reader errors correctly?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Check the number of rep levels in Parquet read_records [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6232:
URL: https://github.com/apache/arrow-rs/pull/6232#issuecomment-2286813048
🤔 the tests appear to be failing now 🤔
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org