Name iterative optimizer trace spans

[electrum]

Use unique, descriptive names such as Iterative:PushProjectionIntoTableScan for iterative optimizer trace spans rather than the generic name IterativeOptimizer with a list of all the rule names. @nineinchnick found that the list of rule names is huge and can break trace collection systems. The single name is smaller, easier to understand, and directly correlates to the optimizer source code.

My understanding of the optimizer is limited, so the current list of names is the best guess from Codex, but looked reasonable to me. However, the actual name doesn't matter that much. The important thing is that when you are looking at a production trace with a long-running optimizer, the name lets you easily identify it.

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

[Copilot]

Pull request overview

This PR improves optimizer tracing by giving IterativeOptimizer instances explicit, concise names and recording those names in trace span attributes, removing the previously-emitted (potentially huge) list of rule names.

Changes:

• Add a required name to IterativeOptimizer (plus accessor) and plumb it through all construction sites.
• Update LogicalPlanner tracing to record iterative optimizers as Iterative:<name> and stop attaching rule-name arrays.
• Adjust affected tests to pass a stable name when constructing IterativeOptimizer.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
core/trino-main/src/main/java/io/trino/sql/planner/iterative/IterativeOptimizer.java	Adds a name field, updates constructors, and exposes getName() for tracing.
core/trino-main/src/main/java/io/trino/sql/planner/LogicalPlanner.java	Changes optimizer span attribute to use Iterative:<name> for iterative optimizers; removes rule-list emission.
core/trino-main/src/main/java/io/trino/tracing/TrinoAttributes.java	Removes the unused optimizer-rules attribute key.
core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java	Supplies descriptive names for all production IterativeOptimizer instances.
core/trino-main/src/test/java/io/trino/sql/planner/optimizations/TestSetFlattening.java	Passes an explicit IterativeOptimizer name in the test harness.
core/trino-main/src/test/java/io/trino/sql/planner/optimizations/TestReorderWindows.java	Passes an explicit IterativeOptimizer name in the test harness.
core/trino-main/src/test/java/io/trino/sql/planner/optimizations/TestMergeWindows.java	Passes an explicit IterativeOptimizer name in the test harness.
core/trino-main/src/test/java/io/trino/sql/planner/optimizations/TestEliminateSorts.java	Passes an explicit IterativeOptimizer name in the test harness.
core/trino-main/src/test/java/io/trino/sql/planner/iterative/TestIterativeOptimizer.java	Names iterative optimizers used in rule stats / timeout tests.
core/trino-main/src/test/java/io/trino/sql/planner/assertions/BasePlanTest.java	Names the iterative optimizer used in minimal-plan assertions.
core/trino-main/src/test/java/io/trino/sql/planner/TestCanonicalize.java	Names the iterative optimizer used in canonicalization test coverage.
core/trino-main/src/test/java/io/trino/sql/planner/TestAdaptivePlanner.java	Names iterative optimizers used in adaptive planner tests.
core/trino-main/src/test/java/io/trino/execution/TestPlannerWarnings.java	Names the iterative optimizer used in warning-related planning tests.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[losipiuk]

Are all the names unique - would be nice if they were

[electrum]

@losipiuk Good idea. I had that thought originally, but wasn't sure if it was necessary. I'll update this so that the shared/reused optimizers have a separate name per invocation.

[electrum]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

📝 Walkthrough

Walkthrough

This change introduces explicit naming to the IterativeOptimizer class by adding a required name field to its constructors and providing getName() and withName(String name) accessor methods. The PlanOptimizers class is updated to assign names to optimizer instances using the new .withName() method or constructor arguments. All test files that instantiate IterativeOptimizer directly are updated to provide name arguments. The tracing logic in LogicalPlanner is modified to emit the optimizer name via the new getName() method instead of the previously removed OPTIMIZER_RULES span attribute. The OPTIMIZER_RULES attribute key is removed from TrinoAttributes.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

core/trino-main/src/main/java/io/trino/sql/planner/iterative/IterativeOptimizer.java (1)

86-88: Reject blank optimizer names at construction time.

requireNonNull prevents null, but "" or whitespace would still produce an unhelpful Iterative: trace name and undermine the diagnostic goal of this PR.

