Skip to content

Make ExecutionPlan implementations immutable #987

New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom
Closed
rdettai opened this issue Sep 10, 2021 · 2 comments
Closed

Make ExecutionPlan implementations immutable #987

rdettai opened this issue Sep 10, 2021 · 2 comments
Labels
enhancement New feature or request

Comments

@rdettai
Copy link
Contributor

rdettai commented Sep 10, 2021

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
This is a follow up to #962.

Currently we cannot compute all the statistics at instantiation because some implementations of the ExecutionPlan trait are mutable.

Describe the solution you'd like
The proposition is to make all ExecutionPlan implementations immutable and then transform the statistics method signature to:
fn statistics(&self) -> &Statistics;

Describe alternatives you've considered
Instead of trying to compute the statistics at instantiation, a solution could be to have statistics(&mut self) and cache the result from there. But if the plan is mutable this is error prone because the cached statistics needs to be correctly invalidated when updates occur.

Additional context
#965 (comment)
@rdettai rdettai added the enhancement New feature or request label Sep 10, 2021
@rdettai rdettai mentioned this issue Sep 10, 2021
Merged
@mingmwang
Copy link
Contributor

mingmwang commented Oct 28, 2022 •
edited
Loading

Is it still valid? I think today's ExecutionPlan is already immutable except for the run time plan metrics.

@rdettai
Copy link
Contributor Author

rdettai commented Nov 6, 2022

I think a lot of things have changed since I opened this. Closing it.

@rdettai rdettai closed this as not planned Won't fix, can't repro, duplicate, stale Nov 6, 2022
unkloud pushed a commit to unkloud/datafusion that referenced this issue Mar 23, 2025
@mbutrovich
feat: Implement bloom_filter_agg (apache#987)
e3ac6cf 
* Add test that invokes bloom_filter_agg.

* QueryPlanSerde support for BloomFilterAgg.

* Add bloom_filter_agg based on sample UDAF. planner instantiates it now. Added spark_bit_array_tests.

* Partial work on Accumulator. Need to finish merge_batch and state.

* BloomFilterAgg state, merge_state, and evaluate. Need more tests.

* Matches Spark behavior. Need to clean up the code quite a bit, and do `cargo clippy`.

* Remove old comment.

* Clippy. Increase bloom filter size back to Spark's default.

* API cleanup.

* API cleanup.

* Add BloomFilterAgg benchmark to CometExecBenchmark

* Docs.

* API cleanup, fix merge_bits to update cardinality.

* Refactor merge_bits to update bit_count with the bit merging.

* Remove benchmark results file.

* Docs.

* Add native side benchmarks.

* Adjust benchmark parameters to match Spark defaults.

* Address review feedback.

* Add assertion to merge_batch.

* Address some review feedback.

* Only generate native BloomFilterAgg if child has LongType.

* Add TODO with GitHub issue link.
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@rdettai @mingmwang