askalt commented on code in PR #19893:
URL: https://github.com/apache/datafusion/pull/19893#discussion_r2729177299
##########
datafusion/physical-expr/src/projection.rs:
##########
@@ -125,7 +125,7 @@ impl From<ProjectionExpr> for (Arc<dyn PhysicalExpr>,
String) {
/// indices.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct ProjectionExprs {
- exprs: Vec<ProjectionExpr>,
+ exprs: Arc<[ProjectionExpr]>,
Review Comment:
Done.
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -544,11 +544,11 @@ pub struct AggregateExec {
/// Aggregation mode (full, partial)
mode: AggregateMode,
/// Group by expressions
- group_by: PhysicalGroupBy,
+ group_by: Arc<PhysicalGroupBy>,
Review Comment:
Done.
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -498,6 +506,146 @@ impl RecordBatchStream for ProjectionStream {
}
}
+/// Describes an option immutable reference counted shared projection.
Review Comment:
> 1. Move this into datafusion/physical-expr/src/projection.rs (not in
physical plan) so it is near ProjectionExprs
Done
> I also recommend documenting why this structure exists (because it is
cheap to clone)
Done
> 2. Move the Option outside (so the signature is Option<ProjectionRef>
rather than OptionProjectionRef)
Initially I done in this way, but in this case, take a look at possible
`HashJoinExec::try_new` signature. It seems we want something like
```rust
fn try_new(
...
projection: Option<impl Into<ProjectionRef>>,
...
)
```
To be able to pass `Some(vec![1,2,3])` and `Some(projection_ref)` as well.
But `Option<impl ...>` forces us to explicitly specify `None` type, like
`None::<ProjectionRef>` which looks annoying for me. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
