refactor: Move LimitedDistinctAggregation to physical-optimizer crate#12036
Conversation
Weijun-H
changed the title
LimitedDistinctAggregation to physical-optimizer crate
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @Weijun-H
Any concerns @lewiszlw or @jayzhan211 ?
|
|
||
| //! Tests for the limited distinct aggregation optimizer rule | ||
|
|
||
| use std::sync::Arc; |
There was a problem hiding this comment.
I think if possible it would be best to keep these tests alongside the code in limited_distinct_aggregation.rs though I know in some cases the tests have too many dependencies to the core crate for that to be feasible
Even though most of the the use of the core crate is SessionContext which could be removed, I think the use of ParquetExec in the tests makes moving these tests the best option at this point
There was a problem hiding this comment.
Nice idea! We could try to eliminate SessionContext in the test later.
There was a problem hiding this comment.
Nice idea! We could try to eliminate SessionContext in the test later.
Thanks -- I think we could remove SessionContext -- the problem is that ParquetExec is also in the core datafusion crate which is not nearly as easy to remove from the tests I don't think.
We could make a StubParquetExec or something for testing the physical optimizer passes (which would allow us to keep the tests in the datafusion-physical-optimizer), but then I worry that the real parquet exec would diverge from the stub and the tests would not catch regressions.
|
|
||
| //! Tests for the limited distinct aggregation optimizer rule | ||
|
|
||
| use std::sync::Arc; |
There was a problem hiding this comment.
I think if possible it would be best to keep these tests alongside the code in limited_distinct_aggregation.rs though I know in some cases the tests have too many dependencies to the core crate for that to be feasible
Even though most of the the use of the core crate is SessionContext which could be removed, I think the use of ParquetExec in the tests makes moving these tests the best option at this point
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
3 participants
Which issue does this PR close?
part of #11502
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?