-
Notifications
You must be signed in to change notification settings - Fork 906
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
early type checks in RowConverter
#3080
Conversation
Decimal types are already handled by `downcast_primitive`.
Check supported row format types when creating the converter instead of during conversion. Also add an additional method `RowConverter::supports_fields` to check types w/o relying on an error. Closes apache#3077.
arrow/src/row/mod.rs
Outdated
/// Check if the given fields are supported by the row format. | ||
pub fn supports_fields(fields: &[SortField]) -> bool { | ||
for f in fields { | ||
let data_type = &f.data_type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be simplified to something like
match data_type {
DataType::Dictionary(_, v) => !DataType::is_nested(v.as_ref()),
_ => !DataType::is_nested(data_type),
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#3083 will make this even simpler
I took the liberty of simplifying this so it can make the arrow release |
Benchmark runs are scheduled for baseline = ed20bf1 and contender = 8d364fe. 8d364fe is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #3077.
Rationale for this change
RowConverter
API by implementing "if you can create it, you can also feed data into it (modulo overflows)".What changes are included in this PR?
RowConverter::supports_fields
to check if fields are supported by row formatRowConverter::new
can now failAre there any user-facing changes?
BREAKING CHANGE:
RowConverter::new
can now fail