Copilot commented on code in PR #50154:
URL: https://github.com/apache/arrow/pull/50154#discussion_r3391595937
##########
cpp/src/parquet/arrow/fuzz_encoding_internal.cc:
##########
@@ -155,6 +155,14 @@ namespace {
// Just to use std::vector<T> while avoiding std::vector<bool>
using BooleanSlot = std::array<uint8_t, sizeof(bool)>;
+// OOM during fuzzing is an expected soft failure; any other non-OK status
+// indicates a real bug and should abort so OSS-Fuzz can report it.
+Status FuzzCheckOk(const Status& st) {
+ if (st.IsOutOfMemory()) return st;
+ ARROW_CHECK_OK(st);
+ return Status::OK();
+}
Review Comment:
`FuzzCheckOk` doesn’t convey the key behavior (OOM is allowed to propagate;
all other errors abort). Consider renaming to something that encodes that
policy (e.g., `FuzzAbortOnNonOOM` / `AbortOnNonOOMStatus`) so future call sites
don’t misuse it.
##########
cpp/src/parquet/arrow/fuzz_encoding_internal.cc:
##########
@@ -155,6 +155,14 @@ namespace {
// Just to use std::vector<T> while avoiding std::vector<bool>
using BooleanSlot = std::array<uint8_t, sizeof(bool)>;
+// OOM during fuzzing is an expected soft failure; any other non-OK status
+// indicates a real bug and should abort so OSS-Fuzz can report it.
+Status FuzzCheckOk(const Status& st) {
+ if (st.IsOutOfMemory()) return st;
+ ARROW_CHECK_OK(st);
+ return Status::OK();
+}
Review Comment:
After `ARROW_CHECK_OK(st)`, returning `Status::OK()` works but is a bit
non-obvious since it discards the original (OK) `st`. Returning `st` instead
keeps the function’s behavior more transparent and makes it clearer the helper
is a policy gate rather than a status mapper.
##########
cpp/src/parquet/arrow/fuzz_encoding_internal.cc:
##########
@@ -300,13 +308,15 @@ struct TypedFuzzEncoding {
encoder->Put(*reference_array_);
auto reencoded_buffer = encoder->FlushValues();
auto reencoded_data = reencoded_buffer->template span_as<uint8_t>();
- auto array = DecodeArrow(roundtrip_encoding_,
reencoded_data).ValueOrDie();
- ARROW_CHECK_OK(array->ValidateFull());
- ARROW_CHECK_OK(CompareAgainstReference(array));
+ auto array_result = DecodeArrow(roundtrip_encoding_, reencoded_data);
+ RETURN_NOT_OK(FuzzCheckOk(array_result.status()));
+ auto array = std::move(*array_result);
+ RETURN_NOT_OK(FuzzCheckOk(array->ValidateFull()));
Review Comment:
This pattern (call returning `Result<T>`, separately check `status()`, then
dereference/move) is easy to get wrong in future edits (e.g., accidentally
dereferencing without the check). Consider adding a small helper that accepts a
`Result<T>` and either returns the value or propagates OOM / aborts on other
errors, so call sites become a single expression and reduce the chance of
misuse.
--
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]
