-
Notifications
You must be signed in to change notification settings - Fork 130
feat[vortex-array]: expr array that represents lazy computation #5400
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
Conversation
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
59b7966 to
c13a536
Compare
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
CodSpeed Performance ReportMerging #5400 will improve performances by 11.24%Comparing Summary
Benchmarks breakdown
Footnotes
|
|
@claude review pelase |
| // Check if the parent is an ExprArray wrapping this struct | ||
| // If so, we can partition the expression over the struct fields | ||
| if let Some(expr_array) = parent.as_opt::<ExprVTable>() { | ||
| let session = ExprSession::default(); |
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 have a plan of how to inject this in a future PR using the expr session context
| /// The caller must ensure that `dtype` matches `expr.return_dtype(child.dtype())`. | ||
| /// Violating this invariant may lead to incorrect results or panics when the array is used. | ||
| pub unsafe fn unchecked_new(child: ArrayRef, expr: Expression, dtype: DType) -> Self { | ||
| Self { |
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 quite like the pattern we have started to use in vortex-vector where we have new that calls try_new().expect(), and new_unchecked (note not unchecked_new) has if cfg(debug_assertions) { try_new().expect() } else { Self { ... }}
| impl OperatorVTable<ExprVTable> for ExprVTable { | ||
| fn reduce(array: &ExprArray) -> VortexResult<Option<ArrayRef>> { | ||
| // Get the default expression session | ||
| let session = ExprSession::default(); |
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 leave a louder fixme / todo that we have to inject this
Added an ExprArray and a few reduce rules ---- Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Added an ExprArray and a few reduce rules ---- Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Added an ExprArray and a few reduce rules