Minor: Refactor memory size estimation for HashTable

[marvinlanhenke]

Which issue does this PR close?

Closes #8764.

Rationale for this change

As stated in the issue here the goal is to consolidate estimation logic into a single function. This allows for better documentation as well as better maintainability.
Also there was an issue with the existing implementation that could still overflow if the checked_mul was capped at usize::MAX. I tried to fix this by checking for overflows and returning usize::MAX as a 'max cap'.

What changes are included in this PR?

• extract logic into single function
• fixed missing overflow checks
• moved logic into datfusion common
• used the new function in count_distinct/native.rs and hash_join.rs

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

[marvinlanhenke]

@alamb @yyy1000 PTAL

[yyy1000]

I see it's good abstraction.

My only concern is, it seems that the original goal was kind of ideal, which just pass the hashtable and get the estimate size. But now before using this function, user have to calculate the fixed_size themself, and also the calculation is not always the same.

Though the abstraction is not pretty enough, I think given the current code this PR is still an improvement. :)

[marvinlanhenke]

I see it's good abstraction.

My only concern is, it seems that the original goal was kind of ideal, which just pass the hashtable and get the estimate size. But now before using this function, user have to calculate the fixed_size themself, and also the calculation is not always the same.

Though the abstraction is not pretty enough, I think given the current code this PR is still an improvement. :)

Yes, I totally agree with you on this. However, as discussed in the issue, it still might be worth it due to the improved documentation.

[yyy1000]

However, as discussed in the issue, it still might be worth it due to the improved documentation.

Agree! I left a comment which may improve but it's not necessary. Thanks!

[alamb]

Thank you so much @marvinlanhenke -- this looks like a great step forward in readability 🙏

Thank you @yyy1000 for your review

I left some suggestions / comments. Let me know what you think

[marvinlanhenke]

@alamb
I made the changes based on your comments. Thanks for the comprehensive review. 🚀

I changed fn estimate_memory_size(..) to return Result<usize> which should set us up for any possible future changes in the Accumulator trait; for now I simply unwrap in fn size() and panic.

[alamb]

Thank you @marvinlanhenke

[alamb]

Thanks again @marvinlanhenke