You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "jatin510 (via GitHub)" <gi...@apache.org> on 2024/11/02 20:17:40 UTC
[PR] converted LexOrderingRef to &LexOrdering [datafusion]
jatin510 opened a new pull request, #13233:
URL: https://github.com/apache/datafusion/pull/13233
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes https://github.com/apache/datafusion/issues/13220.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] converted LexOrderingRef to &LexOrdering [datafusion]
Posted by "jatin510 (via GitHub)" <gi...@apache.org>.
jatin510 commented on PR #13233:
URL: https://github.com/apache/datafusion/pull/13233#issuecomment-2453116676
@alamb am i going in right directions ?
as this PR changes lots of files ? 😅
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Deprecate `LexOrderingRef` and `LexRequirementRef` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13233:
URL: https://github.com/apache/datafusion/pull/13233#issuecomment-2455539067
FYI @berkaysynnada
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Deprecate `LexOrderingRef` and `LexRequirementRef` [datafusion]
Posted by "jatin510 (via GitHub)" <gi...@apache.org>.
jatin510 commented on PR #13233:
URL: https://github.com/apache/datafusion/pull/13233#issuecomment-2453433695
Thanks !
made the suggested changes.
We can come up with a follow-up PR for more refinements!
@alamb
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Deprecate `LexOrderingRef` and `LexRequirementRef` [datafusion]
Posted by "jatin510 (via GitHub)" <gi...@apache.org>.
jatin510 commented on code in PR #13233:
URL: https://github.com/apache/datafusion/pull/13233#discussion_r1826985958
##########
datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs:
##########
@@ -1553,7 +1552,7 @@ mod tests {
Arc::new(BuiltInWindowExpr::new(
last_value_func,
&[],
- LexOrderingRef::default(),
+ LexOrdering::default().as_ref(),
Review Comment:
using
`&LexOrdering::default()`
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Deprecate `LexOrderingRef` and `LexRequirementRef` [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #13233:
URL: https://github.com/apache/datafusion/pull/13233#issuecomment-2456456787
I have merged with main to resolve conflicts. I will merge this once the tests pass
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Deprecate `LexOrderingRef` and `LexRequirementRef` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13233:
URL: https://github.com/apache/datafusion/pull/13233#discussion_r1826957395
##########
datafusion/physical-expr-common/src/sort_expr.rs:
##########
@@ -352,20 +346,26 @@ pub struct LexOrdering {
pub inner: Vec<PhysicalSortExpr>,
}
+impl AsRef<LexOrdering> for LexOrdering {
+ fn as_ref(&self) -> &LexOrdering {
+ self
+ }
+}
+
impl LexOrdering {
// Creates a new [`LexOrdering`] from a vector
pub fn new(inner: Vec<PhysicalSortExpr>) -> Self {
Self { inner }
}
- pub fn as_ref(&self) -> LexOrderingRef {
- &self.inner
- }
-
pub fn capacity(&self) -> usize {
self.inner.capacity()
}
+ pub fn from_ref(lex_ordering_ref: &LexOrdering) -> Self {
Review Comment:
I think this method could be removed as it is the same as `clone()`
##########
datafusion/physical-expr-common/src/sort_expr.rs:
##########
@@ -545,6 +596,16 @@ impl IntoIterator for LexRequirement {
}
}
+impl<'a> IntoIterator for &'a LexOrdering {
Review Comment:
👍
##########
datafusion/core/src/physical_optimizer/enforce_sorting.rs:
##########
@@ -392,7 +392,11 @@ fn analyze_immediate_sort_removal(
let sort_input = sort_exec.input();
// If this sort is unnecessary, we should remove it:
if sort_input.equivalence_properties().ordering_satisfy(
- sort_exec.properties().output_ordering().unwrap_or_default(),
+ &sort_exec
+ .properties()
+ .output_ordering()
+ .cloned()
+ .unwrap_or_default(),
Review Comment:
I think if we had `LexOrdering::empty()` as explained below, we could avoid this clone like this
```rust
&sort_exec
.properties()
.output_ordering()
.unwrap_or(LexOrdering::empty()),
```
There are similar things below too
##########
datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs:
##########
@@ -1553,7 +1552,7 @@ mod tests {
Arc::new(BuiltInWindowExpr::new(
last_value_func,
&[],
- LexOrderingRef::default(),
+ LexOrdering::default().as_ref(),
Review Comment:
I don't think the `as_ref()` is needed here (it just makes another copy)
##########
benchmarks/src/sort.rs:
##########
@@ -70,79 +70,88 @@ impl RunOpt {
let sort_cases = vec![
(
"sort utf8",
- vec![PhysicalSortExpr {
- expr: col("request_method", &schema)?,
- options: Default::default(),
- }],
+ LexOrdering {
Review Comment:
Maybe this could be `LexOrdering::new`
##########
datafusion/physical-expr-common/src/sort_expr.rs:
##########
@@ -409,6 +405,36 @@ impl LexOrdering {
pub fn truncate(&mut self, len: usize) {
self.inner.truncate(len)
}
+
+ /// Merge the contents of `other` into `self`, removing duplicates.
+ pub fn merge(mut self, other: LexOrdering) -> Self {
+ self.inner = self.inner.into_iter().chain(other).unique().collect();
+ self
+ }
+
+ /// Converts a `LexRequirement` into a `LexOrdering`.
Review Comment:
I also included this change in https://github.com/apache/datafusion/pull/13222 (mostly as a note that this will be a logical conflict)
##########
datafusion/physical-plan/src/sorts/streaming_merge.rs:
##########
@@ -51,18 +52,34 @@ macro_rules! merge_helper {
}};
}
-#[derive(Default)]
pub struct StreamingMergeBuilder<'a> {
streams: Vec<SendableRecordBatchStream>,
schema: Option<SchemaRef>,
- expressions: LexOrderingRef<'a>,
+ expressions: &'a LexOrdering,
metrics: Option<BaselineMetrics>,
batch_size: Option<usize>,
fetch: Option<usize>,
reservation: Option<MemoryReservation>,
enable_round_robin_tie_breaker: bool,
}
+static EMPTY_ORDER: OnceLock<LexOrdering> = OnceLock::new();
+
+impl<'a> Default for StreamingMergeBuilder<'a> {
+ fn default() -> Self {
+ Self {
+ streams: vec![],
+ schema: None,
+ expressions: EMPTY_ORDER.get_or_init(LexOrdering::default),
Review Comment:
👍
What do you think about moving this `EMPTY_ORDER` to `LexOrdering`?
Something like
```rust
impl LexOrdering {
...
/// Return an empty LexOrdering (no expressions)
pub empty() -> &'static LexOrdering {
EMPTY_ORDER.get_or_init(LexOrdering::default),
}
}
```
That would avoid needing to copy LexOrderings quite as much (I'll comment inline
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Deprecate `LexOrderingRef` and `LexRequirementRef` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13233:
URL: https://github.com/apache/datafusion/pull/13233#issuecomment-2457561683
Thanks again @berkaysynnada and @jatin510
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Deprecate `LexOrderingRef` and `LexRequirementRef` [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada merged PR #13233:
URL: https://github.com/apache/datafusion/pull/13233
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org