Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite UDAF reversed expression name #11629

Open
jayzhan211 opened this issue Jul 24, 2024 · 4 comments
Open

Rewrite UDAF reversed expression name #11629

jayzhan211 opened this issue Jul 24, 2024 · 4 comments
Assignees

Comments

@jayzhan211
Copy link
Contributor

jayzhan211 commented Jul 24, 2024

          I think it would be good to eventually move this to a method in `https://github.com/apache/datafusion/pull/11611` though I agree this is good for now. Maybe we can file a ticket to track

Originally posted by @alamb in #11611 (comment)

I think we could extend to rewrite the whole expression name, and it could be more straightforward what the name is rewritten

                let mut name = self.name().to_string();
                // If the function is changed, we need to reverse order_by clause as well
                // i.e. First(a order by b asc null first) -> Last(a order by b desc null last)
                if self.fun().name() == reverse_udf.name() {
                } else {
                    replace_order_by_clause(&mut name);
                }
                replace_fn_name_clause(&mut name, self.fun.name(), reverse_udf.name());

Something like

let mut name = self.name().to_string();
name = self.fun.reverse_name(name)

Add reverse_name in AggregateUDFImpl

trait AggregateUDFImpl {
    fn reverse_name(&self) -> String {
        ...
    }
}

Specificially we need to rewrite name for First/Last. For array_agg, the name should be the same.

@dharanad
Copy link
Contributor

take

@dharanad
Copy link
Contributor

I plan to close this by 10th Aug. I hope that okay ? Feel free to re assign if this is a priority

@dharanad
Copy link
Contributor

Apologies for the delay here. Somehow i missed this. @jayzhan211 I went through the conversation and somewhat understood the issue. But for better clarity can you share more info here. Once again sorry, for the delay here.

@jayzhan211
Copy link
Contributor Author

What you need to do is, add a new method reverse_name

trait AggregateUDFImpl {
    fn reverse_name(&self) -> String {
        ...
    }
}

Replace with the code here

                let mut name = self.name().to_string();
                // If the function is changed, we need to reverse order_by clause as well
                // i.e. First(a order by b asc null first) -> Last(a order by b desc null last)
                if self.fun().name() == reverse_udf.name() {
                } else {
                    replace_order_by_clause(&mut name);
                }
                replace_fn_name_clause(&mut name, self.fun.name(), reverse_udf.name());

if the function name is changed (i.e. First -> Last), order by clause required to rewrite too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants