Support join cardinality estimation less conservatively

[jackkleeman]

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.

[xudong963]

Good finding, after resolving the comments, i think it's good to go

[jackkleeman]

the behaviour of these three have changed because we will use the distinct values even though we have no range

[jackkleeman]

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

[findepi]

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.

• datafusion/datafusion/physical-plan/src/joins/utils.rs

  Lines 566 to 573 in a951fc9

  	// 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()
  	{
  	return None;
  	}

So maybe we remove the condition after all?

[jackkleeman]

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

[findepi]

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.

[findepi]

@alamb @ozankabak PTAL

[alamb]

Maybe related to

• Discussion: API for Join Access Path and Join Order Selection #17718

[alamb]

Thanks @jackkleeman @findepi and @xudong963 !