jorgecarleitao commented on a change in pull request #7796:
URL: https://github.com/apache/arrow/pull/7796#discussion_r459002736
##########
File path: rust/datafusion/src/execution/physical_plan/expressions.rs
##########
@@ -47,81 +47,43 @@ use arrow::compute::kernels::sort::{SortColumn,
SortOptions};
use arrow::datatypes::{DataType, Schema, TimeUnit};
use arrow::record_batch::RecordBatch;
-/// Represents an aliased expression
Review comment:
I assume that you refer to the physical - not logical - plan in your
comment, as this file is about the physical plan. The logical plan does have an
`Alias` expression in `logicalplan.rs`.
IMO this is the second core idea of this PR: we do not need aliases as
physical expressions at all. The proof for me is
1. this PR removes `fn name()` from the trait `PhysicalExpr` and
2. the implementation of `Alias` is: for all method `X(...)` other than
`X=name`, `return self.expr.X(...)`
i.e. without `name()`, `Alias` is indistinguishable from its `self.expr`
(apart from using extra memory for `self.name`) and thus serves no purpose.
Note that we still have that name in the schema of the physical
`projection`, so we can still back-propagate a column name to resolve a
column's expression at a given projection (e.g. to optimize projections or
perform filter push down). So, all the information is still available at the
physical plan.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
