PHOENIX-7879 Tests for EXPLAIN text and ExplainPlanAttributes serialization compatibility#2495
Open
apurtell wants to merge 1 commit into
Open
PHOENIX-7879 Tests for EXPLAIN text and ExplainPlanAttributes serialization compatibility#2495apurtell wants to merge 1 commit into
apurtell wants to merge 1 commit into
Conversation
…zation compatibility Co-authored-by: Claude <noreply@anthropic.com>
Contributor
Author
|
Force pushed a commit message addendum adding Claude as co-author. |
There was a problem hiding this comment.
Pull request overview
Adds a backward-compatibility “golden” test harness to freeze Phoenix EXPLAIN output (both plan-step text and ExplainPlanAttributes JSON) so future grammar changes become explicit and reviewable, while also stabilizing JSON serialization for previously problematic attributes.
Changes:
- Introduce
ExplainOracle+ExplainChangeRuleinfrastructure to compare normalized current EXPLAIN output against embedded baselines and produce readable diffs on mismatch. - Add text/JSON normalizers to strip environment-specific noise (parallelism counts, row/byte estimates, region locations, lookup counts, etc.).
- Stabilize
ExplainPlanAttributesJSON output ordering and add custom Jackson serializers forserverMergeColumnsandregionLocations.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| phoenix-core/src/test/java/org/apache/phoenix/query/explain/ExplainTextNormalizer.java | Normalizes EXPLAIN text by eliding environment-specific details. |
| phoenix-core/src/test/java/org/apache/phoenix/query/explain/ExplainJsonNormalizer.java | Normalizes EXPLAIN JSON attributes by nulling/zeroing cluster-dependent fields and recursing into nested plans. |
| phoenix-core/src/test/java/org/apache/phoenix/query/explain/ExplainChangeRule.java | Provides rule hooks to transform “golden” expected text/JSON for intentional grammar changes. |
| phoenix-core/src/test/java/org/apache/phoenix/query/explain/ExplainOracle.java | Orchestrates normalization, rule application, and mismatch diff generation for EXPLAIN comparisons. |
| phoenix-core/src/test/java/org/apache/phoenix/query/explain/ExplainOracleTest.java | Corpus of compatibility tests plus serializer/order/normalizer sanity checks. |
| phoenix-core-client/src/main/java/org/apache/phoenix/compile/ServerMergeColumnsSerializer.java | Adds Jackson serialization for Set<PColumn> in plan attributes. |
| phoenix-core-client/src/main/java/org/apache/phoenix/compile/RegionLocationsListSerializer.java | Adds Jackson serialization for List<HRegionLocation> in plan attributes. |
| phoenix-core-client/src/main/java/org/apache/phoenix/compile/ExplainPlanAttributes.java | Freezes JSON property order and wires in the two custom serializers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+51
to
+56
| Collections.sort(names, (a, b) -> { | ||
| if (a == null && b == null) return 0; | ||
| if (a == null) return -1; | ||
| if (b == null) return 1; | ||
| return a.compareTo(b); | ||
| }); |
Comment on lines
+729
to
+737
| /** | ||
| * Convenience wrapper that builds {@link #defaultAttrs()} and sets the four fields every | ||
| * connection-backed scan emits via {@code ExplainTable.explain}: {@code iteratorTypeAndScanSize}, | ||
| * {@code consistency}, {@code explainScanType}, {@code tableName}, and {@code keyRanges}. | ||
| * @param scanType the {@code explainScanType} string (with its trailing space, e.g. | ||
| * {@code "FULL SCAN "}) | ||
| * @param table the {@code tableName} value | ||
| * @param keys the {@code keyRanges} string (may be {@code null} or empty) | ||
| */ |
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.
Add a backward compatibility test harness that freezes Phoenix's
EXPLAINoutput and makes any future change to the grammar an explicit, reviewable act.ExplainPlanAttributesgains a@JsonPropertyOrderannotation, andRegionLocationsListSerializerandServerMergeColumnsSerializerhandle the two attributes that aren't cleanly serializable.New test package
org.apache.phoenix.query.explainimplements compatibility checks.ExplainOraclecompiles each query against a connectionless Phoenix driver and compares both the textual and JSON representations of the plan against an expected baseline, producing line-by-line and JSON-pointer diffs on mismatch. Normalizers strip environment-specific noise before comparison.ExplainTextNormalizercollapses parallelism/chunk counts to , removes row/byte stats and region-location lines, andExplainJsonNormalizernulls out region locations, lookup counts, split chunks, and row/size estimates and recurses into nested join plans.ExplainChangeRulesupports future PRs that intentionally change the grammar. New rules can be appended that transform the baseline into its new expected shape. Every change is intentional and reviewable. The diff in the rule itself documents exactly what changed.The golden text in
ExplainOracleTestshould be periodically regenerated with theExplainChangeRulesdropped afterward. So,ExplainChangeRuleis clearly optional. The golden text can simply be updated. However this could be a good practice.