gortiz opened a new pull request, #19441:
URL: https://github.com/apache/pinot/pull/19441
> **Draft until #19409, #19410 and #19411 merge.** This is stacked on them.
GitHub compares against
> `master`, so the diff shown here also contains their commits. **The change
under review here is the
> single commit "Feed broker statistics to the Calcite planner" (+2,279).**
I will rebase and mark
> this ready as the parents land.
Contributes to #18740. Part of the split that replaced #18741.
The previous three PRs collect per-segment statistics and store them
broker-locally. Nothing read
them. This one makes the multi-stage planner read them.
```text
┌──────────────┐ ┌──────────────┐ ┌──────────────┐
┌──────────────────┐ ┌──────────────┐
│ 1 — contracts│──▶│ 2 — │──▶│ 3 — │──▶│ 4 — this PR
│──▶│ 5 — join │
│ + stores │ │ collection │ │ selection │ │ planner wiring
│ │ reordering │
└──────────────┘ └──────────────┘ └──────────────┘
└──────────────────┘ └──────────────┘
#19409 #19410 #19411
```
## What it does
`PinotTable#getStatistic()` surfaces the collected row count to Calcite, and
a
`PinotRelMdSelectivity` metadata handler uses statistics for filter and
time-range selectivity.
`BrokerStatisticsProvider` adapts the broker's stats manager to the
planner-facing SPI, wired
through `MultiStageBrokerRequestHandler`.
## Confidence is the load-bearing part
A statistic is used only when its confidence is `EXACT` or `ESTIMATED`.
`LOW` and `UNKNOWN` are
treated as **absent**, so the planner falls back to the heuristics it uses
today. That is what keeps
table types with systematically biased raw counts — upsert, dedup, tables
with consuming segments —
on their current behaviour rather than producing confidently wrong plans.
The gate is an allow-list (`StatConfidence.isUsableForCosting()`) applied in
one place, deliberately:
`StatConfidence` is documented append-only, so a confidence tier added later
must default to being
rejected rather than silently trusted. It is shared by the row-count and NDV
paths so the numerator
and denominator of one estimate cannot disagree.
## Known limitation, stated up front
Only **scan row counts** are statistics-backed. Join and filter selectivity
still come from
Calcite's constant `RelMdUtil.guessSelectivity` — 0.15 per equality
conjunct. Measured on TPC-H SF1,
the join cardinality for `lineitem ⋈ partsupp` is estimated ~18,000× high.
NDV-based selectivity is
Phase 2 of #18740.
This is a precision limitation rather than a correctness one — query results
never change, and
because final cardinality is order-invariant it does not by itself make plan
choice unsound — but it
bounds how much the estimates are currently worth.
## Reviewer notes
- **Off by default.** Requires `pinot.broker.stats.enabled=true` (default
false). A separate
`pinot.broker.multistage.use.statistics` acts as a kill switch: it stops
the planner consuming
statistics without discarding the collected store, so a regression can be
rolled back without
re-warming.
- **Calcite's metadata provider is only displaced when statistics can
actually answer.** With the
no-op provider the Pinot handlers would do real work per Filter and
TableScan only to fall through
to the same guess Calcite returns for free; brokers that never enabled
statistics should not pay
that, nor run a different selectivity code path.
- **`PinotTable` memoises** its statistics lookups. The store is written
continuously by the Helix
cluster-change threads that collect segment metadata, so an unmemoised
read could take the scan
cardinality from one snapshot and the selectivity denominator from
another, making the same query
plan differently between compiles.
- **`createMultiStageBrokerRequestHandler` gains a parameter.** Deliberate:
resolving the provider
inside the method body would let an existing override in a fork keep
compiling while silently
dropping the wiring, so `stats.enabled=true` would cost collection, change
nothing, and log
nothing.
- No wire-format, DataTable or segment-version change; nothing to coordinate
on a rolling upgrade.
## Testing
- `PinotRelMdSelectivityTest` — time-range and NDV selectivity, plus the
degradation paths: a
`SIMPLE_DATE_FORMAT` time column declines the time path rather than
reading a day number as an
instant; an unknown estimate falls back instead of collapsing selectivity
to zero; a
LOW-confidence row count disables the time path; a time predicate survives
a **reordering**
projection, which is the case that catches a column-remapping inversion.
- `PinotTableStatisticTest` — the confidence gate, and that statistics are
fetched **at most once**
per table (a call-count assertion, verified to fail when the memo is
bypassed).
- `PinotRelMdRowCountTest` — the row count reaching `RelMetadataQuery`, and
that a no-op provider
leaves it unknown so Calcite supplies its own heuristic.
- `BrokerStatisticsProviderTest` — the adapter, including that an absent
estimate stays absent
rather than becoming a confident zero.
- `MultiStageBrokerRequestHandlerTest` — that the provider actually reaches
`QueryEnvironment.Config`; every planner-side test builds a
`QueryEnvironment` directly, so
without this the wiring could be dropped with all of them still green.
- `BrokerStatsCollectionIntegrationTest` — starts a real broker with
statistics enabled and asserts
the planner estimates **exactly** the ingested row count, read from
`EXPLAIN ... INCLUDING ALL ATTRIBUTES`. Asserts a number rather than plan
shape, so it does not
become a plan-pinning test.
--
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]