Proposed guard

     public IterativeOptimizer(String name, PlannerContext plannerContext, RuleStatsRecorder stats, StatsCalculator statsCalculator, CostCalculator costCalculator, Predicate<Session> useLegacyRules, List<PlanOptimizer> legacyRules, Set<Rule<?>> newRules)
     {
         this.name = requireNonNull(name, "name is null");
+        checkArgument(!name.isBlank(), "name is blank");
         this.plannerContext = requireNonNull(plannerContext, "plannerContext is null");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/trino-main/src/main/java/io/trino/sql/planner/iterative/IterativeOptimizer.java`
around lines 86 - 88, The constructor IterativeOptimizer currently only checks
name for null; add a guard to reject empty or whitespace-only names by
validating that the provided name.trim() is not empty and throw an
IllegalArgumentException (or similar) with a clear message if it is; update the
IterativeOptimizer(String name, ...) constructor to perform this check before
assigning to the field so the instance.name cannot be blank and will produce a
meaningful trace label.

core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java (1)

362-399: Nit: the base names on the shared optimizer variables are never emitted.

inlineProjections ("InlineProjections", Line 363), simplifyOptimizer ("SimplifyExpressions", Line 386), and columnPruningOptimizer ("PruneUnreferencedOutputs", Line 394) — plus pushIntoTableScanOptimizer ("PushIntoTableScan", Line 675) and pushProjectionIntoTableScanOptimizer ("PushProjectionIntoTableScan", Line 688) — are only ever added to the optimizer list via .withName(...), so these initial names never appear in traces. Consider either:

• Using one of the call sites directly (without withName) when the generic name is fine, or
• Naming the base instances something like "InlineProjections.template" / leaving a comment that they're templates,

…so a future reader doesn't assume the base name is reachable. Purely a readability nit; behavior is correct.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java`
around lines 362 - 399, The optimizer instances inlineProjections,
simplifyOptimizer, columnPruningOptimizer (and similarly
pushIntoTableScanOptimizer / pushProjectionIntoTableScanOptimizer) are
constructed with base names that are never emitted because callers use
.withName(...); rename their base names to indicate they're templates (e.g.,
"InlineProjections.template", "SimplifyExpressions.template",
"PruneUnreferencedOutputs.template", "PushIntoTableScan.template",
"PushProjectionIntoTableScan.template") or add a brief comment above each
construction noting these are template instances whose visible name is set via
withName(...) so future readers won't expect the base name to appear in traces.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@core/trino-main/src/main/java/io/trino/sql/planner/iterative/IterativeOptimizer.java`:
- Around line 86-88: The constructor IterativeOptimizer currently only checks
name for null; add a guard to reject empty or whitespace-only names by
validating that the provided name.trim() is not empty and throw an
IllegalArgumentException (or similar) with a clear message if it is; update the
IterativeOptimizer(String name, ...) constructor to perform this check before
assigning to the field so the instance.name cannot be blank and will produce a
meaningful trace label.

In `@core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java`:
- Around line 362-399: The optimizer instances inlineProjections,
simplifyOptimizer, columnPruningOptimizer (and similarly
pushIntoTableScanOptimizer / pushProjectionIntoTableScanOptimizer) are
constructed with base names that are never emitted because callers use
.withName(...); rename their base names to indicate they're templates (e.g.,
"InlineProjections.template", "SimplifyExpressions.template",
"PruneUnreferencedOutputs.template", "PushIntoTableScan.template",
"PushProjectionIntoTableScan.template") or add a brief comment above each
construction noting these are template instances whose visible name is set via
withName(...) so future readers won't expect the base name to appear in traces.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3c0e4883-784d-4fe0-adbf-7006ff4015ab

📥 Commits

Reviewing files that changed from the base of the PR and between 95c08d4 and b5c14ce.

📒 Files selected for processing (13)

• core/trino-main/src/main/java/io/trino/sql/planner/LogicalPlanner.java
• core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java
• core/trino-main/src/main/java/io/trino/sql/planner/iterative/IterativeOptimizer.java
• core/trino-main/src/main/java/io/trino/tracing/TrinoAttributes.java
• core/trino-main/src/test/java/io/trino/execution/TestPlannerWarnings.java
• core/trino-main/src/test/java/io/trino/sql/planner/TestAdaptivePlanner.java
• core/trino-main/src/test/java/io/trino/sql/planner/TestCanonicalize.java
• core/trino-main/src/test/java/io/trino/sql/planner/assertions/BasePlanTest.java
• core/trino-main/src/test/java/io/trino/sql/planner/iterative/TestIterativeOptimizer.java
• core/trino-main/src/test/java/io/trino/sql/planner/optimizations/TestEliminateSorts.java
• core/trino-main/src/test/java/io/trino/sql/planner/optimizations/TestMergeWindows.java
• core/trino-main/src/test/java/io/trino/sql/planner/optimizations/TestReorderWindows.java
• core/trino-main/src/test/java/io/trino/sql/planner/optimizations/TestSetFlattening.java

💤 Files with no reviewable changes (1)

• core/trino-main/src/main/java/io/trino/tracing/TrinoAttributes.java