Simplify percentile_cont for 0/1 percentiles#18837
Conversation
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
I suggest to add some tests in sqllogictest https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest
It should run some SQL queries that this optimization is applicable, and we first ensure the result is expected, and also do a EXPLAIN to ensure such optimization is applied.
In fact, we can move most of the test coverage to sqllogictests, instead of unit tests here. The reason is:
- SQL tests are simpler to maintain
- The SQL interface is more stable, while internal APIs may change frequently. As a result, good test coverage here can easily get lost during refactoring.
There was a problem hiding this comment.
I kept the unit tests along with the new sql test in the sqllogictest. Should I remove the unit tests or is it okay?
There was a problem hiding this comment.
We should remove the unit tests if they duplicate the sqllogictests
There was a problem hiding this comment.
We should remove the unit tests if they duplicate the sqllogictests
+1 unless there are something can't be covered by slt tests
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
Thanks for picking this up. Have a few suggestions to simplify the code
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
We should remove the unit tests if they duplicate the sqllogictests
|
Thanks @kumarUjjawal |
4 participants
Which issue does this PR close?
percentile_contto min/max when percentile is 0 or 1 #18108Rationale for this change
Literal 0/1 percentiles don’t need percentile buffering; using min/max keeps results identical.
What changes are included in this PR?
Are these changes tested?
Added tests
Are there any user-facing changes?