Use exceptions instead of return values to handle errors in CompactProtocolReader
#14582
Conversation
…impr-no-bool-return-error
vuule
added
cuIO
…impr-no-bool-return-error
| bool const exit_function = FunctionSwitchImpl<index>::run(cpr, field_type, field, op); | ||
| if (exit_function) { return false; } |
There was a problem hiding this comment.
This is where the logic inverts and false signals failure. This logic is(was) used from here to the top-level read functions.
…impr-no-bool-return-error
…cudf into impr-no-bool-return-error
…impr-no-bool-return-error
…cudf into impr-no-bool-return-error
|
|
||
| void assert_field_type(int type, FieldType expected) | ||
| { | ||
| CUDF_EXPECTS(type == expected, |
There was a problem hiding this comment.
Oh no, this is comparing int vs enum.
There was a problem hiding this comment.
We always compared ints and FieldType in this code, e.g. if (field_type != ST_FLD_LIST) { return true; }. I think it's fine, given that we throw if the values are not equal - no int values that are not valid FieldType values will get past this check. We have to sanitize the input somewhere 🤷♂️
There was a problem hiding this comment.
We definitely need to refactor it, changing into enum class for FieldType.
There was a problem hiding this comment.
I'm not following. We have to compare the int input with a FieldType value somewhere. Using an enum class helps with the scope of the value names, but we still need the int <-> enum comparison, just with a static_cast.
There was a problem hiding this comment.
Then we will do an explicit static_cast before comparison. I think it is better than this implicit comparison.
There was a problem hiding this comment.
Just checked, FieldType is not exactly part of the spec, we have some creative freedom around it.
If you don't mind, let's make it an enum class in a separate PR. It's a wider change than the error handling this PR is about.
| inline void operator()(CompactProtocolReader* cpr, int field_type) | ||
| { | ||
| if (field_type != ST_FLD_LIST) { return true; } | ||
| assert_field_type(field_type, ST_FLD_LIST); |
There was a problem hiding this comment.
Probably we are wrong from the beginning. Why not field_type an enum value, instead of int?
There was a problem hiding this comment.
In addition, should the caller handle the input type instead of checking it here?
There was a problem hiding this comment.
field_type is obtained by the caller by parsing the encoded input, so it could be any value if the input file is garbage. We cannot assume it will be a valid FieldType.
As for the validation on the caller side - the caller only sees these parquet_field_xyz objects as functors and does not know the actual field type that is being read. We could move validation to the caller if parquet_field classes somehow expose the expected field type, but I don't think this is a better option.
|
/merge |
Description
Align the coding style with the rest of libcudf and use exceptions to report errors instead of returning a bool.
This simplifies the control flow in many functions. In addition, the return value used top be ignored at many call sites.
Checklist