You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "jackkleeman (via GitHub)" <gi...@apache.org> on 2025/09/08 15:43:49 UTC
[PR] Support join cardinality estimation if distinct_count is set [datafusion]
jackkleeman opened a new pull request, #17476:
URL: https://github.com/apache/datafusion/pull/17476
The goal of this PR is to allow cardinality statistics being passed through joins even if fields don't have max and min values set, as long as a distinct value estimate is provided.
Currently we require max and min to be set, as they might be used to estimate the distinct count. This is unnecessarily conservative if distinct_count has actually been provided, in which case max and min won't be used at all and the presence of max or min has no influence over how good of an estimate it is.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support join cardinality estimation if distinct_count is set [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #17476:
URL: https://github.com/apache/datafusion/pull/17476#issuecomment-3281031486
I definitely agree with the alternative
> it seems to me that we should either a) not allow unbounded range, in fact only allow ranges that are numeric (so cardinality info is available) or b) not require range at all
Since we're debating a change that will do (b) and thus make the code more coherent, I agree to proceed with that.
I am not taking a stance which one of (a) or (b) is better. We can switch from (b) to (a) later, when we known which one is better.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support join cardinality estimation less conservatively [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #17476:
URL: https://github.com/apache/datafusion/pull/17476#issuecomment-3283818075
@alamb @ozankabak PTAL
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support join cardinality estimation if distinct_count is set [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #17476:
URL: https://github.com/apache/datafusion/pull/17476#discussion_r2335808457
##########
datafusion/physical-plan/src/joins/utils.rs:
##########
@@ -563,11 +563,14 @@ fn estimate_inner_join_cardinality(
.iter()
.zip(right_stats.column_statistics.iter())
{
- // Break if any of statistics bounds are undefined
- if left_stat.min_value.get_value().is_none()
- || left_stat.max_value.get_value().is_none()
- || right_stat.min_value.get_value().is_none()
- || right_stat.max_value.get_value().is_none()
+ // Break if we don't have enough information to calculate a distinct count
+ // If distinct_count isn't provided directly, we need min and max to be provided
+ if (left_stat.distinct_count.get_value().is_none()
+ && (left_stat.min_value.get_value().is_none()
+ || left_stat.max_value.get_value().is_none()))
+ || (right_stat.distinct_count.get_value().is_none()
+ && (right_stat.min_value.get_value().is_none()
+ || right_stat.max_value.get_value().is_none()))
Review Comment:
Would this be easier to read?
```suggestion
let has_enough_info = |stat: &ColumnStatistics| {
stat.distinct_count.get_value().is_some()
|| (stat.min_value.get_value().is_some()
&& stat.max_value.get_value().is_some())
};
if !has_enough_info(left_stat) || !has_enough_info(right_stat)
```
##########
datafusion/physical-plan/src/joins/utils.rs:
##########
@@ -2016,20 +2019,20 @@ mod tests {
),
// When we have distinct count.
(
- (10, Inexact(1), Inexact(10), Inexact(10), Absent),
- (10, Inexact(1), Inexact(10), Inexact(10), Absent),
Review Comment:
Why change existing test cases?
Maybe let's just add new ones?
##########
datafusion/physical-plan/src/joins/utils.rs:
##########
@@ -2071,18 +2074,13 @@ mod tests {
),
// No min or max (or both).
(
- (10, Absent, Absent, Inexact(3), Absent),
- (10, Absent, Absent, Inexact(3), Absent),
- None,
- ),
- (
- (10, Inexact(2), Absent, Inexact(3), Absent),
- (10, Absent, Inexact(5), Inexact(3), Absent),
+ (10, Absent, Absent, Absent, Absent),
+ (10, Absent, Absent, Absent, Absent),
Review Comment:
This became exact same as "No column level stats." input (one above).
Let's not take changing inputs in existing test cases lightly.
If they are mislabelled, perhaps let's just fix the labels
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support join cardinality estimation if distinct_count is set [datafusion]
Posted by "jackkleeman (via GitHub)" <gi...@apache.org>.
jackkleeman commented on code in PR #17476:
URL: https://github.com/apache/datafusion/pull/17476#discussion_r2336102151
##########
datafusion/physical-plan/src/joins/utils.rs:
##########
@@ -2069,20 +2077,25 @@ mod tests {
(10, Absent, Absent, Absent, Absent),
None,
),
- // No min or max (or both).
+ // No min or max (or both), but distinct available.
(
(10, Absent, Absent, Inexact(3), Absent),
(10, Absent, Absent, Inexact(3), Absent),
- None,
+ Some(Inexact(33)),
Review Comment:
the behaviour of these three have changed because we will use the distinct values even though we have no range
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support join cardinality estimation if distinct_count is set [datafusion]
Posted by "jackkleeman (via GitHub)" <gi...@apache.org>.
jackkleeman commented on code in PR #17476:
URL: https://github.com/apache/datafusion/pull/17476#discussion_r2336102151
##########
datafusion/physical-plan/src/joins/utils.rs:
##########
@@ -2069,20 +2077,25 @@ mod tests {
(10, Absent, Absent, Absent, Absent),
None,
),
- // No min or max (or both).
+ // No min or max (or both), but distinct available.
(
(10, Absent, Absent, Inexact(3), Absent),
(10, Absent, Absent, Inexact(3), Absent),
- None,
+ Some(Inexact(33)),
Review Comment:
the behaviour of these two have changed because we will use the distinct values even though we have no range
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support join cardinality estimation less conservatively [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #17476:
URL: https://github.com/apache/datafusion/pull/17476
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support join cardinality estimation if distinct_count is set [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #17476:
URL: https://github.com/apache/datafusion/pull/17476#issuecomment-3275464684
> if you dont provide distinct count, we require you to provide range. however, we accept even an unbounded range, which is kind of no information - in that case we just use the row count as our guess, basically.
That's a good observation.
This explains test changes I got after removal of the condition block, rather than make it more complicated.
- https://github.com/apache/datafusion/blob/a951fc9c33416a6b240bf24a0120fce3ca37914f/datafusion/physical-plan/src/joins/utils.rs#L566-L573
So maybe we remove the condition after all?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support join cardinality estimation if distinct_count is set [datafusion]
Posted by "jackkleeman (via GitHub)" <gi...@apache.org>.
jackkleeman commented on PR #17476:
URL: https://github.com/apache/datafusion/pull/17476#issuecomment-3280208885
Id be inclined to remove the check entirely and always use a row count as a default guess for cardinality. If you agree I will do this change
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support join cardinality estimation if distinct_count is set [datafusion]
Posted by "jackkleeman (via GitHub)" <gi...@apache.org>.
jackkleeman commented on PR #17476:
URL: https://github.com/apache/datafusion/pull/17476#issuecomment-3274056503
should be ok now.
another thing i noticed; if you dont provide distinct count, we require you to provide range. however, we accept even an unbounded range, which is kind of no information - in that case we just use the row count as our guess, basically. also we allow string ranges which have no cardinalirty info. is this intentional? it seems to me that we should either a) not allow unbounded range, in fact only allow ranges that are numeric (so cardinality info is available) or b) not require range at all
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support join cardinality estimation less conservatively [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17476:
URL: https://github.com/apache/datafusion/pull/17476#issuecomment-3334759994
Thanks @jackkleeman @findepi and @xudong963 !
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support join cardinality estimation less conservatively [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17476:
URL: https://github.com/apache/datafusion/pull/17476#issuecomment-3334757451
Maybe related to
- #17718
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org