Repository navigation
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of all supplied changed paths at 1322847. Eight new P2 findings cover incomplete semantic keys, cross-table inner-join expansion collisions, selective/zero input baselines, strict learned freshness, stale skew costing, and learned placeholder replacement. No existing inline findings were supplied. No additional user focus was specified.
Critical checkpoint conclusions:
- Goal and evidence: HBO identity, feedback reuse and manual management are implemented, but the inline findings prevent reliable reuse in the concrete plans described. Added tests cover many identity and injection cases; they do not establish automatic feedback reuse.
- Scope and reuse: the broad change is related to HBO; group descriptions and providers centralize the main lifecycle. Semantic fields and parallel consumers need consistent treatment.
- Concurrency: planner, profile, admin, config and loader threads share caches. Pinned mutation/load operations serialize on pinnedLoadLock, including internal SQL. Planning reads concurrent caches without acquiring that lock; no concrete deadlock was found.
- Lifecycle: per-query snapshots and cache expiry bound retained feedback. Loader restore is one-time and best effort per FE. The learned-placeholder replacement defect is described inline; no native/static initialization issue applies.
- Configuration: mutable ratio, guard and persistence settings are read on use; pinned cache capacity is explicitly startup-only. Strict learned-version behavior is defective as reported.
- Compatibility: no new FE-BE wire field or user-data format changes. Old learned keys cold-miss; internal pinned table columns are checked and old schemas explicitly require recreation.
- Parallel paths: filters, joins, aggregates, Java/Python UDF metadata and physical reconstruction were checked. Broadcast skew costing lacks the freshness fence used by row-count lookup. Mark-join semantics require a distinct key.
- Conditional checks: unsupported CTE consumers, table sampling, external feedback and physical-only partial stages are deliberately excluded; no duplicate local/final aggregate issue was verified.
- Tests: unit/golden and regression coverage was inspected, including negative command/guard cases. Real profile publication/reuse, restart loading and concurrent internal SQL lack end-to-end coverage. No builds or tests were run under this static-only contract.
- Test results: the added persistence output agrees with its injected values and singleton queries. Eight legacy suites are unchanged-content renames. Output-generation provenance was not independently executed.
- Observability: EXPLAIN applied/skipped annotations and persistence/load/snapshot logs support diagnosis; no separate logging defect was verified.
- Persistence and failover: pinned records use internal OLAP SQL, with checked execution errors, generation-bounded deletes and pending-load tombstones; cross-FE live refresh is explicitly unsupported. Restart/failover behavior remains unexecuted.
- Data writes and atomicity: no user-data write or transaction visibility/delete-bitmap path changes. Pinned memory operations are serialized and database persistence is declared best effort; no additional concrete atomicity bug was found.
- FE-BE propagation: existing runtime counters/statistics transport are reused; the counter-to-population mismatch is reported inline.
- Performance and memory: caches, scan-width gating and relation prefilters limit common-path work. Incorrect estimate reuse/misses and stale skew discounts can regress plans; no additional substantiated performance issue was found.
- Other review concerns: all candidates and initial risks have explicit dispositions in the shared ledger. Findings concern estimates, costing and feature behavior; no wrong-result claim is made.
Convergence is recorded separately after reviewer validation of the frozen findings. Validation limits above describe static review scope, not executed runtime results.
Review converged after 3 round(s).
| // arguments keep a different number of rows in another time zone | ||
| return className + '<' + encodeSlotComponent(sessionTimeZone()) + '>'; | ||
| } | ||
| return className; |
There was a problem hiding this comment.
[P2] Preserve semantic fields in normalized expressions
Aggregate(gb=CAST(t.s AS DECIMAL(10,0))) -> Scan(t) and the same plan with DECIMAL(10,1) both encode A{gb:Cast(col(t.s))}. For string values '1.1' and '1.2' these have one versus two groups. Cast.targetType is outside its children, so this fallback drops it in both literal modes; computeAggregate can then reuse the other query's learned or pinned count while identical scans pass freshness. The same omission drops OrderExpression direction: HAVING group_concat(v ORDER BY v ASC)='a,b' and DESC share an exact key even though only one group passes. Include cast targets and order direction/null ordering in expression identity, and cover these paired plans.
There was a problem hiding this comment.
不需要. 出现cast 绝大部分都是因为 两个decimal字段的 scale/precision不匹配.如果cast的type不同,说明参与计算的两个字段至少一个不同. 所以两个hbo条目本身就不可能匹配
| } | ||
| Map<String, String> tables = new TreeMap<>(); | ||
| Plan joinPlan = (Plan) join; | ||
| List<Plan> relations = joinPlan.collectToList(planNode -> planNode instanceof CatalogRelation); |
There was a problem hiding this comment.
[P2] Include the bound tables in inner-join expansion keys
FROM t1 a JOIN t2 b ON a.k=b.k and FROM t3 a JOIN t4 b ON a.k=b.k in the same database retain the same alias-qualified condition, even after alias-only projects are eliminated. During estimation this traversal finds no relations beneath GroupPlan leaves. In the concrete plan, bound slot qualifiers are internal.db.a/internal.db.b, but the lookup map only indexes bare aliases and relation.getQualifier() (catalog/database), so those bindings are empty there too. Both joins consequently get the same expansion fingerprint despite different tables and fan-outs; an expansion pinned for the first is applied to the second without a freshness guard. Resolve stable table identities through memo groups and index the full bound qualifiers consistently with EXPLAIN.
There was a problem hiding this comment.
Implemented in the new commit of this branch (hbo-structinfo-squashed):
- An expansion key now requires every operand of the join conditions — the equality pairs and the residual conditions — to be a direct slot of a catalog table, i.e.
SlotReference.getOriginalTable()andgetOriginalColumn()present. The operand identity isqualifiedNameOf(originalTable) + "." + originalColumn.getName(), read from the slot itself, so it is identical while the group is estimated (GroupPlanchildren) and while the chosen plan is printed, and it does not depend on the alias or on the qualifier shape (internal.db.a). - The key is the structural hash of: the join type, the sorted equality pairs rendered with those operand identities (
EqualTo(col(internal.db.t1.k),col(internal.db.t2.k))), the sorted residuals (literals kept) and the sortedoperands:[...]. Duplicate alias bindings, subtree traversal and relation lookups are gone. - A join whose operand is not a table column (cast, aggregate output, derived alias) has no key:
canonicalOf/fingerprintOfreturn empty, so EXPLAIN prints nojoin type=join_expansionline and no lookup can match it. That is the fail-closed behaviour you asked for instead of a colliding key. - An outer join additionally names the columns of each side (
sides:[...], read from the child output slots), soA LEFT JOIN BandB LEFT JOIN Astay distinct while remaining stable across the two call sites. Inner joins stay commutative. - Tests:
HboJoinConditionsTestnow pinst1 AS a JOIN t2 AS bvst3 AS a JOIN t4 AS b(different keys), a computed operand (no key), the outer-join side, the residual literals and that the key is not the sha256 of the canonical text. The expansion regression suite reads its fingerprint/struct from the EXPLAIN annotation and has not been re-run on a FE rebuilt from this commit yet; I will report the result once it is.
| // identity names the sub tree only, not how often the plan references it | ||
| out.count(child.mapping.size()); | ||
| for (Expression mapped : child.mapping) { | ||
| out.text(renderExpression(mapped, mode)); |
There was a problem hiding this comment.
[P2] Keep value-defining literals in set-operation mappings
These mappings define branch output values, so folding their literals can change distinct cardinality. For SELECT a AS x FROM t UNION SELECT a % N AS x FROM u, BuildAggForUnion produces Aggregate(gb=x) -> UnionAll(Project(t.a), Project(u.a % N)). With t={0} and u=0..999, N=2 yields two groups and N=1000 yields 1000, but NO_LITERAL encodes both mappings as Mod(col(u.a),lit(*)). The aggregate's learned key therefore collides, and unchanged scans with no predicates pass both matching and freshness. Preserve mapping literals in both modes, as grouping definitions already do; the existing computed-branch golden pair currently records this collision.
There was a problem hiding this comment.
不需要. NO_Literal 模式本身就意味着常熟部分的差异会带来行数误差, 注册NO_LITERAL hbo 就意味着用户认为可以忽略这个误差
| String table = HboScanDescriptor.qualifiedNameOf(scan.getTable()); | ||
| // the input rows of a scan node are the rows it read, i.e. the number a catalog row | ||
| // count (which also ignores the predicates) can be compared with | ||
| recordedRowsByTable.merge(table, input.getInputRows(), Long::sum); |
There was a problem hiding this comment.
[P2] Compare learned inputs against the same row population
Filter(k=1) -> Scan(t) on 100000 sorted distinct keys can physically read one row: storage key ranges prune the row bitmap before raw_rows_read/ScanRows are incremented. RuntimeProfile now records that counter, but the live descriptor contains all 100000 selected-partition rows. An immediate identical query therefore compares 1 with 100000, calls its fresh feedback stale, and cannot reuse the measured filter or parent join/aggregate count. Zero reads have the opposite failure: recorded == 0 is still skipped as unmeasured, so an absent-key result survives insertion of matching rows. Snapshot a comparable input-population baseline and distinguish missing measurements from measured zero.
There was a problem hiding this comment.
learned HOB 是老版本的功能, 且老版本里已经是不可用状态. 不在这个pr里处理
| continue; | ||
| } | ||
| compared = true; | ||
| String tableState = recorded == live[0] ? STATE_LIVE |
There was a problem hiding this comment.
[P2] Enforce strict version matching for learned entries
With hbo_row_count_change_ratio=0, this branch still declares equal input counts live without checking a recorded visible version. For Aggregate(gb=t.v) -> Scan(t), learn one group from 1000 identical values, then update those values to many distinct values. After compaction and row-count refresh the population can again be 1000 while the visible version has advanced. The new version-free key and unchanged scan predicates match, so applyLearnedStats applies the obsolete one-group estimate despite strict mode. ScanPlanStatistics retains a scan reference but no immutable version snapshot. Record the learned input versions and require equality in nonpositive-tolerance mode, as the pinned path does.
There was a problem hiding this comment.
learned hbo 的问题是老版本的遗留问题, 不在这个pr处理
| Optional<PlanNodeAndHash> planNodeAndHashOpt; | ||
| // the publish path keys a join with the constant agnostic form, and that is the | ||
| // form which the read side publishes for skew ratios as well | ||
| planNodeAndHashOpt = HboUtils.getHboPlanNodeAndHash(physicalHashJoin, |
There was a problem hiding this comment.
[P2] Check learned freshness before applying the skew discount
This parallel consumer now uses a version-free key but calls getMatchedPlanStatistics, which checks predicates/runtime-filter safety without learnedEntryFreshness. After a skewed join is learned, substantial uniformly distributed growth can exceed the input-row tolerance and eliminate that skew. HboStatsCalculator.applyLearnedStats rejects that run as stale, yet broadcast costing still reads its old skew ratio and multiplies probe/build work by 0.1, potentially choosing an inappropriate broadcast plan. Previously the embedded visible version prevented this hit. Retrieve the matched entry and validate its input freshness before using its skew measurements.
There was a problem hiding this comment.
learned hbo 的问题是老版本的遗留问题, 不在这个pr处理
| // wildcard that matches by fingerprint alone. A published entry always carries the rows of at | ||
| // least one scan (see InputTableStatisticsInfo#hasInputTables, the publish path drops entries | ||
| // which recorded none), so an empty input list can only be a manual injection | ||
| for (RecentRunsPlanStatisticsEntry injected : planStatistics.getRecentRunsStatistics()) { |
There was a problem hiding this comment.
[P2] Let actual feedback replace an injected learned placeholder
After HBO SET LEARNED STATISTICS for a join's NO_LITERAL key, the cache contains an empty-input wildcard. The first actual profile publication does not replace it: Profile.updatePlanStatistics cannot FULL_MATCH its zero inputs with the real nonempty input list and appends the measurement. Later identical executions replace only that measurement, leaving [wildcard, measured] below the default ten-entry limit indefinitely. This loop always returns the wildcard, so its arbitrary row count keeps overriding real feedback and bypassing drift checks. Remove the placeholder when actual same-key feedback arrives, or prefer a matching measured entry once one exists; otherwise LEARNED behaves like a persistent pinned override.
There was a problem hiding this comment.
手动注入learned条目的场景是测试场景,生产环境不会用手动注入learned 条目
| break; | ||
| case JOIN: | ||
| out.text(node.flattenable ? "inner" : String.valueOf(node.joinType)); | ||
| out.text(distinctIdentityConditions(node.conditions)); |
There was a problem hiding this comment.
[P2] Distinguish marking joins from filtering semi joins
Project(t.k, mark AS m) -> LEFT_SEMI_JOIN(mark=true, t.k=u.k) -> [Scan(t), Scan(u)] preserves every t row, while the same semi join without the marker filters unmatched rows. For nonnullable keys, projected t.k IN (SELECT k FROM u) produces the first plan; an ordinary semi join produces the second. With t={1,2}, u={1}, they output two versus one rows, but this key writes only join type and merged conditions, omitting isMarkJoin(). Both literal modes collide, and computeJoin can apply the other plan's learned/pinned count because their input scans are identical. Encode marking semantics in the join identity and cover both plans.
There was a problem hiding this comment.
GroupStructInfo 增加属性记录join是否是mark
There was a problem hiding this comment.
Fixed in the branch ([fix](fe) Include the mark-join flag in the join identity):
GroupStructInfo.Nodenow carriesmarkJoin(fromJoin.isMarkJoin()), the canonical writes it asJ{<type>,mark,c:[...]}and the structural key writes it as a field, soProject(mark AS m) -> LEFT_SEMI_JOIN(mark=true, ...)and the plainLEFT_SEMI_JOINover the same scans no longer share a key in either literal mode.- Two corpus cases pin the pair (
join_mark_semi/join_plain_semi) and a test asserts the keys differ in both modes and that the canonical writes the flag. - Note: because the flag is a field of the join identity, the fingerprints of all join cases moved once (the regenerated corpus shows that change explicitly).
- Tests:
org.apache.doris.nereids.stats.*+ the HBO command suite, 200 cases pass. The regression suites were not re-run against a FE rebuilt from this commit.
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 28773 ms |
TPC-DS: Total hot run time: 151139 ms |
ClickBench: Total hot run time: 24.31 s |
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed all 116 changed paths at head 5a2df43e76166bd3fed18aed527a1b748fcb3594 using the supplied aggregate diff. Five new findings: four P2 issues affecting outer-join identity, null-extension, learned-feedback association and persisted replacement revisions; one P3 issue affecting live cache configuration. These can misapply optimizer estimates or lose persisted statistics; no transaction-data visibility defect was established.
Critical checkpoint conclusions:
- Goal and proof: the structural-key, injection, feedback and persistence pipeline is present, but the inline counterexamples show it is not fully correct. Supplied tests were inspected statically.
- Scope and reuse: the broad planner/profile/command changes serve this feature and reuse existing providers/internal SQL; no unrelated source change identified.
- Concurrency and locking: FE-local SET/DELETE/load serialize consistently and internal planner reads avoid lock reentry. Cross-FE timestamp generations fail to protect a replacement (P2 inline).
- Lifecycle: daemon retry/enable/stop behavior, memo/cache ownership and postprocessor rebuild order checked. Parent/fragment/runtime-filter registration follows rebuilding; no separate lifecycle defect confirmed.
- Configuration: mutable ratios and persistence flag are read at use; the pinned bound is explicitly startup-only. The new relation-table cache misses the mutable size/expiry callback (P3 inline).
- Compatibility: structural keys replace legacy fingerprints; internal-table column checks reject an incompatible schema. Mixed-version/restart behavior was not executed.
- Parallel paths: logical/physical plans, both literal modes, cloud/non-cloud access, partial/storage aggregation exclusions and the separate skew consumer checked. Outer self-join and nullable-side definition paths have the inline identity defects.
- Conditional/error paths: type/scope/value/STRUCT/age validation, unavailable internal storage and documented unknown-baseline behavior checked. No additional unsupported-shape or error-propagation issue confirmed.
- Test coverage: focused unit, golden and positive/negative regression coverage is substantial; the five reported cases and true distributed replacement races lack adequate coverage. No builds or tests were run under the static-only review contract.
- Expected results: persistence output and generated golden expectations were checked against code. The captured cluster corpus is historical syntax/shape evidence, not execution against this head.
- Observability: EXPLAIN/SHOW expose keys, modes, baseline/guard/application states; load/persistence/snapshot failures include useful logging. No additional critical metrics gap identified.
- Persistence and failover: FE-local authority, one-time loading and best-effort SQL are explicit. They do not protect a newer cross-FE persisted row from timestamp-bounded deletion (P2 inline); restart/failover was not runtime-validated.
- Writes and atomicity: no transaction-row, MoW or version-visibility mechanism changed. Local cache/write ordering is serialized; the persisted-statistics replacement race remains.
- FE/BE variables: no new transmitted fields or function ABI requiring corresponding BE changes identified.
- Performance and memory: scan caps, cached descriptions, empty-cache/relation guards and administrative cache bounds checked. Retained relation-key supersets were treated as a documented optimization tradeoff without a substantiated material impact. No runtime performance claim is made.
- Other concerns/user focus: no additional user focus was supplied; all initial risks and candidates have explicit evidence-based dispositions in the review ledger.
Existing review context: the mark-join flag and different-table inner-expansion bindings are fixed in this head. Six existing P2 concerns about expression metadata, set mappings, learned freshness/populations, skew freshness and learned-wildcard precedence remain known context and were not reposted. No existing P0/P1 comment requires an ID in existing_blocking_comment_ids.
Coverage limits: static analysis only; no builds, tests, cluster runs or performance measurements. All changed paths and assigned risk questions were covered, with no unresolved review candidate remaining at delivery.
Convergence: true after 3 rounds. All three final-round reviewers returned NO_NEW_VALUABLE_FINDINGS on the identical five-comment set; required coverage and candidate/risk dispositions are complete.
Review converged after 3 round(s).
| if (!slot.getOriginalTable().isPresent() || !slot.getOriginalColumn().isPresent()) { | ||
| return Optional.empty(); | ||
| } | ||
| return Optional.of(HboScanDescriptor.qualifiedNameOf(slot.getOriginalTable().get()) + "." |
There was a problem hiding this comment.
[P2] Keep each equality operand bound to its outer-join input
LEFT_OUTER_JOIN(a.k=b.v, Scan(t a), Scan(t b)) and the same join on a.v=b.k, selecting all four columns, get identical expansion keys here. For t(k,v)={(1,1),(2,1)}, they produce three versus two rows. Both operands become the original-table names t.k/t.v, the equality sorts them, and sideOperands sees the same {t.k,t.v} on both sides. A factor of 1.5 injected for the first therefore estimates three rows for the second as well, with no binding/freshness guard. Preserve the input/occurrence of each operand before canonicalizing an outer self-join, and cover this reversed-column pair. This is distinct from the existing different-table INNER JOIN thread: both inputs here read the same table.
There was a problem hiding this comment.
Fixed in the branch ([fix](fe) Order the operands of an outer join expansion by their side):
- The operands of an expansion pair are now ordered by the side they are read from: the operand which belongs to
join.child(0)is written first (the side is resolved by the slot exprId against the child outputs, so it is the same while a group is estimated withGroupPlanchildren and while the chosen plan is printed), and the pair is not sorted for a join whose sides are not interchangeable. - Consequences:
t a LEFT OUTER JOIN t b ON a.k = b.vand the same join ona.v = b.know rendert.k = t.vandt.v = t.k(different keys), while the same pair written in either order (b.v = a.kagainsta.k = b.v) normalizes to one key. - An inner join keeps its operands sorted: the optimizer may swap its children and the count of an inner self join does not depend on which occurrence is called the left one.
- Tests:
HboJoinConditionsTestpins the reversed outer self join, the either-order pair and the inner self join (6 cases), andorg.apache.doris.nereids.stats.*+ the HBO command suite pass (201 cases). The expansion regression suite has not been re-run against a FE rebuilt from this commit.
There was a problem hiding this comment.
对不能交换顺序的join, join等式的左右需要和join孩子的左右保持一致
| } | ||
| Map<ExprId, Expression> definitions = new LinkedHashMap<>(); | ||
| for (Child child : node.children) { | ||
| definitions.putAll(child.node.columnDefinitions); |
There was a problem hiding this comment.
[P2] Preserve null-extension in inherited column definitions
Consider Aggregate(gb=x) -> LEFT_OUTER_JOIN(t.k=u.k) -> [Scan(t), Project(u.k,coalesce(u.v,0) AS x)->Scan(u)], versus computing that same x in a project above the join. With t.k={1,2} and u={(k=1,v=0)}, the first has groups {0,NULL} (two rows); the second has {0} (one row). This merge forwards the right-side x definition unchanged across null-extension, so applyColumnDefinitions rewrites both grouping keys to the same Coalesce expression. Transparent projects then give both plans identical keys in both literal modes, and identical scan baselines pass freshness, allowing the other plan's pinned/learned group count to apply. Retain the nullable-side evaluation boundary when propagating definitions (or reject unsupported definitions), and add the paired plans.
There was a problem hiding this comment.
null-extend 标签, 同时在null-extend 节点上添加编号
| // rows, and the drift comparison of the published entry expects the rows of every scan | ||
| PlanStatisticsWithInputInfo published = outputPlanStatisticsMap.get(publishedKey); | ||
| outputPlanStatisticsMap.put(publishedKey, new PlanStatisticsWithInputInfo( | ||
| published.getId(), published.getPlanStatistics(), |
There was a problem hiding this comment.
[P2] Keep each learned output count with the inputs that produced it
UnionAll(Filter(k<10, Scan(t)), Filter(k<1000, Scan(t))) over the same unaliased key table containing k=0..999 can publish two scans under the same S fingerprint. The filters are embedded into their scan operators. If the narrow scan is visited first, this retains its output=10, while InputTableStatisticsInfo.merge chooses the other scan's larger input=1000 together with its predicate and RelationId. On the next identical query, computeFilter's scan-key fallback matches that retained k<1000 predicate and freshness accepts 1000 against the table's 1000 rows, then applies the unrelated output=10 to the 1000-row branch. Keep output and corresponding inputs together, using separate recent-run entries for different predicate/input signatures. Unlike the existing ScanRows-population thread, this mismatched pair passes both matching and freshness.
There was a problem hiding this comment.
learned 条目不在本pr处理. learned hbo 要删掉重新实现
| String sql = "DELETE FROM " + FULL_QUALIFIED + " WHERE `fingerprint` = '" | ||
| + StatisticsUtil.escapeSQL(fingerprint) + "' AND `" + CREATE_TIME_COLUMN | ||
| + "` <= '" + createTimeLiteral(generation) + "'"; | ||
| StatisticsUtil.execUpdate(sql); |
There was a problem hiding this comment.
[P2] Use a write revision that distinguishes replacements across FEs
create_time comes from System.currentTimeMillis() before the FE-local lock, so it is not a unique generation across FEs. Two concurrent SETs for one fingerprint can capture the same millisecond; after B's INSERT replaces A's row, a conditional stale deletion on A still matches B's acknowledged replacement through this <= predicate. Clock skew can likewise give a later replacement a smaller timestamp, and tombstoneCovers will suppress that replacement during a pending load too. This violates the stated guarantee that a newer SET on another FE survives removal of the judged entry, even when every SQL operation succeeds. Persist a distinct revision/CAS token for the entry and target that revision when a known entry is removed, rather than ordering independent writes by local wall-clock time.
There was a problem hiding this comment.
修改del的顺序, 先删除table中的行, 成功再删除cache中的行. 去掉tombstoneCovers 机制
| Cache<String, Map<RelationId, Set<Expression>>> scanToFilterCache = | ||
| buildHboScanToFilterCache(cacheNum, expireSeconds); | ||
| scanToFilterCache.putAll(provider.scanToFilterCache.asMap()); | ||
| provider.scanToFilterCache = scanToFilterCache; |
There was a problem hiding this comment.
[P3] Rebuild the relation-table cache when its configuration changes
relationTableIdCache is constructed with hbo_plan_info_cache_num and expire_hbo_plan_info_cache_in_fe_second, but this callback never rebuilds it while updating every sibling cache. For example, starting with both limits disabled and then setting positive bounds through ADMIN leaves this new cache unbounded and non-expiring for subsequent filtered queries until FE restart; reducing a positive size/TTL likewise keeps the original limits. Both settings are mutable and invoke this callback. Rebuild the relation-to-table cache with the updated size/expiry and carry over its entries, as done for the adjacent scan-filter cache.
There was a problem hiding this comment.
这是learned hbo 专属的成员. learned hbo 后续pr要完全重做, 采用pinned 的处理方式替代 relatoinId的作用. 不在这个pr里实现
TPC-H: Total hot run time: 28272 ms |
TPC-DS: Total hot run time: 150859 ms |
ClickBench: Total hot run time: 24.23 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed PR #69322 at 25063e6 using the supplied merge-base diff. Two new P2 findings: cache-order-dependent null-extension fingerprints can defeat a copied pin, and INNER self-join expansion keys lose operand incidence across multiple equalities. Both are inline with concrete plan examples. No new P0/P1 finding; existing_blocking_comment_ids is empty because the supplied threads contain no P0/P1.
Critical checkpoint conclusions:
- Goal and proof: the structural-key, management, lookup, feedback, EXPLAIN and persistence routes are connected; the two new identity defects prevent the intended reuse guarantees. Unit and regression cases were inspected statically.
- Scope and clarity: all 116 changed paths were covered. The broad change serves HBO; eight legacy suites are pure renames. Physical-plan state-copy edits retain memo provenance.
- Concurrency and locking: admin sessions and the loader serialize stored/cache mutations with pinnedLoadLock; planner lookups do not acquire it. Internal SQL can hold that management lock during IO, but no concrete deadlock was established. Existing cross-FE replacement deletion remains reported in discussion_r4336321664.
- Lifecycle: startup loader retries and stops after its one-time load; local entries win over loaded rows. Cache bounds and expiry were traced. Memo warmup order is the new lifecycle defect in the first inline.
- Configuration: new live thresholds and persistence toggle are read on use; pinned-cache capacity is explicitly startup-only. The existing mutable relation-cache omission remains discussion_r4336321670.
- Compatibility: new persistence schema uses named columns and checks required columns; old schema/unavailable storage fails management operations and retries loading. No FE-BE wire schema or backend function symbol changed. Mixed-version execution was not run.
- Parallel paths: logical/physical filters, joins, aggregates, final versus partial/storage aggregation, costing, publication, commands, cloud/shared-nothing contexts and EXPLAIN were checked. Known learned/skew freshness concerns remain in their existing threads.
- Conditions and invariants: filter-small guard, expansion caps and preserved-side floors, finite/precision checks, selectors, unsupported inputs and literal modes were traced. The second inline identifies a missing multi-conjunct self-join identity invariant. Transaction visibility, BE memory ownership and delete-bitmap protocols are unchanged.
- Test coverage: focused JUnit and SQL cases cover normal and negative paths, but omit the two new scenarios; the race test covers only a sequential same-FE replacement. Actual restart, concurrent multi-FE and runtime behavior remain unvalidated under the static-only contract.
- Expected results: golden resources and the persistence .out were inspected; no additional definite mismatch was established. Generation and execution were not verified.
- Observability: EXPLAIN fingerprints, used/skipped states and expansion details plus SHOW baseline/state/detail expose matching; persistence/load logs include relevant identifiers. No separate actionable observability defect established.
- Persistence and failover: stored writes/deletes precede local cache mutation, and SQL errors propagate. Existing internal SQL/catalog mechanisms provide durability; one-time load provides restart recovery. Cross-FE judged-generation deletion is already reported, not reposted.
- Writes and crash atomicity: no user-table transaction protocol changed. Persist-before-cache SET and delete-before-cache invalidation leave recoverable durable outcomes on crash; FE-local cache/table state is not a cross-FE atomic update, as reflected in the existing deletion thread.
- FE-BE variables: runtime counters use existing profile names; the added settings govern FE planning. No new wire field requires additional send-path handling.
- Performance: empty-cache/relation gates, per-group memoization and scan limits reduce repeated key work. Those gates interact with boundary numbering as reported; no additional concrete performance regression established. Persistence IO stays off normal lookups.
- Final reconsideration: expression identity, nullable projections, mark joins, self joins, residuals, subpaths, shared memo children, input alignment, empty input, publication merging and alternate consumers were rechecked. Unreachable/synthetic candidates were dismissed with evidence; known issues were deduplicated.
Existing-thread disposition: the reported single-pair outer expansion and different-table binding examples now use side-aware/original-table bindings; the supplied mark-semi and coalesce examples receive markers. Related marked CROSS_JOIN/constant-definition omissions remain covered by the existing marking/null-extension threads. Existing expression metadata, folded set mappings, learned input/output pairing, input population/zero, strict versions, skew freshness, injected wildcard priority and relation-cache configuration concerns were not reposted. None is an existing P0/P1 finding.
User focus: no additional focus was supplied; the whole PR was reviewed. Static analysis only: no builds, tests, source changes or runtime validation. No outstanding concrete candidate or changed-path coverage gap remains within that scope.
Review convergence: reached after 2 review rounds. All three latest reviewers returned NO_NEW_VALUABLE_FINDINGS on the same frozen two-comment set; required static coverage and candidate/risk dispositions are complete.
Review converged after 2 round(s).
| node.joinType = join.getJoinType(); | ||
| node.markJoin = join.isMarkJoin(); | ||
| if (join.getJoinType().isOuterJoin()) { | ||
| node.nullExtensionBoundaryId = ctx.nextBoundaryId++; |
There was a problem hiding this comment.
[P2] Make null-extension IDs independent of memo cache order
Consider Aggregate(gb=x) -> LEFT_JOIN(s, LEFT_JOIN(t, Project(u, coalesce(u.v,0) AS x))). With no cached HBO entries, EXPLAIN describes the joins bottom-up in separate Ctxs, so the grouping definition becomes NullExtended<0>(NullExtended<0>(...)). Pin only the printed three-table aggregate key, then explain the identical query with HBO enabled and an empty learned cache: applyPinnedStats skips the two-table child through its relation-key prefilter, so the first three-table lookup builds the outer boundary as 0 and its cold inner boundary as 1. The grouping definition is now NullExtended<0>(NullExtended<1>(...)), changing both fingerprints and preventing the copied pin from matching. Cached children return without rebasing their IDs. Assign stable boundary identities when composing cached subtrees and cover the EXPLAIN-to-pin transition; this is separate from the already-reported missing null-extension marker.
There was a problem hiding this comment.
null-extension 使用 child-index 路径记录 null 生产者. 前面版本用 ctx 的 idGen 会产生不确定的编号
| private static String canonicalFirst(Join join, Expression conjunct, String left, String right) { | ||
| Integer leftSide = sideOf(join, conjunct.child(0)); | ||
| Integer rightSide = sideOf(join, conjunct.child(1)); | ||
| if (isCommutative(join.getJoinType()) || leftSide == null || rightSide == null |
There was a problem hiding this comment.
[P2] Preserve operand incidence across inner-join equality pairs
Sorting each pair independently loses which self-join occurrence supplies an operand. For t(k,v,w)={(1,1,1),(2,1,2)}, INNER_JOIN(a.k=b.v AND a.v=b.w, Scan(t a), Scan(t b)) returns one row, while the same join on a.v=b.k AND a.v=b.w returns two. Both keys contain the same sorted original-column pairs {t.k=t.v, t.v=t.w} and operand set, since INNER joins carry no side bindings. An expansion factor of 0.5 pinned for the first therefore estimates one row for the second too; this path has no structural/freshness check to separate them. Canonicalize one global side swap for the entire condition set, preserving shared-operand incidence, rather than swapping each equality separately. This is a distinct multi-conjunct INNER self-join case from the existing single-equality outer-join thread.
There was a problem hiding this comment.
因为同名表的 self join , inner join 也需要像 left join 那样调整 equal join 条件 的左右顺序.
|
/review |
### What problem does this solve?
Problem Summary: the key of an injected join expansion named the alias a query gave a relation
(`a.k = b.k`), so `t1 AS a JOIN t2 AS b` and `t3 AS a JOIN t4 AS b` shared one key although they read
other tables and expand differently; the attempt to add the alias to table bindings could not work,
because a group being estimated has `GroupPlan` children (no relation below them to inspect) and the
concrete plan qualifies the operands as `internal.db.a`, which the alias table did not index. The
expansion entry carries no data state to guard it, so the factor of one join was applied to the other.
An expansion key now requires every operand of the join conditions to be a direct slot of a catalog
table (`SlotReference.getOriginalTable()` and `getOriginalColumn()` present): the key names the column's
own table and name, so it is the same while a group is estimated and while the chosen plan is printed,
and it does not depend on aliases or on the shape of a qualifier. A join whose operands are computed
columns (a cast, an aggregate output, a derived alias) has no expansion key at all: EXPLAIN prints no
`join type=join_expansion` line for it and no lookup matches it, instead of sharing a key with another
table pair. An outer join also keeps the columns of each side, which is what distinguishes the
preserved side; an inner join stays commutative.
### Release note
A manually pinned join expansion entry is now bound to the tables its conditions read; a join whose
operands are not direct table columns can no longer carry an expansion entry.
### Check List (For Author)
- Test: Unit Test
- `HboJoinConditionsTest` pins the outer join side, the residual literals, the two joins over
different tables with the same aliases, the computed operand without a key and the structural
hash (5 cases pass); the regression suite which injects expansions reads its fingerprint and
struct from the EXPLAIN annotation, and has not been re-run against a FE rebuilt from this commit.
- Behavior changed: Yes - expansion keys are bound to the tables of the conditions; computed operands
are refused.
- Does this need documentation: No
### What problem does this solve?
Problem Summary: the identity of a join was its type and its conditions. A mark join, which implements a
non-nullable `IN` subquery as `Project(mark AS m) -> LEFT_SEMI_JOIN(mark=true, t.k = u.k)`, preserves
every row of its left side and produces a marker column, while the same semi join without the marker
drops the unmatched rows: with t = {1, 2} and u = {1} the first plan outputs two rows and the second
one, with the same join type, the same condition and the same input scans. Both plans rendered one key,
and since their inputs are identical the freshness comparison could not separate them either, so a
learned or pinned count measured on one was applied to the other.
The mark flag is now part of the join identity in both forms: the canonical string writes `J{<type>,mark,
c:[...]}` and the structural key writes the flag, so the two plans keep different keys in both literal
modes. Every join fingerprint moved once by this change (the flag is a field of the identity even when it
is false).
### Release note
A mark join and the plain semi join of the same columns no longer share an hbo statistics entry.
### Check List (For Author)
- Test: Unit Test
- Two corpus cases pin the mark semi join and the plain one over the same scans; a test asserts their
keys differ in both literal modes and that the canonical writes the flag. The regenerated file
gained those cases and moved the fingerprints of the join cases (the intended change), and 200
cases of the stats package plus the HBO command suite pass. The regression suites were not re-run
against a FE rebuilt from this commit.
- Behavior changed: Yes - the mark-join flag is part of a join key.
- Does this need documentation: No
### What problem does this solve?
Problem Summary: the operand identity of a join expansion is the column of a table, so in a self join
both sides of an equality resolve to the same table: `t a LEFT OUTER JOIN t b ON a.k = b.v` and the same
join on `a.v = b.k` both rendered `t.k = t.v` (the pair was sorted for commutativity), and the side
information (`sides:[...]`) is the same for both sides of a self join. The two plans keep a different
number of rows, and an expansion factor pinned for one was applied to the other without any data state
to guard it.
The operands of a pair are now ordered by the side they are read from: the operand of the left child is
written first, so `a.k = b.v` and `a.v = b.k` keep different keys, while the same pair written in either
order (`b.v = a.k` against `a.k = b.v`) keeps one. An inner join stays commutative: the optimizer may
swap its children and its count does not depend on which occurrence is called the left one, so its
operands remain sorted by their identity.
### Release note
An hbo join expansion entry of an outer self join is no longer applied to the join of the same table with
the columns reversed.
### Check List (For Author)
- Test: Unit Test
- `HboJoinConditionsTest` pins the outer self join with the columns reversed (different keys), the
same pair written in either order (one key) and the inner self join (one key); 201 cases of the
stats package plus the HBO command suite pass.
- Behavior changed: Yes - the operand order of an outer join expansion pair is the side it is read from.
- Does this need documentation: No
…oundary id
### What problem does this solve?
Problem Summary: the column definitions of a transparent project are inlined into the node above it, but a
side of an outer join is NULL for the rows the join does not match: a definition computed there which can
turn NULL into a non-NULL value (a coalesce, an `is null`, a udf, ...) denotes a different value above the
join. `Aggregate(gb = x)` over `LEFT_OUTER_JOIN(t.k = u.k)` with `coalesce(u.v, 0) AS x` computed on the
right side and the same expression computed above the join rendered one `A{gb:Coalesce(...)}` although the
first returns the groups `{0, NULL}` and the second `{0}`; a pinned or learned count of one applied to the
other on unchanged scans.
Every description now numbers the null extensions it contains in build order. A definition which crosses a
null-extended side is wrapped with the id of that boundary (`NullExtended<0>(Coalesce(...))`) when the
expression can turn NULL into a non-NULL value (the classification the outer join push down already uses,
extended over the sub tree, with a udf treated conservatively). The id is assigned inside one description,
which is memoized on the group, so the estimation path and the EXPLAIN path render the same marker. A
definition which cannot change its value (arithmetic, a cast, a plain column) is not wrapped, so the keys
of those plans do not move.
### Release note
A pinned or learned hbo statistics entry of a plan whose value is computed on the nullable side of an outer
join is no longer applied to the plan which computes it above the join.
### Check List (For Author)
- Test: Unit Test
- Two corpus cases pin the pair (below the join against above it) and their keys differ in the marker;
201 cases of the stats package plus the HBO command suite pass. The regression suites were not
re-run against a FE rebuilt from this commit.
- Behavior changed: Yes - a null-extended definition is marked with the id of the boundary it crossed.
- Does this need documentation: No
…d report its failure
### What problem does this solve?
Problem Summary: a deletion removed the in-memory pinned entry first and the stored row second; when
the stored deletion failed the statement still succeeded, and the failure was compensated with a
tombstone that the lazy load replayed: the tombstone had to carry a generation (the create time of the
row it judged) to avoid deleting a row another FE wrote afterwards, and two FEs can capture the same
millisecond or disagree on the clock, so a newly acknowledged SET could be removed by the replayed
deletion.
The stored row is now removed first and the in-memory entry only afterwards: when the stored deletion
fails the statement fails (`AnalysisException`) and the entry stays active, so an entry the load reads
later always describes an entry the user was told is active. The tombstone bookkeeping, its generation
and the replay at load time are removed; the load installs the stored rows as they are, which makes the
whole cross-FE generation/revision question moot for the deletion path.
### Release note
`HBO DELETE STATISTICS` now fails when the persisted entry cannot be removed from the internal table,
and the entry stays active in that case.
### Check List (For Author)
- Test: Unit Test
- `HboPinnedStatisticsRaceTest` keeps the case that a replacement is not removed and drops the
generation cases; `HboShowStatisticsCommandTest` and the input-statistics suite pass.
- Behavior changed: Yes - a failed persisted removal now fails the statement instead of being replayed.
- Does this need documentation: No
…anup going entry by entry
### What problem does this solve?
Problem Summary: a pinned SET put the entry into memory and then wrote the stored row as a best-effort
step, so a failed write left an entry which only existed in that FE and disappeared on the next restart
without the statement saying so. A stale cleanup removed the entries one by one and the stored removal of
one of them could now abort the whole statement (its in-memory entry removed first in the previous
ordering, its row left behind).
A pinned SET writes the stored row first and, when that fails, fails the statement and leaves the memory
untouched - the same order the deletion takes, so memory and table cannot disagree about an entry the
user was told about. `HBO DELETE STALE STATISTICS` keeps going when one entry's stored row cannot be
removed: that entry stays active and is judged again by the next run, and the statement reports how many
entries it removed and how many it kept for that reason.
### Release note
`HBO SET STATISTICS` now fails when the entry cannot be persisted to the internal table, and
`HBO DELETE STALE STATISTICS` no longer aborts on one entry whose stored row cannot be removed.
### Check List (For Author)
- Test: Unit Test
- `HboPinnedStatisticsRaceTest`, `HboShowStatisticsCommandTest` and the input-statistics suite pass
(the persistence path is disabled in these tests, the statement-level behaviour is exercised by
the hbo regression suites which run with the internal table enabled).
- Behavior changed: Yes - a failed persisted SET fails the statement; a stale cleanup skips such an entry.
- Does this need documentation: No
…build counter
### What problem does this solve?
Problem Summary: the boundary a definition crossed was numbered by a counter of the description being
built (`Ctx.nextBoundaryId++`). Each description is memoized per group and may be built as the root of its
own context, so the number a boundary got depended on which group was described first and on whether a
child description was already cached: describing the two-table join of a nested pair on its own numbered
it 0, and composing it later into the three-table description left it 0 while the outer boundary also got
0; when the prefilter of a pinned entry skipped the child, both boundaries were numbered inside one
context instead and became 0 and 1. The same plan therefore rendered `NullExtended<0>(NullExtended<0>(...))`
or `NullExtended<0>(NullExtended<1>(...))` depending on the cache, and a fingerprint copied from EXPLAIN no
longer matched.
A marker now carries the child indices from the boundary to the consumer instead of a number: the boundary
itself writes no index, and a node with more than one child prepends the index of the child the definition
came from. The code is a property of the assembled tree - a cached child description keeps the text of its
own subtree and the parent only prepends its index - so it is the same whatever order the descriptions were
built in, and two boundaries can no longer share a marker.
### Release note
The hbo statistics key of a plan whose value is computed below an outer join no longer depends on the order
the memo describes its groups in.
### Check List (For Author)
- Test: Unit Test
- The regenerated corpus renders `NullExtended(...)` for the boundary of the pair case; the golden
suite (27 cases) passes. The stats package is covered by the gate of the next commit.
- Behavior changed: Yes - the null-extension marker is a child-index code, not a boundary id.
- Does this need documentation: No
…n expansion key
### What problem does this solve?
Problem Summary: the operands of every equality were sorted on their own, which lost the incidence of a
self join: `INNER_JOIN(a.k = b.v AND a.v = b.w, Scan(t a), Scan(t b))` and the same join on
`a.v = b.k AND a.v = b.w` both rendered `{t.k = t.v, t.v = t.w}` with the operand set `{t.k, t.v, t.w}`,
although with t(k,v,w) = {(1,1,1),(2,1,2)} the first keeps one row and the second two: the first equates
the two columns of a with two columns of b, the second equates a.v with both of them. An expansion factor
pinned for one was applied to the other, and this store has no data state to guard it.
The pairs of an inner join are now ordered by one global orientation of its two sides: every operand of
the left child is written first, and the lexicographically smaller of the two possible orientations (all
pairs as given, or all pairs flipped) is the key. Flipping the whole set keeps the join commutative, while
ordering the pairs on their own lost which operands share a column. An outer join preserves a side, so its
orientation is fixed (the operand of the left child first), and a pair whose operands come from the same
side is ordered by identity.
### Release note
An hbo join expansion entry of an inner self join with several equality conditions is no longer applied to
the join with another incidence of the same columns.
### Check List (For Author)
- Test: Unit Test
- `HboJoinConditionsTest` keeps its cases for the outer self join (the reversed columns differ), the
either-order pair and the inner self join (one key); 200 cases of the stats package plus the HBO
command suite pass. The multi-conjunct incidence case needs a three column scan fixture and is a
follow-up test.
- Behavior changed: Yes - the operand order of an inner join expansion pair is one global orientation.
- Does this need documentation: No
### What problem does this solve?
Problem Summary: an expansion key named the whole condition set, residuals included, so the factor of
`a.k = b.v` could not be used by `a.k = b.v AND a.v > b.w`, and the two conditions had to be kept apart
by a residual text which itself was not oriented with the equality pairs: `INNER(a.k=b.v AND a.v>b.w)` and
`INNER(a.v=b.k AND a.v>b.w)` rendered the same key although the residual touches one of the equality
operands in the first plan only.
A key now describes the equality conditions only: the factor is the fan out of the equi join, and the non
equality conditions keep their own selectivity on top of it. `JoinEstimation` takes an expansion factor
that scales the equality estimate *before* the residual conditions are applied, and the estimator applies
the injected factor that way, so one entry serves every query with the same equality pair whatever
residual it adds, and a selective residual still lowers the estimate. A join whose equality operands are
not direct table columns still has no key.
### Release note
An hbo join expansion entry is matched by the equality conditions of a join and scales their estimate; a
residual predicate is applied on top of it. The factor of an existing entry is relative to the equality
estimate now, so entries tuned with the previous meaning should be reviewed.
### Check List (For Author)
- Test: Unit Test
- `HboJoinConditionsTest` pins that a residual does not change the key; 200 cases of the stats package
plus the HBO command suite pass. The regression suite which injects factors reads its numbers from
the annotation and now asserts the reported base is at most the larger input; it has not been
re-run against a FE rebuilt from this commit.
- Behavior changed: Yes - the key of a join expansion entry is the equality pair, and the factor scales
the equality estimate instead of the larger input.
- Does this need documentation: Yes - the meaning of a JOIN_EXPANSION factor changed.
…ingerprint preimage
### What problem does this solve?
Problem Summary: the structural key of a group wrote the rendered text of its conditions, its branch
mappings and its header expressions as one length prefixed string. Within that string the values of an
expression were still concatenated by the printed grammar (`lit(` ... `,` ... `:type`), so a value which
carried those characters could stand for a field boundary of its own: the array `['a','b']` and the
singleton array whose element is `a:TEXT),lit(b` produced the same preimage, although they match
different rows.
The fingerprint preimage now serializes expressions structurally as well: every expression writes its
kind, its value defining fields (a literal's type and value, a slot's qualifier and name) and its
children, each with its own length, and the conditions are sorted and de-duplicated over those bytes.
A composite literal keeps one field per element, so an element cannot forge the boundary of another.
The display form (canonical, shape, EXPLAIN, the struct a user pastes) is unchanged; only the fingerprint
moves. A missing semantic field (a cast target, an order direction) is not addressed here and remains a
per-expression follow-up.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- The regenerated corpus changes only its fingerprint lines (the shapes, canonicals and scan tokens
are byte identical); 200 cases of the stats package plus the HBO command suite pass.
- Behavior changed: Yes - all fingerprints moved once (the display form did not).
- Does this need documentation: No
### What problem does this solve?
Problem Summary: the previous commit introduced the length prefixed expression key but the condition
collections of a filter, an aggregation and a join still wrote the rendered text of their expressions
(and the join case was formatted with the new mark-join flag, so the replacement of that site did not
apply). The array literal case was therefore still one key for `['a','b']` and for the singleton array
whose element is `a:TEXT),lit(b`: the elements of a composite literal were only separated by the printed
grammar again.
The three sites now write the structural expression key as well, which also restores the identity
suffixes the display name carries (a udf's scope and id, a match analyzer, a search dsl, a lambda's
arguments, the distinct flag), the variant sub path of a slot (`v['age']` is a different column) and the
sorting/de-duplication of the conditions over their bytes. A corpus case pins the two arrays apart.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- The new corpus case `filter_with_an_injected_delimiter_array_literal` asserts the two arrays keep
different fingerprints in both literal modes; 201 cases of the stats package plus the HBO command
and pinned-race suites pass.
- Behavior changed: Yes - the fingerprints of the condition sites moved once (the display form did not).
- Does this need documentation: No
…it reads
### What problem does this solve?
Problem Summary: the definition of a computed column kept only the expression, not the sub tree which
produces the rows it reads, so two plans whose definitions share the expression rendered one key. With
t(v) = {0,1,1}:
Aggregate(group by a.x) Aggregate(group by b.x)
Cross Cross
Project(v+1 AS a.x) -> TopN(v ASC, 2) -> Scan(t)
Project(v+1 AS b.x) -> TopN(v DESC, 2) -> Scan(t)
both definitions become `Add(col(t.v),lit(1))` and both grouping keys render the same though `a.x` has
two values and `b.x` one: the Cross (which names both branches) is the same in both plans, so nothing in
the key says which branch the aggregation groups by. An exact pin of the two group query therefore
applied to the one group query.
A computed definition which reads the output of a node that selects the rows (a top-n or a limit) is now
bound to that producer: the marker records the child indices from the producer to the consumer, which is
the same structural code the null extension marker uses. `NullExtended` becomes the general `Bound`, with
the null extension as the case where the crossing is gated on the definition being able to turn NULL into
a non NULL value; a row selecting boundary binds every definition. The code, a property of the assembled
tree, is the same whether the child was described fresh or reused from the memo.
### Release note
An hbo statistics entry of a plan whose computed column is bound to a top-n or a limit is no longer
applied to the plan computing the same expression over another such producer.
### Check List (For Author)
- Test: Unit Test
- The regenerated corpus renders `Bound(...)` for the null extension pair case; 201 cases of the stats
package plus the HBO command suite pass. A corpus case for the top-n pair needs the top-n fixture and
is a follow-up.
- Behavior changed: Yes - a computed column above a top-n or a limit is bound to that producer.
- Does this need documentation: No
…ural key
### What problem does this solve?
Problem Summary: the canonical renderer sorts the operands of a commutative expression, but the
structural key wrote the children of every expression in their written order. `NormalizeBinaryPredicatesRule`
and `SimplifyComparisonPredicate` only commute constant-left comparisons, so `Filter(t.a = t.b)` and
`Filter(t.b = t.a)` rendered one struct and hashed two: a pinned row count copied from one query was not
found by the query which only reverses the two columns.
The structural key now normalizes the operators the renderer already recognizes as order insensitive
(equality, null safe equality, and, or): each child is serialized on its own, the child keys are sorted by
their unsigned bytes, and the sorted keys are written length prefixed. The code does not read the rendered
text, so two operands whose text would tie still have a deterministic order. The corpus fingerprints of
plans whose written operand order differed from the sorted one move once.
### Release note
An hbo statistics entry of a plan with a commutative expression inside a filter or a join condition is no
longer sensitive to the written operand order.
### Check List (For Author)
- Test: Unit Test
- A test asserts `Filter(t.a = t.b)` and `Filter(t.b = t.a)` share the fingerprint in both literal
modes; the regenerated corpus (70 fingerprint lines) and 201 cases of the stats package plus the
HBO command suite pass. The regression suites were not re-run against a FE rebuilt from this commit.
- Behavior changed: Yes - the structural key sorts the children of a commutative expression.
- Does this need documentation: No
### What problem does this solve?
Problem Summary: a temporary partition is a short lived copy which a manual job replaces, and the queries
which read one are rare, so a key recorded for it is not worth the risk of a stale match; the partition
identity was also not part of the scan key in every path, which made two scans of one table differ in
places but match in others.
An olap scan whose selected partitions contain a temporary partition now refuses the description with the
reason `temporary partition of <table>`, like a tablesample does: no struct info, no fingerprint and
therefore no pinned or learned entry is generated for that sub tree, and nothing it feeds into can be
applied. The refusal is reported by EXPLAIN and by the `HBO SHOW` verdicts.
### Release note
An hbo statistics entry is no longer injected for a query which reads a temporary partition.
### Check List (For Author)
- Test: Unit Test
- 202 cases of the stats package and the HBO command suite pass. A case with a temporary partition
needs the olap table fixture of the catalog tests and is a follow-up.
- Behavior changed: Yes - a scan of a temporary partition is refused instead of being given a key.
- Does this need documentation: No
### What problem does this solve?
Problem Summary: `PhysicalStorageLayerAggregate` (the wrapper a count-on-index plan puts over the scan it
reads) was described as a node kind of its own, so the canonical string of the filter above it named the
scan (`F{...}(A{sl,op=COUNT_ON_MATCH}(S{t}))`) while the structural summary of that filter recognized
neither the wrapper nor its embedded relation: the wrapper is a catalog relation without children, so
`summaryOf` fell through both of its leaf branches and produced an empty summary. The pinned entry
lookup uses the summary as its necessary condition, so a pin copied from the EXPLAIN of that filter
registered the relation and was then rejected by the prefilter before its fingerprint was compared - the
entry was stored and never applied.
The hbo information of the wrapper is now the one of the relation it embeds: the description of the
group is the description of that scan, and the summary counts it as one scan of that table. The wrapper
itself is still not published or advertised (its translator delegates to the embedded relation and the
read side applies statistics at filter/join/aggregation nodes only), but the filter above it now has a
summary which matches its canonical string, so its pin can be found.
### Release note
A pinned hbo statistics entry of a filter above a count-on-index plan is applied again.
### Check List (For Author)
- Test: Unit Test
- The stats package, the golden corpus and the HBO command suite pass. A dedicated unit case which
builds a `PhysicalStorageLayerAggregate` still needs the physical-scan harness (no
`PlanConstructor` helper for it yet) and is left for a follow-up.
- Behavior changed: Yes - the canonical string of a group whose node is the storage wrapper is the scan
it reads, and the fingerprints of those groups moved once.
- Does this need documentation: No
### What problem does this solve?
Problem Summary: five fields of the structural key were written in a form which cannot be read back, so
two structures the key is supposed to separate collided (or an equivalent one was split):
1. A slot's qualifier was joined with `.` (`[cat,x.y,z]` and `[cat,x,y.z]` both became `cat.x.y.z`, two
groups against one for a grouping over such a slot).
2. A variant sub path was joined with NUL, so the base column (`[]`) and the empty key (`[""]`) both
became the empty string (`IsNull(v)` keeps one row of t.v={SQL NULL,{"":1},{"a":2}}, `IsNull(v[''])`
keeps two).
3. A string literal's type (VARCHAR(4) for '%x_%', VARCHAR(7) for '%long_%') was written even in the
NO_LITERAL key, so the constant agnostic key of two patterns of different length still differed.
4. The operand of an expansion pair was written raw, so a quoted column name containing `),col(` could
forge the boundary between two operands.
5. The nested function of `ForEachCombinator` is a field, not a child, so `min_foreach` and `max_foreach`
had the same identity although they keep different groups.
A qualifier and a sub path are now counted sequences of length prefixed components; a string literal folds
its type to VARCHAR in the NO_LITERAL key; an operand escapes `%`, `,` and the parentheses before the pair
text is assembled; and a for each combinator writes the name of its nested function (`min_foreach`).
### Release note
Hbo statistics keys distinguish quoted identifier components, variant sub paths, string patterns of
different length and aggregate combinators of different nested functions.
### Check List (For Author)
- Test: Unit Test
- The regenerated corpus (142 fingerprint lines) and 202 cases of the stats package plus the HBO
command suite pass. The paired fingerprint fixtures for the qualifier and variant cases need file
scan and variant slot fixtures and are follow-ups.
- Behavior changed: Yes - the five fields above are encoded without ambiguity.
- Does this need documentation: No
…ax_foreach The two combinators are one class with a nested aggregate function field, so the key has to write the nested function name; the paired filter over the aggregate now asserts the fingerprints differ in both literal modes.
### What problem does this solve?
Problem Summary: the description refused a `LogicalOlapScan` with a table sample, but a storage layer
aggregate embeds a `PhysicalOlapScan`, and a sampled physical scan passed the check: the sample reads a
random subset, different on every execution, so a row count recorded under it cannot be reused - and the
storage aggregate is described as the scan it embeds, so it inherited the sample. A physical file scan
with a sample was likewise accepted, because the check also only looked at the logical file scan.
A table sample now invalidates the description for both scan classes, and a sampled `PhysicalFileScan` is
treated like a snapshot or scan params (unsupported). The reason is reported as
`tablesample of <table>`, so no struct info, fingerprint, pinned or learned entry is generated for such a
sub tree.
### Release note
An hbo statistics entry is no longer injected for a query which reads a table sample, whatever the scan
class or the operator which embeds it.
### Check List (For Author)
- Test: Unit Test
- 203 cases of the stats package (including the new paired combinator case) plus the HBO command suite
pass. A sampled physical scan case needs the olap scan fixture and is a follow-up.
- Behavior changed: Yes - a sampled physical scan (and a sampled physical file scan) is refused.
- Does this need documentation: No
… its columns
### What problem does this solve?
Problem Summary: the key of an outer join expansion appended the columns each side produces
(`sides:[t.k,t.v|u.k,u.w]`) to say which relation is on the preserved side. Column pruning pushes a
key-only project below the join and the project elimination passes keep it, so the same join on the same
equality read `sides:[t.k,t.v|u.k,u.w]` with `SELECT *` and `sides:[t.k|u.k]` for a key-only selection:
the equality population is identical, but a factor copied from one EXPLAIN missed the other.
An outer join now orders the operands of each equality by side - the operand of the left child is written
first - which names the preserved side without any output column, so column pruning cannot change the key
anymore. A pair which cannot name a side has no key: both operands from one child (the mirror plan applies
the condition inside the other child) and a pair whose operands are the same column (the two occurrences
are interchangeable in the key although only one of them is preserved) are refused, as the factor of such
a join would describe an orientation it cannot tell apart. The `sides` field, `sideOperands` and the output
column scan are gone.
### Release note
An hbo join expansion entry of an outer join is keyed by the join type and the side ordered equality
pairs: it no longer changes under column pruning, and an outer join whose equality pair reads one child
only has no key.
### Check List (For Author)
- Test: Unit Test
- Paired tests pin the side ordered keys, the two refusal cases, and that a narrow (key-only) project
below each child keeps the key; 205 cases of the stats package plus the HBO command suite pass.
- Behavior changed: Yes - the outer join expansion key loses its output columns and refuses a pair
without a side.
- Does this need documentation: No
…ead lock
### What problem does this solve?
Problem Summary: the freshness comparison of a pinned entry called `table.getIndexIdList()` to know
whether the table has a rollup, without the table read lock. The list is a filtered copy of OlapTable's
plain index hash map, which a concurrent drop of a materialized view rewrites under the write lock, so an
HBO SHOW or DELETE STALE could throw ConcurrentModificationException and abort.
The index count is captured under the table read lock, which is released before the visible version rpc.
### Release note
HBO SHOW / HBO DELETE STALE no longer fail when a rollup or materialized view is dropped concurrently.
### Check List (For Author)
- Test: Unit Test
- 205 cases of the stats package plus the HBO command suite pass; a coordinated reader/DDL case is a
follow-up.
- Behavior changed: No
- Does this need documentation: No
… not loaded
### What problem does this solve?
Problem Summary: after a restart the pinned entries are loaded from the internal table. When that load
fails (the backend or the cloud compute group is unavailable), `ensurePinnedLoaded` returned normally,
so the admin caller presented the empty or partial cache as the complete set: HBO SHOW reported no stored
entries and HBO DELETE STALE acknowledged success without inspecting or deleting the persisted stale ones,
which the loader then installed later.
`ensurePinnedLoaded` now reports whether the required load completed, and `getAllPinnedPlanStatistics`
(the admin entry point) raises an error while it has not; the background loader keeps its retry loop.
### Release note
HBO SHOW and HBO DELETE STALE report an error instead of an empty result while the pinned statistics of
the FE could not be loaded.
### Check List (For Author)
- Test: Unit Test
- 205 cases of the stats package plus the HBO command suite pass; a load failure case needs the
internal table fixture and is a follow-up.
- Behavior changed: Yes - an incomplete initial load is an error for the admin statements.
- Does this need documentation: No
### What problem does this solve?
Problem Summary: the pinned entries are persisted with `StatisticsUtil.escapeSQL`, which doubles a
backslash assuming the standard sql mode, but the internal session of `execUpdate` inherited the global
default: with `NO_BACKSLASH_ESCAPES` a table name `t\q` was stored with two literal backslashes while the
in-memory entry kept one. After a restart the doubled name was registered as the relation key of that row,
and the identical live query was rejected before the fingerprint lookup.
The internal session now clears `NO_BACKSLASH_ESCAPES`, so both the write and every other internal
statement read the escaped values the way `escapeSQL` produces them.
### Release note
A pinned hbo entry whose structure contains a backslash (a quoted identifier, for example) keeps working
after an FE restart with NO_BACKSLASH_ESCAPES in the global sql_mode.
### Check List (For Author)
- Test: Unit Test
- 205 cases of the stats package plus the HBO command suite pass; a restart case with a backslash
identifier needs the persistence fixture and is a follow-up.
- Behavior changed: Yes - the internal session sql mode no longer inherits NO_BACKSLASH_ESCAPES.
- Does this need documentation: No
…ensitive
### What problem does this solve?
Problem Summary: the expression identity appends the session time zone when a function's value depends on
it, but the detection is a list plus a date-type check: a function whose argument and result are both not
date like slips through. `from_unixtime` is BIGINT to VARCHAR, so `Filter(Like(from_unixtime(t.ts),
'1970-01-01 00:%'))` over `ts = 0..3599` keeps 3600 rows in +00:00 and none in +08:00, yet the two plans
rendered one identity and one fingerprint: a pin copied in one session applied in the other, and the
unchanged scan baseline passed freshness. `current_time` returns TIME, which is not date like either,
although the session zone decides which wall clock it reads.
Both names are now in the session dependent list. The other epoch and time functions were audited:
`hour/minute/second/microsecond_from_unixtime` and `unix_timestamp` were already listed;
`now`/`current_timestamp`/`localtime` return DATETIME, and the `*_second_timestamp` family takes or returns
a date like type, so the type check covers them; `utc_time`/`utc_timestamp` read UTC and do not depend on
the session zone; `sec_to_time`, `time_to_sec`, `time_format`, `make_time`, `time_diff`, `add_time` and
`sub_time` operate on a TIME value and never read the zone.
### Release note
An hbo statistics entry of a plan whose predicate formats an epoch (`from_unixtime`) or reads the wall
clock (`current_time`) is no longer shared between sessions in different time zones.
### Check List (For Author)
- Test: Unit Test
- A paired test pins that the same `from_unixtime` predicate has different fingerprints in +00:00 and
+08:00; 206 cases of the stats package plus the HBO command suite pass. The `current_time` pair needs
a TIME column fixture and is a follow-up.
- Behavior changed: Yes - two more functions carry the session time zone in their identity.
- Does this need documentation: No
… expression
### What problem does this solve?
Problem Summary: the session locale decides the day and month names a function emits, but the expression
identity only carried the session time zone. `Filter(dayname(t.d) = 'Monday')` over 1000 rows of
2024-01-01 and 1000 rows of 2024-01-02 keeps 1000 rows with lc_time_names='en_US' and none with 'de_DE',
yet both sessions rendered one identity and one fingerprint: the unchanged scan baseline passed freshness
and a pin copied in the English session overrode the German estimate.
`dayname` and `monthname` are now locale sensitive by name, and a format based conversion (`date_format`,
`from_unixtime`, `str_to_date`, `time_format`) is locale sensitive when its format argument names a month
or a weekday (`%M`, `%W`, `%a`, `%b`) or cannot be inspected because it is computed at run time; a call
without a format argument keeps its key. The session locale is appended to the identity next to the time
zone (`DayName<de_DE>`, `FromUnixtime<+08:00|en_US>`), read from the session which plans the query.
### Release note
An hbo statistics entry of a plan whose expression prints a day or month name is no longer shared between
sessions with different lc_time_names.
### Check List (For Author)
- Test: Unit Test
- A paired test pins that `dayname(d) = 'Monday'` has different fingerprints with en_US and de_DE;
207 cases of the stats package plus the HBO command suite pass. The pin reuse pair and the format
argument cases need their own fixtures and are follow-ups.
- Behavior changed: Yes - a locale sensitive function carries the session locale in its identity.
- Does this need documentation: No
0f40234 to
cda45a6
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of head cda45a6c16479290eeb6af40ef6d44afff9a2dca: two new P2 findings are included inline. They concern VARIANT field identity in join expansion and quoted-alias corruption of scan baselines. Existing discussions and replies were checked against all 95 live inline comments; known concerns were suppressed, including author-declined ones. No existing P0/P1 finding was present to carry forward.
The two complete reviewers and separate risk reviewer covered the authoritative changed-file list, related callers, tests and special paths. Each returned NO_NEW_VALUABLE_FINDINGS in the second round after the two accepted findings were independently verified. Every candidate and risk focus has an accepted, duplicate or evidence-based dismissal disposition; no separate question or assigned coverage gap remains. Convergence describes completion of the review process; the reported defects still need attention.
Critical checkpoint conclusions:
- Goal and proof: structural HBO keys, pinned/learned lookup, equality expansion, persistence and EXPLAIN/SHOW are wired end to end. The two new defects and previously reported limitations prevent treating the feature as fully correct. Test sources support nominal behavior; they were not executed here.
- Scope and clarity: the broad FE change is focused on HBO and preserving its identity through physical-plan rebuilds. All 117 diff sections, including eight unchanged legacy test moves, were covered.
- Concurrency: planner-local memo state, daemon loading, connection-thread administration, profile publication, config callbacks and cross-FE SQL writes were traced. The local load/write monitor does not block ordinary cache-only planning. Existing metadata-snapshot and cross-FE replacement concerns remain known; no additional lock cycle was established.
- Lifecycle: the loader retries initial failures, responds to persistence enablement, and exits after completion or disabled internal schema. Descriptors live with query memo groups; copied mutable-state holders are independent. Known relation-key retention remains covered by its existing thread.
- Configuration: row-drift and FILTER_SMALL thresholds are read at lookup; persistence is hot mutable with documented one-time loading; pinned cache size is startup-only. The existing relation-table-cache callback omission was suppressed.
- Compatibility: the structural key replaces the prior HBO identity. The internal SQL table validates required columns and refuses incompatible old schemas. No additional mixed-version protocol defect was established statically.
- Thrift/protobuf IDs: no gensrc/thrift or gensrc/proto definitions change, so the cross-branch field-ID checkpoint is not applicable.
- Parallel paths: logical/physical OLAP scans, file scans, storage wrappers, partial aggregation, exact/agnostic modes, pinned/learned precedence and the CostModel consumer were traced. Previously raised physical-tablet and learned-skew freshness issues were not reposted.
- Conditions and errors: unsupported shapes/selectors, invalid parameters, numeric exactness, missing structures, persistence failures and initial-load failures were checked. Existing stale-cleanup error acknowledgment and expansion early-return/selectivity concerns remain known.
- Test coverage: added unit, golden and regression sources cover nominal identity, modes, guards, drift, commands and persistence. The new findings lack the relevant field-pair and quoted-alias cases. Restart/failover and coordinated races were inspected through control flow only.
- Expected results: committed golden/captured data and the persistence .out were inspected. The known equality-only expansion assertion mismatch and row-report timing issue remain in existing threads. No expected output was generated or certified by execution.
- Observability: EXPLAIN applied/skipped markers, SHOW baseline/detail and persistence/loader logs expose the intended decisions. The alias finding makes SHOW disagree with query-side freshness; existing cleanup failure visibility remains known.
- Transactions, persistence and failover: ordinary internal-table SQL supplies storage without a new EditLog operation. Local SQL-before-cache ordering and putIfAbsent load precedence were checked. Existing cross-FE generation loss and DELETE visibility limitations remain covered by prior comments.
- Data writes and atomicity: writes concern HBO caches and its internal statistics table; user transaction commit/publish and delete-bitmap code are unchanged. No additional crash-induced leak, deadlock or user-data loss was established in the changed paths.
- FE/BE variables: runtime feedback uses existing fields/counters; hbo_max_scans_per_group is forwarded and fingerprint display stays in FE EXPLAIN. No new transmitted field or missing BE consumer was found.
- Performance and memory: scan-count/empty-cache/relation gates and memoized descriptors/renderings reduce key work; planning avoids internal SQL. Existing relation-index growth and cache-reconfiguration concerns were suppressed. No benchmark or runtime memory validation was performed.
- Other concerns and user focus: producer binding, volatile sharing, semantic expression fields, operand encoding and learned input alignment were rechecked against prior discussions. The tentative nested-namespace/dotted-column case was retained as evidence under the existing operand-encoding concern, not posted again. No additional user focus was supplied.
Validation was static only: no builds, engine/JVM tests, regression execution, benchmarks, source edits or submodule initialization. All 18,028 right-side lines in the authoritative hunks match the current checkout. Live PR base/head and the unchanged working tree were rechecked before delivery.
Review converged after 2 round(s).
| } | ||
| return Optional.of(escapeOperandComponent( | ||
| HboScanDescriptor.qualifiedNameOf(slot.getOriginalTable().get())) | ||
| + "." + escapeOperandComponent(slot.getOriginalColumn().get().getName())); |
There was a problem hiding this comment.
[P2] Preserve VARIANT subpaths in expansion operands
The default subpath rewrite turns these supported reduced plans into direct slot equalities:
INNER(l.v['a'] = r.v['a'], Scan(l), Scan(r))
INNER(l.v['b'] = r.v['b'], Scan(l), Scan(r))
The extracted slots retain original column v, so this return gives both joins the same JE fingerprint. Give each table {a:1,b:1},{a:1,b:2} and use supported ALTER TABLE ... MODIFY COLUMN v SET STATS ('row_count'='2','ndv'='2','num_nulls'='0'). StatsCalculator fetches those known statistics by base name v, so both equality estimates are two. A factor of two correctly gives the a join four rows, but applyPinnedJoinExpansion also applies it to the b join, which returns two; the Cartesian cap permits four. Include the counted subpath in the operand identity, or decline such operands. This is separate from the earlier GroupStructInfo subpath fix: this auxiliary expansion lookup still drops the entire path.
| // '#' introduces the partition / tablet / index selector markers of a scan token, so a table name | ||
| // which carries one would be truncated to the part before it while the planner summary keeps the | ||
| // whole name - the relation key of the entry would then never match the query | ||
| return name.replace("%", "%25").replace(".", "%2E").replace("#", "%23") |
There was a problem hiding this comment.
[P2] Escape commas in aliases before rendering scan metadata
For the full-table plan Filter(predicate) -> Scan(t AS alias) with quoted alias x,p1/2, the alias reaches this codec unchanged and produces a token such as S{internal.db.t#ti:ID#ra:x,p1/2,v2,r1000}. parse() splits on commas, truncates the alias to x, and invents a partial-partition baseline (selected=1,total=2); a full scan writes no real p field to replace it. Pin the advertised key/struct, then insert 10 rows and obtain comparable reported rows at v3/r1010. Query-side between/stateOf still applies the pin as DRIFTED, but catalog-side of() treats the false partial selection as UNKNOWN. Once the entry has positive age, HBO DELETE STALE STATISTICS OLDER_THAN 0 therefore removes this usable pin; an ordinary full-table alias remains DRIFTED and is retained. Escape the scan-token comma separator in alias components and cover the descriptor round trip and cleanup behavior.
History-based optimizer statistics for the nereids planner
What problem does this solve?
The nereids optimizer estimates a plan from catalog statistics alone, so a query whose data distribution
or predicate selectivity the catalog cannot describe (a skewed join key, a selective range, a filter
whose matching rows are known from previous executions) is estimated from heuristics.
This PR adds a history-based optimizer (HBO):
of the node kinds, the normalized expressions, the scan identities and the children. Two literal modes
are keyed: the exact one and the constant agnostic one which folds the literals of a predicate. The key
is the sha256 of the structural identity of that tree (length-prefixed fields), not of its printed
form, so a table name, an alias or a literal which carries a character of the printed grammar cannot
stand for another field. The scan token records the table id, the relation alias, the index and the
selected partitions - the constant agnostic key is then only applied to the same data state.
statistics it recorded (rows read per scan) and against the current data state (visible version, row
count, partitions), so a stale or drifted entry is not applied. A published entry carries the
predicates of the scans it measured, the grouping definitions and the join conditions, so two queries
which differ in a predicate the key folds cannot share a count.
HBO SET/SHOW/DELETE STATISTICS(pinned entries),HBO SET LEARNED STATISTICS(tests and experts),
hbo_persist_pinned_to_internal_db(pinned entries survive a restart, withgeneration-bounded deletions), and
SET show_hbo_fingerprintprints the key and the struct of everynode in EXPLAIN together with the verdict of the pinned and learned entries.
Release note
plan; pinned statistics can be managed with the
HBO ... STATISTICSstatements, persisted to__internal_schema, andEXPLAINcan print the plan key together with the statistics which wereapplied.
Check List (For Author)
org.apache.doris.nereids.statspackage - struct-info golden corpus (76 plan cases, 51text cases, regenerated and asserted), scan identity, freshness, learned guards, pinned race,
expansion keys, input-table merge - plus the HBO command suites: 200 cases pass.
query_p0/hboto its baseline (19 suites; the 8legacy/*suites fail because the environment does not support them). The injection, drift,learned, persistence and display suites pass.
of a node with
SET show_hbo_fingerprint=true.need a manual.
Note
This PR is the squashed version of #67549 (one commit per branch tip instead of the incremental history),
opened with the same tree so the review can be done on a single change.
Known limitation: the join identity is bound to the relation aliases
Two queries which differ only by the names of their aliases get different keys:
Their fingerprints differ (
a0424a5c…against97991fdf…on a local cluster), because a join conditionis rendered with the alias qualifier of its slots and every scan token carries the alias of its relation
(
#ra:). A pinned or learned entry is therefore alias sensitive: renaming an alias invalidates theentries of the plan, and they have to be pinned again.
That is deliberate, and it is the safe direction: an alias is the only occurrence identity a plan
provides. A table can appear more than once, and the occurrences play different roles, which the join
conditions alone cannot tell apart - normalizing the operands to their table and column is not enough,
even for an inner join:
An occurrence index derived from the tree would be stable for neither of the two call sites (a group is
estimated with
GroupPlanchildren, while EXPLAIN prints the chosen plan), so it cannot replace thealias either.
Follow-up (not part of this PR): make the key alias insensitive with a canonical labelling of the join
graph - instance nodes labelled by their table, edges labelled by the columns and the operator, refined
deterministically into a canonical numbering. Automorphic instances then share a label, which is
harmless for a row-count estimate (swapping them only transposes the join output), while instances with
different roles (the two queries above) are distinguished. Until then, the alias stays part of the
identity.