yjhjstz commented on code in PR #2006:
URL: https://github.com/apache/cloudberry/pull/2006#discussion_r4032367390
##########
src/backend/gporca/libnaucrates/src/statistics/CFilterStatsProcessor.cpp:
##########
@@ -144,64 +150,30 @@ CFilterStatsProcessor::SelectivityOfPredicate(CMemoryPool
*mp,
GPOS_ASSERT(nullptr != local_col_ref);
CDouble ndv =
result_stats->GetNDVs(local_col_ref);
- if (ndv < 1.0)
- {
- // An NDV of less than 1 means
that we have no stats on this column
- result = result *
CHistogram::DefaultSelectivity;
- }
- else
+ // an NDV below 1 means that we have no
stats on this column
+ if (ndv >= 1.0)
{
- result = result * (1 / ndv);
+ scale_factor = ndv;
}
}
- else
- {
- // a comparison col op <outer ref>
other than an equals
- result = result *
CHistogram::DefaultSelectivity;
- }
- num_outer_ref_preds++;
- }
- else
- {
- // if it is a true filter, then we had no
expressions with outer refs
- if (!CUtils::FScalarConstTrue(pexpr))
- {
- // some other expression, not of the
form col op <outer ref>,
- // e.g. an OR expression
- result = result *
CHistogram::DefaultSelectivity;
- num_outer_ref_preds++;
- }
}
+ outer_scale_factors->Append(GPOS_NEW(mp)
CDouble(scale_factor));
}
expr_with_outer_refs->Release();
outer_ref_exprs->Release();
}
- // apply damping factor to the outer ref predicates whose selectivities
we multiplied above
- if (have_local_preds)
- {
- // add one for the combined non-outer refs which were dampened
internally,
- // but not in combination with the preds on outer refs
- num_outer_ref_preds++;
- }
- if (1 < num_outer_ref_preds)
- {
- CStatisticsConfig *stats_config =
- CStatisticsConfig::PstatsconfDefault(mp);
-
- result =
- std::min(result.Get() /
CScaleFactorUtils::DampedFilterScaleFactor(
-
stats_config, num_outer_ref_preds)
-
.Get(),
- 1.0);
-
- stats_config->Release();
- }
+ const CDouble outer_scale_factor =
+ CScaleFactorUtils::CalcScaleFactorCumulativeConj(stats_config,
Review Comment:
Reusing `CalcScaleFactorCumulativeConj` here changes the damping shape for
the **outer-only** case (no local predicate), and for 3+ outer refs it is more
aggressive than the code it replaces:
- old: `prod(1/ndv_i) / 0.75^n` (one division for the whole group;
`have_local_preds` is false so no +1)
- new: factor k (sorted desc) is divided by `0.75^k` for k >= 1, i.e.
`0.75^(1+2+...+(n-1))`
n=2: both x1.78. n=3: old x2.37, new x5.62. n=4: old x3.16, new x17.8.
Repro on the patched build (AO table, `btree (x, y, z)`, NDV 3 each, 10k
rows, replicated 1-row outer):
```sql
CREATE TABLE ao3 (id int, x int, y int, z int, w int) WITH (appendonly=true)
DISTRIBUTED RANDOMLY;
INSERT INTO ao3 SELECT n, n % 3, (n/3) % 3, (n/9) % 3, n % 7 FROM
generate_series(0, 9999) g(n);
CREATE INDEX ao3_xyz ON ao3 USING btree (x, y, z);
CREATE TABLE out3 (x int, y int, z int) DISTRIBUTED REPLICATED;
INSERT INTO out3 VALUES (0,0,0);
ANALYZE ao3; ANALYZE out3;
SET optimizer_enable_hashjoin = off;
EXPLAIN SELECT i.* FROM out3 o CROSS JOIN ao3 i WHERE i.x = o.x AND i.y =
o.y AND i.z = o.z;
```
True selectivity is 371/10000 = 0.037. gdb at the return: old `0.0878`
(below the 0.10 AO-btree gate at `CXformUtils.cpp`
`AO_TABLE_BTREE_INDEX_SELECTIVITY_THRESHOLD`) -> `Bitmap Index Scan on
ao3_xyz`; new `local=1 outer_sf=6.407 result=0.156` -> the btree is rejected
and the plan becomes `Seq Scan on ao3` + `Join Filter: ((i.x = o.x) AND (i.y =
o.y) AND (i.z = o.z))`. So for this shape the PR both worsens the estimate and
loses the index.
##########
src/backend/gporca/libnaucrates/src/statistics/CFilterStatsProcessor.cpp:
##########
@@ -132,7 +132,13 @@ CFilterStatsProcessor::SelectivityOfPredicate(CMemoryPool
*mp,
for (ULONG ul = 0; ul < size; ul++)
{
CExpression *pexpr = (*outer_ref_exprs)[ul];
+ if (CUtils::FScalarConstTrue(pexpr))
+ {
+ continue;
+ }
+
CColRef *local_col_ref = nullptr;
+ CDouble scale_factor = 1 /
CHistogram::DefaultSelectivity;
Review Comment:
Minor, and pre-existing, but since this block is being rewritten with
`ParseCmpType()` already available: these outer-ref conjuncts are join
predicates from a statistics standpoint (see the comment above
`DeriveStatsWithOuterRefs` in `CJoinStatsProcessor.cpp`), and the real join
pipeline scores them differently:
- `<, <=, >, >=` ->
`CScaleFactorUtils::DefaultInequalityJoinPredScaleFactor` (3.0) in
`CHistogram.cpp`
- unsupported / complex (e.g. an OR of outer refs) ->
`CScaleFactorUtils::DefaultJoinPredScaleFactor` (100) in
`CJoinStatsProcessor.cpp`
Here both fall into `1 / CHistogram::DefaultSelectivity` (2.5), so the same
predicate is scored 2.5 for index ranking and 3.0 (or 100) for cardinality.
Using the named join constants would align the two without changing the
structure; `1 / CHistogram::DefaultSelectivity` is also already spelled out
verbatim in three other places in stats code, so a named constant would help
either way.
##########
src/backend/gporca/libnaucrates/src/statistics/CFilterStatsProcessor.cpp:
##########
@@ -144,64 +150,30 @@ CFilterStatsProcessor::SelectivityOfPredicate(CMemoryPool
*mp,
GPOS_ASSERT(nullptr != local_col_ref);
CDouble ndv =
result_stats->GetNDVs(local_col_ref);
- if (ndv < 1.0)
- {
- // An NDV of less than 1 means
that we have no stats on this column
- result = result *
CHistogram::DefaultSelectivity;
- }
- else
+ // an NDV below 1 means that we have no
stats on this column
+ if (ndv >= 1.0)
{
- result = result * (1 / ndv);
+ scale_factor = ndv;
}
}
- else
- {
- // a comparison col op <outer ref>
other than an equals
- result = result *
CHistogram::DefaultSelectivity;
- }
- num_outer_ref_preds++;
- }
- else
- {
- // if it is a true filter, then we had no
expressions with outer refs
- if (!CUtils::FScalarConstTrue(pexpr))
- {
- // some other expression, not of the
form col op <outer ref>,
- // e.g. an OR expression
- result = result *
CHistogram::DefaultSelectivity;
- num_outer_ref_preds++;
- }
}
+ outer_scale_factors->Append(GPOS_NEW(mp)
CDouble(scale_factor));
}
expr_with_outer_refs->Release();
outer_ref_exprs->Release();
}
- // apply damping factor to the outer ref predicates whose selectivities
we multiplied above
- if (have_local_preds)
- {
- // add one for the combined non-outer refs which were dampened
internally,
- // but not in combination with the preds on outer refs
- num_outer_ref_preds++;
- }
- if (1 < num_outer_ref_preds)
- {
- CStatisticsConfig *stats_config =
- CStatisticsConfig::PstatsconfDefault(mp);
-
- result =
- std::min(result.Get() /
CScaleFactorUtils::DampedFilterScaleFactor(
-
stats_config, num_outer_ref_preds)
-
.Get(),
- 1.0);
-
- stats_config->Release();
- }
+ const CDouble outer_scale_factor =
+ CScaleFactorUtils::CalcScaleFactorCumulativeConj(stats_config,
+
outer_scale_factors);
+ outer_scale_factors->Release();
result_stats->Release();
local_expr->Release();
- return result;
+ // Outer selectivities are conditional on the local filter. Damping only
+ // their conjunction preserves the local estimate as an upper bound.
+ return local_selectivity / outer_scale_factor;
Review Comment:
This treats the local group and the outer group as independent (no damping
between them), whereas the old code counted the local group as one more damped
predicate. Two consequences I measured:
- A single outer equality with a local predicate gets **no damping at all**:
`a=1 AND b=o.b` -> old `0.0356`, new `0.0200` (= 0.1 * 1/5 exactly).
- ORCA's local path (`MakeHistHashMapConjFilter` ->
`CalcScaleFactorCumulativeConj`) *does* damp two local equalities. So with
identical NDVs (say 100 each), `a=5 AND b=6` scores `1/(100 * max(1,
100*0.75^2)) = 1.78e-4`, while `a=5 AND c=<outer>` scores `0.01/100 = 1.0e-4`.
The outer-ref constant is ranked as strictly more selective than a literal
purely because of its syntactic form, which biases `PexprBitmapSelectBestIndex`
toward indexes covering outer-ref columns.
The stated invariant (`result <= local_selectivity`) does not require
dropping cross-group damping: if you append `1 / local_selectivity` to
`outer_scale_factors` and return `1 / CalcScaleFactorCumulativeConj(...)`, the
product is still >= its largest factor, so the result stays <= min(local, each
outer) while local and outer predicates are damped together, consistent with
how purely local conjunctions are handled.
--
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]
