fix: reject malformed Arrow field IDs - #845
Conversation
| int32_t GetFieldId(const ArrowSchema& schema) { | ||
| Result<int32_t> GetFieldId(const ArrowSchema& schema) { | ||
| if (schema.metadata == nullptr) { | ||
| return kUnknownFieldId; |
There was a problem hiding this comment.
Should this be changed? It is not clear to me what is the difference between unknowFieldId (returning the constant) and InvalidFieldId (returning in a error)
There was a problem hiding this comment.
They are intentionally different. If the PARQUET:field_id metadata is missing, we keep the existing behavior and use the unknown field ID sentinel. If the key is present but the value is malformed or outside int32_t, we return an invalid schema error instead.
| field_id); | ||
| int32_t field_id = 0; | ||
| const auto* end = field_id_value.data + field_id_value.size_bytes; | ||
| const auto [ptr, ec] = std::from_chars(field_id_value.data, end, field_id); |
There was a problem hiding this comment.
What about using ParseNumber<int32_t> to simplify this?
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
Sorry for the late review. Thanks @fallintoplace and @evindj! |
3 participants
Summary
PARQUET:field_idmetadata instead of accepting a valid numeric prefix or returning the unknown-field sentinelTesting
arrow_test --gtest_filter=FromArrowSchemaTest.RejectMalformedFieldIdMetadata