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

Consolidate Example: simplify_udaf_expression.rs into advanced_udaf.rs #13842

Closed
Tracked by #11172
alamb opened this issue Dec 19, 2024 · 5 comments · Fixed by #13905
Closed
Tracked by #11172

Consolidate Example: simplify_udaf_expression.rs into advanced_udaf.rs #13842

alamb opened this issue Dec 19, 2024 · 5 comments · Fixed by #13905
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@alamb
Copy link
Contributor

alamb commented Dec 19, 2024

Is your feature request related to a problem or challenge?

The large number of small examples in DataFusion leads to multiple problems:

  • Unnecessarily large amounts of temporary disk space (each example binary takes 100+ MB)
  • Larger cargo t time (each binary must be linked / created)
  • Makes it harder to find the relevant examples

Thus I think the fewer examples we have the

  1. faster our dev experience will be
  2. easier it will be to find the relevant examples

Describe the solution you'd like

Then update the readme page:

Describe alternatives you've considered

No response

Additional context

I think this is a good first issue as the task is straightforward and doesn't require deep understanding of the DataFusion codebase

@alamb alamb added enhancement New feature or request good first issue Good for newcomers labels Dec 19, 2024
@alamb alamb changed the title Consolidate parquet.sql into Consolidate Example: simplify_udaf_expression.rs into advanced_udaf.rs Dec 19, 2024
@takaebato
Copy link
Contributor

take

@alamb
Copy link
Contributor Author

alamb commented Dec 20, 2024

Thank you @takaebato 🙏

@takaebato
Copy link
Contributor

takaebato commented Dec 24, 2024

@alamb

Sorry for the delay🙏 But I have a quick question!
I’m not too sure about this, but should the return type of AggregateFunctionSimplification perhaps be Result<ExprSimplifyResult>(or a similar enum that can indicate whether or not simplification occurred)?

Currently, it doesn’t seem possible to skip simplification, as it always returns a simplified Expr (or Err).
The simplify method can return an Option, but the decision to simplify seems to rely on information passed to AggregateFunctionSimplification, so might it be better if this method can choose whether or not to simplify..?

@alamb
Copy link
Contributor Author

alamb commented Dec 24, 2024

@alamb

Sorry for the delay🙏 But I have a quick question! I’m not too sure about this, but should the return type of AggregateFunctionSimplification perhaps be Result<ExprSimplifyResult>(or a similar enum that can indicate whether or not simplification occurred)?

Currently, it doesn’t seem possible to skip simplification, as it always returns a simplified Expr (or Err). The simplify method can return an Option, but the decision to simplify seems to rely on information passed to AggregateFunctionSimplification, so might it be better if this method can choose whether or not to simplify..?

The rationale for returning ExprSimplifyResult is so that the argument doesn't have to be cloned -- aka to avoid coping the ownership of argument is passed to the function and there needs to be some way to get the original structuer back

@takaebato
Copy link
Contributor

takaebato commented Dec 26, 2024

@alamb

Thank you so much for the explanation!!
(I also realized that if we want to use the actual implementation conditionally, we can return Ok(Expr::AggregateFunction(aggregate_function)).)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants