alamb commented on code in PR #27238:
URL: https://github.com/apache/datafusion/pull/27238#discussion_r4506507690
##########
datafusion/physical-expr/src/equivalence/class.rs:
##########
@@ -24,16 +24,19 @@ use datafusion_physical_expr_common::physical_expr::{
use indexmap::{IndexMap, IndexSet};
-/// Represents whether a constant expression's value is uniform or varies
across
-/// partitions. Has two variants:
-/// - `Heterogeneous`: The constant expression may have different values for
Review Comment:
I think it is more common to document the field names at their definition
site rather than in the module level so I moved them there
##########
datafusion/physical-expr/src/equivalence/class.rs:
##########
@@ -24,16 +24,19 @@ use datafusion_physical_expr_common::physical_expr::{
use indexmap::{IndexMap, IndexSet};
-/// Represents whether a constant expression's value is uniform or varies
across
-/// partitions. Has two variants:
-/// - `Heterogeneous`: The constant expression may have different values for
-/// different partitions.
-/// - `Uniform(Option<ScalarValue>)`: The constant expression has the same
value
-/// across all partitions, or is `None` if the value is unknown.
+/// How a constant expression's value varies across partitions.
Review Comment:
I also tried to make this more concise so it is easier to undersand
##########
datafusion/physical-expr/src/equivalence/class.rs:
##########
@@ -52,20 +55,11 @@ impl Display for AcrossPartitions {
}
}
-/// A structure representing a expression known to be constant in a physical
-/// execution plan.
-///
-/// The `ConstExpr` struct encapsulates an expression that is constant during
-/// the execution of a query. For example if a filter like `A = 5` appears
-/// earlier in the plan, `A` would become a constant in subsequent operations.
-///
-/// # Fields
+/// An expression known to be constant during the execution of a query.
Review Comment:
similarly I made this more concise and removed the redundant field level
documentation
##########
datafusion/physical-expr/src/equivalence/class.rs:
##########
@@ -300,8 +300,12 @@ type AugmentedMapping<'a> = IndexMap<
(&'a ProjectionTargets, Option<&'a EquivalenceClass>),
>;
-/// A collection of distinct `EquivalenceClass`es. This object supports fast
-/// lookups of expressions and their equivalence classes.
+/// A set of distinct [`EquivalenceClass`]es.
+///
+/// `EquivalenceGroup` supports fast lookups of expressions and their
+/// equivalence classes. See [`EquivalenceProperties`] for more details.
+///
+/// [`EquivalenceProperties`]: crate::equivalence::EquivalenceProperties
Review Comment:
I am trying to route all docs back to EquivalenceProperties where we can
centralize as much as possible
##########
datafusion/physical-expr/src/equivalence/ordering.rs:
##########
@@ -30,16 +31,25 @@ use
datafusion_physical_expr_common::sort_expr::PhysicalSortExpr;
/// ```
///
/// Here, both `[a ASC, b ASC]` and `[c DESC, d ASC]` describe the table
-/// ordering. In this case, we say that these orderings are equivalent.
+/// ordering, so these orderings are equivalent. See [Equivalent sort
+/// expressions] for more details.
+///
+/// An `OrderingEquivalenceClass` is a set of such equivalent orderings.
+///
+/// [`GroupingEquivalenceClass`] is used when groups of equal values are
+/// contiguous, but not in order.
///
-/// An `OrderingEquivalenceClass` is a set of such equivalent orderings, which
Review Comment:
this describes how the data is represented and invariants (which is an
implementation detail) -- moved to the field definition
##########
datafusion/physical-expr/src/equivalence/ordering.rs:
##########
@@ -216,18 +226,11 @@ impl OrderingEquivalenceClass {
/// `ASC NULLS LAST` and `DESC NULLS FIRST`. Rows sorted both ways all have
/// the same value (or are all null).
///
- /// This is how a value that is constant only within groups of rows is
- /// recorded, since a `ConstExpr` is constant across all rows. For example,
- /// `SUM(x) OVER (PARTITION BY k)` has one value per `k`, so the window
adds
- /// `[k, w]` once for each of the four directions of `w`.
- ///
- /// ### Future Improvements
+ /// This is how a whole-partition window aggregate records that its value
Review Comment:
I found the existing documentation quite hard to understand about
whole-partition window aggregates, etc. more coming soon
--
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]
