rtpsw commented on PR #14347: URL: https://github.com/apache/arrow/pull/14347#issuecomment-1273349266
> No, although looking at it, the fix is a little weird - it means we're comparing data of two different types, or the data doesn't match its type? I run into the issue that led to this PR when a unit test compared data of two different types, one is the expected and another is the unexpected. My quick impression was that there seemed to be sufficiently many test cases that compare this way, without trying to compare types or to validate first. I figured it would be easier to fix the comparison in one place than to fix many test cases. > Right, since the data types are supposed to match, this PR is only guarding against invalid data. It you want to make sure data is valid, you should call Validate or ValidateFull before doing anything else. I may be missing something, but I don't think validating would have fixed the issue, because I believe in the above case both expected and actual data were valid, since they both were obtained via JSON parsing; they were just of different types. > Regardless, it seems something like this would suffice? ... `// assuming this segfaults without this PR` I didn't check your particular case yet, but I did run into a segfault in the case I described above. My vote would be to minimize segfaults as an indication of test failure, if only because such a failure would be less convenient to work with. -- 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]
