alamb commented on code in PR #10807:
URL: https://github.com/apache/datafusion/pull/10807#discussion_r1630226174
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -861,105 +861,6 @@ mod test {
assert_eq!(expected, formatted_plan);
}
- #[test]
- fn id_array_visitor() -> Result<()> {
Review Comment:
I think I understand why you moved these tests to the core crate (as they
rely on aggregate functions that will soon not be direct dependencies on
datafusion-optimizer
However, with this change now the tests for individual passes may be spread
in two places (core and optimizer) so evaluating test coverage will be harder.
I think we should try to keep all the tests together
Also, I think it is quite nice that the tests are with each optimizer pass
(I found it quite helpful when I was working on avoiding as many copies in the
optimizer).
Options I can see:
1. Move all the other optimizer tests over to `datafusion/core`
2. find a way to leave the tests in `datafusion-optimizer`
One idea I had about keeping the tests in `datafusion-optimizer` would be
to add stu aggregate functions to the optimizer crate (for example we could add
a `sum()` `AggregateUDF` that had the same signature as sum but didn't actually
run (would panic if we tried to create an accumulator, etc)?
The downside is that then there is a danger that the stubs don't behave the
same way as the actual functions and there are bugs / limitations when they are
used together...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
