-
Notifications
You must be signed in to change notification settings - Fork 174
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
[FEAT] Add getter for Struct and List expressions #1775
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1775 +/- ##
==========================================
+ Coverage 84.60% 84.88% +0.28%
==========================================
Files 55 55
Lines 5554 5604 +50
==========================================
+ Hits 4699 4757 +58
+ Misses 855 847 -8
|
idx_iter: &mut impl Iterator<Item = Option<i64>>, | ||
default: &Series, | ||
) -> DaftResult<Series> { | ||
let default = default.cast(self.child_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.
default
isn't a reserved keyword in Rust but the syntax highlighting is different on it. Maybe because it's in std.
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.
Looking good so far!
src/daft-core/src/array/ops/list.rs
Outdated
); | ||
|
||
// set all default indices to 0 if there is only one | ||
let multiple_defaults = match default.len() { |
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 we can probably make an assertion that default
has length 1 and only allow for that case from the user-API.
Should be pretty rare that a user wants to have a different default per-element!
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.
In Python-land you can wrap the default into a LiteralExpression
like so:
def get(idx: int, default: Any):
default_expr = lit(default)
...
Then when it gets evaluated in Rust it should evaluate the a Series with length 1 I believe (we should double-check!)
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.
Thanks! This method does seem to work, although it doesn't seem to emit any errors when you pass in an Expression, it just manages it as a generic PyObject.
I think this is probably better handled on the Rust side, but is there a way to check of an expression's type is castable to another's?
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.
Looking good :)
Sorry that this PR is getting a little larger than expected now, most of the changed files are just adding implementations for |
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.
Great work!
Let's also add these expressions into our documentation: https://github.com/Eventual-Inc/Daft/blob/main/docs/source/api_docs/expressions.rst#nested
This PR adds getters for both lists and structs, which enables the access of elements in
ListArray
,FixedSizeListArray
, andStructArray
types.For lists:
This method takes in an index, which can be a single value or an expression, and optionally a default value, which will be set when the index is out of bounds. It retrieves the value at index of each list in the array.
For structs:
This method takes in the name of the field to get, and returns an expression of that struct's field.