alamb commented on code in PR #12457: URL: https://github.com/apache/datafusion/pull/12457#discussion_r1759251540
########## datafusion/physical-expr-common/src/physical_expr.rs: ########## @@ -31,8 +31,27 @@ use datafusion_expr_common::columnar_value::ColumnarValue; use datafusion_expr_common::interval_arithmetic::Interval; use datafusion_expr_common::sort_properties::ExprProperties; -/// See [create_physical_expr](https://docs.rs/datafusion/latest/datafusion/physical_expr/fn.create_physical_expr.html) -/// for examples of creating `PhysicalExpr` from `Expr` +/// [`PhysicalExpr`]s represent expressions such as `A + 1` or `CAST(c1 AS int)`. Review Comment: it was strange there was no explanation of what a `PhysicalExpr` was on the trait, so let's fix that (moved the docs from `create_physical_expr` and revamped it a bit) ########## datafusion/physical-expr/src/expressions/column.rs: ########## @@ -33,32 +33,67 @@ use datafusion_expr::ColumnarValue; use crate::physical_expr::{down_cast_any_ref, PhysicalExpr}; /// Represents the column at a given index in a RecordBatch +/// +/// This is a physical expression that represents a column at a given index in a Review Comment: The point that physical exprs are always in terms of indexes wasn't as clear as I think it could be so I tried to improve that -- 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]
