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/11/16 06:01:55 UTC
[I] Convert some panics that happen on invalid parquet files to error results [arrow-rs]
jp0317 opened a new issue, #6737:
URL: https://github.com/apache/arrow-rs/issues/6737
This is a follow-up for the discussion [here](https://github.com/apache/arrow-rs/issues/5332#issuecomment-2460195372). In short, the parquet implementation may panic upon invalid inputs. IIUC ideally we should avoid panics as much as possible, especially those can be converted to error results with small efforts.
--
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.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Avoid panics? [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6737:
URL: https://github.com/apache/arrow-rs/issues/6737#issuecomment-2615470438
`label_issue.py` automatically added labels {'parquet'} from #6738
--
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: [I] Avoid panics? [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6737:
URL: https://github.com/apache/arrow-rs/issues/6737#issuecomment-2549626308
> It seems there is potentially an overall project question of when is using a panic desired and what efforts should be made to clean up existing code with this standard?
Yes I think having clear guidelines on when to panic (basically an uncatchable panic) vs return errors would be helpful
I think there is a tension between the decreased usability of APIs that require handling / propagating errors and allowing errors to propagate. In my opinion, if an API returns an error, it is an error that some program might want to catch. If the only errors that can be returned are due to bugs in code (aka internals errors) it is less clear to me an error is appropriate in that case
For invalid inputs beyond the control of the calling program (e.g. invalid parquet files) i think returning errors rather than panic is a very reasonable behavior
--
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: [I] Avoid panics? [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6737:
URL: https://github.com/apache/arrow-rs/issues/6737#issuecomment-2549762938
> Would this be something to discuss on the mailing list (or in this issue) or something else? I understand this can be contentious and I don't have enough knowledge to actually say what is right for the project but would like to see it move forward so happy to help out where I can.
I recommend we keep the conversation on this ticket as the convention is communication happens on github and thus is is also easier for people to find / discover in the future
I have had good luck in the past sending notes to the mailing list with a pointer to ticket when looking to solicit wider feedback, for example: https://lists.apache.org/thread/lqoh9mkss6xlod3380h85pjzowfsysw1
--
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: [I] Avoid panics? [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6737:
URL: https://github.com/apache/arrow-rs/issues/6737#issuecomment-2555742382
This is great. Thank you
Is anyone willing to try and make a PR to put what appears to be a consensus into the project readme?
Perhaps a "Guidelines for `panic` vs `Result`" section near https://github.com/apache/arrow-rs/blob/main/README.md#deprecation-guidelines
--
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: [I] Avoid panics? [arrow-rs]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on issue #6737:
URL: https://github.com/apache/arrow-rs/issues/6737#issuecomment-2532777597
It seems there is potentially an overall project question of when is using a panic desired and what efforts should be made to clean up existing code with this standard?
--
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: [I] Avoid panics? [arrow-rs]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on issue #6737:
URL: https://github.com/apache/arrow-rs/issues/6737#issuecomment-2549674887
Also CC @etseidl who I think had opinions here
--
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: [I] Avoid panics? [arrow-rs]
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on issue #6737:
URL: https://github.com/apache/arrow-rs/issues/6737#issuecomment-2551572811
> If a state should be unreachable regardless how user input looks like (in which case reaching this state probably indicates a code bug or internal error), then panic is definitely preferred. If a state can be reachable upon invalid/corrupted user inputs, then an error result may be better
Yes, with a caveat. If a state is reachable upon invalid / corrupted user input then that does not mean the panic should be removed. It is more likely that validation needs to be inserted into the pipeline earlier.
For example, let's say I have some invalid parquet input. Maybe a byte array length is larger than the page length. Maybe this causes us to not read all the items in the array and then we generate some kind of panic like `arr_len != expected_len`. We shouldn't simply change that assert into a `Result`. The site of the panic may be distant from the spot where validation should have happened. In other words, if we reach an invalid or unreachable system state, we should identify the part of the input that was incorrect that led to this state, and then insert validation routines to detect that invalid input and report it very clearly (as a result at this point).
--
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: [I] Avoid panics? [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6737:
URL: https://github.com/apache/arrow-rs/issues/6737#issuecomment-2583764172
FYI @jp0317 has created a PR with proposed wording:
- https://github.com/apache/arrow-rs/pull/6958
--
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: [I] Avoid panics? [arrow-rs]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on issue #6737:
URL: https://github.com/apache/arrow-rs/issues/6737#issuecomment-2549670288
> Yes I think having clear guidelines on when to panic (basically an uncatchable panic) vs return errors would be helpful
Would this be something to discuss on the mailing list (or in this issue) or something else? I understand this can be contentious and I don't have enough knowledge to actually say what is right for the project but would like to see it move forward so happy to help out where I can.
> If the only errors that can be returned are due to bugs in code (aka internals errors) it is less clear to me an error is appropriate in that case
My perspective (which is admittedly not rust centric) comes from operating multi-tenant services. In this case the only cases that we wouldn't want to explicitly catch errors at a higher level is memory corruption (I think there is some gray area for out-of-bounds references). I've seen multiple occurrences of the equivalent in panic in C++, causing wide spread outages due to bugs in the assertion (i.e. the developer did not reason correctly about valid vs invalid state). Using a stack-unwinding panic handler is one way to mitigate but they come with there own issues (e.g. mixed FFI/Rust code).
--
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: [I] Avoid panics? [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6737:
URL: https://github.com/apache/arrow-rs/issues/6737#issuecomment-2615470322
`label_issue.py` automatically added labels {'documentation'} from #6958
--
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: [I] Avoid panics? [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on issue #6737:
URL: https://github.com/apache/arrow-rs/issues/6737#issuecomment-2549690061
https://doc.rust-lang.org/book/ch09-03-to-panic-or-not-to-panic.html#guidelines-for-error-handling
I think is relevant here, panics are a perfectly valid way to handle unexpected or illegal system states, especially when this would have safety implications.
Errors should be used where the situation is expected, e.g. due to user input.
I would be pretty lukewarm on a load of PRs randomly replacing assertions with errors, this just makes code harder to read and blurs what situations are expected.
However, PRs that change panics reachable from public APIs into errors are welcome
--
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: [I] Avoid panics? [arrow-rs]
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on issue #6737:
URL: https://github.com/apache/arrow-rs/issues/6737#issuecomment-2551526904
Thanks for all the comments! IIUC it seems we have a consensus like this? `If a state should be unreachable regardless how user input looks like (in which case reaching this state probably indicates a code bug or internal error), then panic is definitely preferred. If a state can be reachable upon invalid/corrupted user inputs, then an error result may be better`
--
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: [I] Avoid panics? [arrow-rs]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on issue #6737:
URL: https://github.com/apache/arrow-rs/issues/6737#issuecomment-2549422290
@alamb @tustvold is this something that has been discussed more widely within the community? having parameters on when panics are allowed would help avoid future friction on contributions we are looking to make.
--
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: [I] Avoid panics? [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #6737:
URL: https://github.com/apache/arrow-rs/issues/6737#issuecomment-2561116186
BTW @pitrou made a good point on the mailing list I will copy here:
> I'm not a Rust user, but I would expect invalid input files to return
> regular errors, not panic. Unlike API usage errors, invalid input files
> are not a bug in the calling code.
> This is also much nicer for bindings in high-level languages such as
Python.
--
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: [I] Avoid panics? [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold closed issue #6737: Avoid panics?
URL: https://github.com/apache/arrow-rs/issues/6737
--
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: [I] Avoid panics? [arrow-rs]
Posted by "jp0317 (via GitHub)" <gi...@apache.org>.
jp0317 commented on issue #6737:
URL: https://github.com/apache/arrow-rs/issues/6737#issuecomment-2551580928
> If a state is reachable upon invalid / corrupted user input then that does not mean the panic should be converted to an error. It is just as likely that validation needs to be inserted into the pipeline earlier
Thanks for calling this out. This is better and clearer.
--
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