You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/07/13 14:22:04 UTC
[GitHub] [arrow] amol- opened a new pull request #10713: ARROW-12837: [C++] Do not crash when printing invalid arrays
amol- opened a new pull request #10713:
URL: https://github.com/apache/arrow/pull/10713
--
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
[GitHub] [arrow] amol- edited a comment on pull request #10713: ARROW-12837: [C++] Do not crash when printing invalid arrays
Posted by GitBox <gi...@apache.org>.
amol- edited a comment on pull request #10713:
URL: https://github.com/apache/arrow/pull/10713#issuecomment-885548327
> As I said, why not. But any work trying to enhance the validity check here will end up reproducing what's already done by `Validate` and `ValidateFull`.
Thanks for pointing out `Validate`. I tried to replace the usage of my custom method with a call to `Validate` and it passes my test, so it's already verifying for the condition I was trying to catch. Locally I had 9 failures when running all tests but were unrelated ( most of them about `PARQUET_TEST_DATA` not being set), so I hope that we can just reuse validate without any side effect.
PS: If the CI confirms that I haven't broke anything else, I plan to also try extending the `<invalid array>` message with something that reports the `Status::Invalid`
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #10713: ARROW-12837: [C++] Do not crash when printing invalid arrays
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10713:
URL: https://github.com/apache/arrow/pull/10713#issuecomment-879134831
https://issues.apache.org/jira/browse/ARROW-12837
--
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
[GitHub] [arrow] pitrou commented on pull request #10713: ARROW-12837: [C++] Do not crash when printing invalid arrays
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10713:
URL: https://github.com/apache/arrow/pull/10713#issuecomment-884290417
I'm not opposed to this a priori, but I would find it better if the test suite avoided creating invalid arrays (except where specifically desired, e.g. to test validation).
--
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
[GitHub] [arrow] bkietz commented on a change in pull request #10713: ARROW-12837: [C++] Do not crash when printing invalid arrays
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10713:
URL: https://github.com/apache/arrow/pull/10713#discussion_r676738066
##########
File path: cpp/src/arrow/pretty_print.cc
##########
@@ -325,6 +325,12 @@ class ArrayPrinter : public PrettyPrinter {
std::is_base_of<FixedSizeListArray, T>::value,
Status>
Visit(const T& array) {
+ Status validArrayState = array.Validate();
+ if (!validArrayState.ok()) {
+ (*sink_) << "<InvalidArray: " << validArrayState.message() << ">";
Review comment:
Nit: this doesn't follow our naming convention for local variables
```suggestion
Status st = array.Validate();
if (!st.ok()) {
(*sink_) << "<InvalidArray: " << st.message() << ">";
```
--
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
[GitHub] [arrow] amol- commented on pull request #10713: ARROW-12837: [C++] Do not crash when printing invalid arrays
Posted by GitBox <gi...@apache.org>.
amol- commented on pull request #10713:
URL: https://github.com/apache/arrow/pull/10713#issuecomment-886578474
@pitrou I think this is ready for a second review, what do you think?
--
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
[GitHub] [arrow] amol- commented on pull request #10713: ARROW-12837: [C++] Do not crash when printing invalid arrays
Posted by GitBox <gi...@apache.org>.
amol- commented on pull request #10713:
URL: https://github.com/apache/arrow/pull/10713#issuecomment-884295728
> I'm not opposed to this a priori, but I would find it better if the test suite avoided creating invalid arrays (except where specifically desired, e.g. to test validation).
I wasn't much concerned about the test suite, which I agree can create valid arrays (creating null arrays smells like a form of premature optimization). I think that my focus was more toward C++ users ending up with a `nullptr` returned from somewhere else. In such case it makes your life a little easier if you just get printed out that the array is invalid instead of having to attach `gdb` to understand why you are facing a crash
--
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
[GitHub] [arrow] bkietz closed pull request #10713: ARROW-12837: [C++] Do not crash when printing invalid arrays
Posted by GitBox <gi...@apache.org>.
bkietz closed pull request #10713:
URL: https://github.com/apache/arrow/pull/10713
--
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
[GitHub] [arrow] amol- commented on a change in pull request #10713: ARROW-12837: [C++] Do not crash when printing invalid arrays
Posted by GitBox <gi...@apache.org>.
amol- commented on a change in pull request #10713:
URL: https://github.com/apache/arrow/pull/10713#discussion_r677336324
##########
File path: cpp/src/arrow/pretty_print.cc
##########
@@ -325,6 +325,12 @@ class ArrayPrinter : public PrettyPrinter {
std::is_base_of<FixedSizeListArray, T>::value,
Status>
Visit(const T& array) {
+ Status validArrayState = array.Validate();
+ if (!validArrayState.ok()) {
+ (*sink_) << "<InvalidArray: " << validArrayState.message() << ">";
Review comment:
Applied your suggested change! I should have used underscores,
Even thought, a bit more talkative variable name would have helped ;)
Even if I get that `st` is a `Status`, it doesn't tell me much about what status it is and I have to navigate to the line of code that returned it to be able to guess, but in this case it's a 3 lines of code block, so I guess that having to look at the top of it to get more context probably doesn't matter too much.
--
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
[GitHub] [arrow] amol- commented on pull request #10713: ARROW-12837: [C++] Do not crash when printing invalid arrays
Posted by GitBox <gi...@apache.org>.
amol- commented on pull request #10713:
URL: https://github.com/apache/arrow/pull/10713#issuecomment-885548327
> As I said, why not. But any work trying to enhance the validity check here will end up reproducing what's already done by `Validate` and `ValidateFull`.
Thanks for pointing out `Validate`. I tried to replace the usage of my custom method with a call to `Validate` and it passes my test, so it's already verifying for the condition I was trying to catch. Locally I had 9 failures when running all tests but were unrelated ( most of them about `PARQUET_TEST_DATA` not being set), so I hope that we can just reuse validate without any side effect.
--
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
[GitHub] [arrow] pitrou commented on pull request #10713: ARROW-12837: [C++] Do not crash when printing invalid arrays
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10713:
URL: https://github.com/apache/arrow/pull/10713#issuecomment-884298688
As I said, why not. But any work trying to enhance the validity check here will end up reproducing what's already done by `Validate` and `ValidateFull`.
--
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
[GitHub] [arrow] bkietz commented on a change in pull request #10713: ARROW-12837: [C++] Do not crash when printing invalid arrays
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10713:
URL: https://github.com/apache/arrow/pull/10713#discussion_r676738066
##########
File path: cpp/src/arrow/pretty_print.cc
##########
@@ -325,6 +325,12 @@ class ArrayPrinter : public PrettyPrinter {
std::is_base_of<FixedSizeListArray, T>::value,
Status>
Visit(const T& array) {
+ Status validArrayState = array.Validate();
+ if (!validArrayState.ok()) {
+ (*sink_) << "<InvalidArray: " << validArrayState.message() << ">";
Review comment:
```suggestion
Status st = array.Validate();
if (!st.ok()) {
(*sink_) << "<InvalidArray: " << st.message() << ">";
```
--
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
[GitHub] [arrow] pitrou commented on pull request #10713: ARROW-12837: [C++] Do not crash when printing invalid arrays
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10713:
URL: https://github.com/apache/arrow/pull/10713#issuecomment-886678588
@bkietz Would you have an opinion on this?
--
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