phillipleblanc commented on PR #2216:
URL:
https://github.com/apache/datafusion-ballista/pull/2216#issuecomment-5186828272
I wonder if it would make sense to support both and let the user choose what
they want to use?
i.e. something like `ballista.planner.range_partitioning.sketch = auto |
tdigest | kll`
- `auto` - recommended default. Use TDigest for simple numeric data when
it is faster; use KLL when multiple columns, nulls, or other types require it.
- `tdigest` - force the existing implementation. Reject queries it cannot
support.
- `kll` - force the new implementation for testing or predictable behavior.
I wouldn't add that to this PR, but for when we eventually get to wiring
itup.
Other than that, I think this makes sense - some small comments:
- **Compare equal accuracy levels**. The PR compares `KLL` and `TDigest`
using inconsistent accuracy calculations. It is like comparing two compressors
using different image-quality settings. First choose settings that produce
roughly equal accuracy, then compare speed and memory.
- **Give both algorithms equally sorted input**. The fastest KLL test
receives already-sorted data, while TDigest receives unsorted data and must
sort it itself. That is not an apples-to-apples race. Add a TDigest test that
receives the same sorted data before claiming KLL is 1.9x faster.
--
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]
