PHOENIX-7923 Simplify EXPLAIN value classes - #2530
Merged
Merged
Conversation
Co-authored-by: Claude Opus 4.8[1m] <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR simplifies the internal value/option handling for EXPLAIN by removing/flattening builder-style helper types and switching to simpler accumulation/deduping approaches, aiming to keep EXPLAIN text/JSON output unchanged.
Changes:
- Removes
ExplainOptions.Builderand updates the SQL parser to accumulate EXPLAIN options directly during parsing. - Simplifies region-boundary de-duping in
ExplainTable#getRegionLocationsForExplainPlanby removing theRegionBoundaryhelper type. - Refactors
ExplainPlanAttributesconstruction to use a single private builder-backed constructor.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| phoenix-core-client/src/main/java/org/apache/phoenix/parse/ExplainOptions.java | Removes the public nested builder type used previously to construct ExplainOptions. |
| phoenix-core-client/src/main/java/org/apache/phoenix/iterate/ExplainTable.java | Replaces RegionBoundary equality/hash logic with string-based boundary keys for deduping region locations. |
| phoenix-core-client/src/main/java/org/apache/phoenix/compile/ExplainPlanAttributes.java | Replaces the large all-args constructor path with a private builder-backed constructor and updates build(). |
| phoenix-core-client/src/main/antlr3/PhoenixSQL.g | Updates EXPLAIN option parsing to use an EnumSet-tracked accumulator and construct ExplainOptions directly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
Test ResultsUnit tests (
|
| Class | Tests | Failures | Errors | Skipped | Notes |
|---|---|---|---|---|---|
ExplainOptionsParserTest |
16 | 0 | 0 | 0 | Covers every EXPLAIN [(<opt>...)] form including (REGIONS, REGIONS) duplicate-rejection and the legacy WITH REGIONS alias — exercises the new EnumSet<ExplainOpt> seen path. |
ExplainJsonOutputTest |
2 | 0 | 0 | 0 | Covers EXPLAIN (REGIONS, FORMAT JSON) end-to-end JSON output — exercises getDefaultExplainPlan() (now seeded by new ExplainPlanAttributesBuilder().build()) and the regionLocations dedupe path. |
ExplainPlanTest |
105 | 0 | 0 | 0 | The connectionless EXPLAIN-grammar baseline suite. Asserts grammar/JSON shape for every operator class — exercises the new builder-driven private ExplainPlanAttributes ctor on every test. |
Other unit tests touching ExplainPlanAttributes / ExplainOptions (9 classes, 315 tests)
| Class | Tests | Failures | Errors | Skipped |
|---|---|---|---|---|
MergeSortResultIteratorTest |
2 | 0 | 0 | 0 |
ConcatResultIteratorTest |
2 | 0 | 0 | 0 |
JoinQueryCompilerTest |
2 | 0 | 0 | 0 |
StatementHintsCompilationTest |
4 | 0 | 0 | 0 |
QueryPlanTest |
4 | 0 | 0 | 0 |
TenantSpecificViewIndexCompileTest |
6 | 0 | 0 | 0 |
PhoenixSyncTableMapperTest |
28 | 0 | 0 | 0 |
QueryOptimizerTest |
52 | 0 | 0 | 1 |
QueryCompilerTest |
215 | 0 | 0 | 2 |
Integration tests (*IT)
Directly EXPLAIN-impacted ITs (2 classes, 37 tests)
| Class | Tests | Failures | Errors | Skipped | Time | Notes |
|---|---|---|---|---|---|---|
ExplainPlanWithStatsEnabledIT |
30 | 0 | 0 | 0 | 50.8 s | Asserts EXPLAIN text/JSON output across all stats-enabled call sites — primary regression guard for ExplainPlanAttributes + plan-total estimate cells. |
QueryLoggerIT |
7 | 0 | 0 | 0 | 47.4 s | Asserts the JDBC query-logger row matches EXPLAIN output, exercising the round-trip through ExplainPlanAttributes. |
Wider IT batch 1 — different EXPLAIN call sites (7 classes, 66 tests)
| Class | Tests | Failures | Errors | Skipped | Time |
|---|---|---|---|---|---|
ClientHashAggregateIT |
2 | 0 | 0 | 0 | 3.3 s |
ProjectArrayElemAfterHashJoinIT |
2 | 0 | 0 | 0 | 2.5 s |
DistinctPrefixFilterIT |
5 | 0 | 0 | 0 | 36.8 s |
UnionAllIT |
19 | 0 | 0 | 0 | 73.6 s |
EmptyColumnIT |
7 | 0 | 0 | 0 | 159.4 s |
TenantSpecificViewIndexIT |
11 | 0 | 0 | 0 | 19.0 s |
CostBasedDecisionIT |
20 | 0 | 0 | 0 | 160.5 s |
Wider IT batch 2 — joins, in-list, delete (4 classes, 370 tests)
| Class | Tests | Failures | Errors | Skipped | Time |
|---|---|---|---|---|---|
SortMergeJoinNoIndexIT |
35 | 0 | 0 | 0 | 13.1 s |
HashJoinNoIndexIT |
33 | 0 | 0 | 0 | 15.4 s |
InListIT |
228 | 0 | 0 | 0 | 145.1 s |
DeleteIT |
74 | 0 | 0 | 0 | 205.6 s |
apurtell
added a commit
to apurtell/phoenix
that referenced
this pull request
Jun 17, 2026
Co-authored-by: Claude Opus 4.8[1m] <noreply@anthropic.com>
asf-gitbox-commits
pushed a commit
that referenced
this pull request
Jul 24, 2026
Co-authored-by: Claude Opus 4.8[1m] <noreply@anthropic.com>
apurtell
added a commit
to apurtell/phoenix
that referenced
this pull request
Aug 4, 2026
Co-authored-by: Claude Opus 4.8[1m] <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Simplify EXPLAIN value classes.
Replaces
ExplainPlanAttributes' private zero args constructor and package private all args constructor with a private builder. RemovesExplainTable.RegionBoundaryin favor of dedupingHRegionLocations ingetRegionLocationsForExplainPlanvia aLinkedHashSet<String>. ReplacesExplainOptions.Builder'sregionsSet/verboseSetcompanion booleans with an inlineEnumSet<Option>in the grammar action. The nestedBuildertype goes away and the parser constructsExplainOptionsdirectly. No behavior change. EXPLAIN text and JSON output are unchanged.Co-authored-by: Claude Opus 4.8[1m] noreply@anthropic.com