-
Notifications
You must be signed in to change notification settings - Fork 58
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
Fix some issues with arrow expression eval #401
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #401 +/- ##
==========================================
- Coverage 78.34% 78.25% -0.09%
==========================================
Files 49 49
Lines 10282 10256 -26
Branches 10282 10256 -26
==========================================
- Hits 8055 8026 -29
- Misses 1775 1780 +5
+ Partials 452 450 -2 ☔ View full report in Codecov by Sentry. |
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.
LGTM just a couple nits for comment/follow-up
} else { | ||
// Last path step. Return it. | ||
Ok(child) | ||
fn extract_column<'a>( |
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.
maybe a super quick example would also be useful? e.g. parent is complex struct column a
and field names are [b, c]
means we return a ref to the nested column c
? am I understanding correctly?
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.
Exactly. Added the missing doc comment.
.cloned() | ||
} | ||
} | ||
(Column(name), _) => extract_column(batch, name.split('.')), |
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.
do we still need to have a follow-up in order to properly handle nested columns (instead of just naive split('.')
?)
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.
Definitely. My WIP nested column code already handles it.
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.
Ah, you refer to the deleted TODO comment. Reinstating it.
Three things: