Skip to content

[Bug] Handle zero filter damping in ORCA conjunctions #2007

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Waloid24 wants to merge 2 commits into
base: main
Choose a base branch
from
+15 −2
Open

[Bug] Handle zero filter damping in ORCA conjunctions #2007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7bb33e0
Handle zero filter damping in ORCA conjunctions
Sep 15, 2026
5e64fe7
Allow damping_factor_groupby to be zero
Sep 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/backend/gporca/libgpopt/src/engine/CStatisticsConfig.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ CStatisticsConfig::CStatisticsConfig(CMemoryPool *mp,
m_max_stats_buckets(max_stats_buckets),
m_phsmdidcolinfo(nullptr)
{
GPOS_ASSERT(CDouble(0.0) < damping_factor_filter);
GPOS_ASSERT(CDouble(0.0) <= damping_factor_filter);
GPOS_ASSERT(CDouble(0.0) <= damping_factor_join);
GPOS_ASSERT(CDouble(0.0) < damping_factor_groupby);
GPOS_ASSERT(CDouble(0.0) <= damping_factor_groupby);
GPOS_ASSERT(0 < max_stats_buckets);

//m_phmmdidcolinfo = New(m_mp) HMMDIdMissingstatscol(m_mp);
Expand Down
13 changes: 13 additions & 0 deletions src/backend/gporca/libnaucrates/src/statistics/CScaleFactorUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,12 +502,25 @@ CScaleFactorUtils::CalcScaleFactorCumulativeConj(

const ULONG num_cols = scale_factors->Size();
CDouble scale_factor(1.0);
if (0 == num_cols)

@yjhjstz yjhjstz Sep 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this reachable? The only caller (CFilterStatsProcessor::MakeHistHashMapConjFilter) unconditionally appends last_scale_factor before calling, so Size() >= 1, and the pre-existing loop already returned 1.0 for zero iterations. It seems to exist only to guard the new (*scale_factors)[0] below. If the zero branch is kept, the sibling CalcScaleFactorCumulativeDisj expresses the same precondition as GPOS_ASSERT(0 < num_cols), which may be the more consistent form.

@Waloid24 Waloid24 Sep 17, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, GPOS_ASSERT(0 < num_cols) will be better for the current usage of this function. We always add one element to the scale_factors array before calling CalcScaleFactorCumulativeConj, so for the possible future changes from other contributors it will be better to express our expectations with GPOS_ASSERT.

@Waloid24 Waloid24 Sep 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look at #2006 I reuse CalcScaleFactorCumulativeConj there and it's possible that there will be no outer predicates, so in this case I expect CalcScaleFactorCumulativeConj will return 1.0. This check if (0 == num_cols) appears due to the reusage of this function in #2006.

{
return scale_factor;
}

if (1 < num_cols)
{
// sort (in desc order) the scaling factor based on the selectivity of each column
scale_factors->Sort(CScaleFactorUtils::DescendingOrderCmpFunc);
}

if (CDouble(0.0) == stats_config->DDampingFactorFilter())

@yjhjstz yjhjstz Sep 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this branch computes exactly what the loop below already computes when the damping factor is 0, so it does not change behavior:

  • CDouble clamps 0 to GPOS_FP_ABS_MIN (1e-250).
  • DampedFilterScaleFactor(cfg, 1) returns 1.0, so the first term is max(MinRows, sf[0]).
  • For ul >= 1, Pow(1e-250, n) re-clamps to 1e-250; sf * 1e-250 <= 1e250 * 1e-250 = 1, so max(MinRows=1.0, ...) is 1.0 for every remaining term.
  • Product = max(1.0, sf[0]) -- identical to this return.

Empirically, SET optimizer_damping_factor_filter = 1e-200 (old loop path) and = 0 (this branch) give bit-identical estimates on the test-plan tables (AND: rows=334, OR: rows=835). So the comment's "rather than relying on CDouble's minimum magnitude" describes a difference that does not materialize. I would suggest dropping the branch (the assert change alone fixes the fallback) or replacing it with a comment explaining why 0 degenerates to the largest scale factor. Also note CDouble::operator== is itself a 1e-250-tolerance compare on clamped values, so the test does rely on the minimum magnitude.

@Waloid24 Waloid24 Sep 17, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the comment "Handle zero explicitly rather than relying on CDouble's minimum magnitude when computing powers" is excessive.

But can we keep this branch as a quick way to return from the function to avoid the (unnecessary in this situation) loop below?

{
// Maximum overlap: retain only the smallest selectivity, represented
// by the largest scale factor. Handle zero explicitly rather than
// relying on CDouble's minimum magnitude when computing powers.
return std::max(CStatistics::MinRows.Get(), (*scale_factors)[0]->Get());
}

for (ULONG ul = 0; ul < num_cols; ul++)
{
// apply damping factor
Expand Down