ozankabak commented on code in PR #16217:
URL: https://github.com/apache/datafusion/pull/16217#discussion_r2126810121
##########
datafusion/physical-expr-common/src/sort_expr.rs:
##########
@@ -516,162 +460,240 @@ impl Display for LexOrdering {
}
}
-impl FromIterator<PhysicalSortExpr> for LexOrdering {
- fn from_iter<T: IntoIterator<Item = PhysicalSortExpr>>(iter: T) -> Self {
- let mut lex_ordering = LexOrdering::default();
-
- for i in iter {
- lex_ordering.push(i);
- }
+impl IntoIterator for LexOrdering {
+ type Item = PhysicalSortExpr;
+ type IntoIter = IntoIter<Self::Item>;
- lex_ordering
+ fn into_iter(self) -> Self::IntoIter {
+ self.exprs.into_iter()
}
}
-impl Index<usize> for LexOrdering {
- type Output = PhysicalSortExpr;
+impl<'a> IntoIterator for &'a LexOrdering {
+ type Item = &'a PhysicalSortExpr;
+ type IntoIter = std::slice::Iter<'a, PhysicalSortExpr>;
- fn index(&self, index: usize) -> &Self::Output {
- &self.inner[index]
+ fn into_iter(self) -> Self::IntoIter {
+ self.exprs.iter()
}
}
-impl Index<Range<usize>> for LexOrdering {
- type Output = [PhysicalSortExpr];
-
- fn index(&self, range: Range<usize>) -> &Self::Output {
- &self.inner[range]
+impl From<LexOrdering> for Vec<PhysicalSortExpr> {
+ fn from(ordering: LexOrdering) -> Self {
+ ordering.exprs
}
}
-impl Index<RangeFrom<usize>> for LexOrdering {
- type Output = [PhysicalSortExpr];
+/// This object represents a lexicographical ordering requirement and contains
+/// a vector of `PhysicalSortRequirement` objects.
+///
+/// For example, a `vec![a Some(ASC), b None]` represents a lexicographical
+/// requirement that firsts imposes an ordering by column `a` in ascending
+/// order, then by column `b` in *any* (ascending or descending) order. The
+/// ordering is non-degenerate, meaning it contains at least one element, and
+/// it is duplicate-free, meaning it does not contain multiple entries for the
+/// same column.
+///
+/// Note that a `LexRequirement` need not enforce the uniqueness of its sort
+/// expressions after construction like a `LexOrdering` does, because it
provides
+/// no mutation methods. If such methods become necessary, we will need to
+/// enforce uniqueness like the latter object.
+#[derive(Debug, Clone, PartialEq)]
+pub struct LexRequirement {
+ reqs: Vec<PhysicalSortRequirement>,
+}
+
+impl LexRequirement {
+ /// Creates a new [`LexRequirement`] from the given vector of sort
expressions.
+ /// If the vector is empty, returns `None`.
+ pub fn new(reqs: impl IntoIterator<Item = PhysicalSortRequirement>) ->
Option<Self> {
Review Comment:
I initially thought so too, but seems like Rust's convention is to use
`try_new` when returning a `Result`, and simply `new` when returning an
`Option`. Here is an example from the standard library:
https://doc.rust-lang.org/std/num/type.NonZeroU32.html
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
