Skip to content

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Apr 7, 2023

This is based on @ozankabak 's PR alamb#11

I tried to pull the commits from that branch to make this work on main

It currently failing to compile with

error[E0277]: the trait bound `PhysicalSortExpr: From<&PhysicalSortRequirement>` is not satisfied
   --> datafusion/core/src/physical_optimizer/sort_pushdown.rs:131:17
    |
131 |                 PhysicalSortRequirement::to_sort_exprs(parent_required.ok_or_else(err)?);
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `From<&PhysicalSortRequirement>` is not implemented for `PhysicalSortExpr`
    |
    = help: the trait `From<PhysicalSortRequirement>` is implemented for `PhysicalSortExpr`
note: required by a bound in `PhysicalSortRequirement::to_sort_exprs`

@github-actions github-actions bot added core Core DataFusion crate physical-expr Changes to the physical-expr crates labels Apr 7, 2023
@ozankabak
Copy link
Contributor

ozankabak commented Apr 7, 2023

It needs:

impl From<&PhysicalSortRequirement> for PhysicalSortExpr {
    fn from(value: &PhysicalSortRequirement) -> Self {
        value.clone().into_sort_expr()
    }
}

and

impl From<&PhysicalSortExpr> for PhysicalSortRequirement {
    fn from(value: &PhysicalSortExpr) -> Self {
        PhysicalSortRequirement::from(value.clone())
    }
}

so that to_sort_exprs and from_sort_exprs can work with both owned and reference arguments, cloning only in the latter case when necessary.

@alamb
Copy link
Contributor Author

alamb commented Apr 28, 2023

Doesn't seem like we have made progress here, closing

@alamb alamb closed this Apr 28, 2023
@alamb alamb changed the title Use generics instead of borrow for zero-cost Use generics instead of borrow for zero-cost in PhysicalSortExpr Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate physical-expr Changes to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants