Repository navigation
[test](fe) Port the tests branch-4.2 added for three master fixes - #70163
Merged
Merged
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#68206, apache#68636, apache#68368 Problem Summary: Three branch-4.2 backports of master fixes added a test that master does not have: - apache#68206 (of apache#61342): a PrimaryKeyConstraint written before its foreign table collections existed lacks `ft`, `ftn` and `ftni`; Gson builds the object without field initializers, so gsonPostProcess fills them in. The new PrimaryKeyConstraintTest removes all three from a serialized constraint, loads it and uses it; without the null checks the load fails with a NullPointerException. - apache#68636 (of apache#57006): Index.getColumnUniqueIds compares an index column with the schema column name without the shadow prefix, so a schema change keeps the index column's unique id. IndexTest gets the 4.2 case, in JUnit 5; it fails without Column.removeNamePrefix. - apache#68368 (of apache#66354): shape_check/clickbench/query1 checks the plan of `SELECT COUNT(*) FROM hits`. Once SimpleAggCacheMgr has cached the exact row count of hits, REWRITE_SIMPLE_AGG_TO_CONSTANT turns the count into a constant and the shape into PhysicalOneRowRelation, so the case fails whenever that cache is warm (P0 build 1087320 of apache#69830). The case now disables the rule, as on branch-4.2; the expected output is unchanged. ### Release note None ### Check List (For Author) - Test: Unit Test / Regression test - FE UT: PrimaryKeyConstraintTest (new), IndexTest (one new case), ConstraintManagerTest and ConstraintPersistTest pass; each new case fails with its fix reverted. - Regression: on a local cluster with the row count cached, query1 fails as it is (PhysicalOneRowRelation) and passes with this change. - Behavior changed: No - Does this need documentation: No Co-authored-by: minghong <[email protected]> Co-authored-by: Jack <[email protected]> Co-authored-by: feiniaofeiafei <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No actionable findings in the three changed test files at e588737. Full-PR review and separate metadata/cache risk reviews all returned NO_NEW_VALUABLE_FINDINGS; main-agent verification and the final sweep found no unresolved source-level questions. There were no existing inline findings to retain and no additional user-provided focus.
Critical checkpoint conclusions:
- Goal and evidence: the PK fixture removes ft/ftn/ftni and reaches the same Doris Gson subtype/postprocess delegates as journal replay. Each missing-field repair is exercised because postprocessing dereferences all three collections. The index fixture checks shadow-prefix removal and case-insensitive matching; without stripping, [107] becomes empty. The ClickBench setting prevents the cache-dependent constant rewrite.
- Scope and clarity: changes are confined to two unit-test cases and one existing shape-test setting, with comments explaining the historical failure conditions.
- Concurrency and locking: unit fixtures are local, with no added threads or locks. The shape test disables the rule that consumes the asynchronous exact-count cache, removing that timing dependency without altering cache synchronization.
- Lifecycle and initialization: the Gson fixture exercises constructor-bypassed collection initialization. Foreign-table operations use local state and lowercase names; no running Env is required. SuiteContext creates and closes the JDBC session, so the SET stays scoped to the suite. No new static state or resource ownership is introduced.
- Configuration: no configuration definition is added. The existing session rule list parses both comma-separated names and is read by the following EXPLAIN.
- Compatibility: old missing-field JSON is exercised through the current reader; production symbols and storage formats are unchanged.
- Thrift/protobuf definitions: no gensrc changes or field-ID changes.
- Parallel paths: both agg/scan and agg/project/scan patterns use REWRITE_SIMPLE_AGG_TO_CONSTANT; visitor, stack and memo execution honor its disabled bit. The shared index-ID method feeds Thrift and cloud PB conversion, and the shadow fixture matches the schema-change caller. No parallel-path update is required for this test-only change.
- Conditional behavior: no new conditionals are added. Comments explain absent metadata fields and the exceptional shape-test rule setting. PRUNE_EMPTY_PARTITION remains disabled as before.
- Test coverage: JUnit Jupiter conventions and test discovery are consistent with the module. PK assertions cover primary columns, empty foreign-table state, add, rename and removal; the shadow-column assertion augments existing index matching/order/null/empty coverage. Existing planner tests retain coverage of the constant rewrite itself.
- Expected results: no .out files change. Unit expectations agree with the inspected implementations; the unchanged global/local aggregation shape is consistent with disabling the one-row rewrite. No test execution result is claimed by this review.
- Observability: no runtime feature or failure path is added, so no additional logs or metrics are needed.
- Transactions and persistence: journal deserialization uses the same Gson path under test. No EditLog write/replay, failover or transaction behavior changes.
- Writes, atomicity and crash behavior: changes perform only fixture mutations and EXPLAIN. Committed-data visibility, partition-version and delete-bitmap invariants are unaffected; no new production write or cleanup path is introduced.
- FE/BE propagation: no new variable, protocol field or sending path.
- Performance and memory: fixtures are small and bounded; no production hot path or memory-accounting change is introduced.
- Further recheck: immutable List.of inputs are copied or traversed read-only. The preexisting FilteredRules list construction does not bypass disablement because actual transforms independently check the forbidden bit. No additional PR defect was substantiated.
Review converged in one round with complete required static coverage and no unresolved candidates. Builds, tests, mutation experiments and Checkstyle were not run under the review-only contract; the author's execution claims were not independently reproduced.
Review converged after 1 round(s).
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
TPC-H: Total hot run time: 28725 ms |
Contributor
TPC-DS: Total hot run time: 151204 ms |
Contributor
ClickBench: Total hot run time: 24.12 s |
Contributor
Author
|
run nonConcurrent |
Gabriel39
approved these changes
Nov 5, 2026
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.
What problem does this PR solve?
Issue Number: None
Related PR: #68206, #68636, #68368
Problem Summary:
In short. Three branch-4.2 backports of master fixes each added a test: #68206 (of #61342), #68636 (of #57006) and #68368 (of #66354). Master has the three fixes but none of the tests. This PR ports them. Two guard a fix that has no test on master; the third keeps the ClickBench
query1shape case from failing whenever the exact row count ofhitsis cached. The PR changes tests only.Background, and what each test covers.
PrimaryKeyConstraintwritten before its foreign table collections existed has noft,ftnorftniin its JSON. Gson builds the object without its field initializers, so these stay null unlessgsonPostProcessfills them in, and an FE that replays the journal from the start (without an image, or withmetadata_failure_recovery) failed with a NullPointerException. [fix](constraint) Fix NPE in PrimaryKeyConstraint when loading old metadata #61342 added the null checks without a test; branch-4.2 [fix](constraint) Fix NPE in PrimaryKeyConstraint when loading old metadata #68206 added one on branch-4.2 forft, the only collection there. The newPrimaryKeyConstraintTestremoves all three from a serialized constraint, loads it, and adds, renames and removes a foreign table on it.__doris_shadow_<name>.Index.getColumnUniqueIdscompares the index's column names with the schema's afterColumn.removeNamePrefix, so the new tablet metadata keeps the index column's unique id. [fix](inverted index) remove shadow prefix when matching index columns in schema change #57006 has no test; branch-4.2: [fix](inverted index) Match shadow index columns during schema change (#57006) #68636 added one toIndexTest, ported here to JUnit 5.shape_check/clickbench/query1checks the plan ofSELECT COUNT(*) FROM hits. OnceSimpleAggCacheMgrhas cached the exact row count ofhits,REWRITE_SIMPLE_AGG_TO_CONSTANTturns the count into a constant and the shape intoPhysicalOneRowRelation, so the case fails whenever that cache is warm when it runs. It did on P0 for [refactor](coordinator) Unify query execution in Coordinator #69830 (build 1087320: expected--hashAgg[GLOBAL], real--PhysicalOneRowRelation), a PR that does not touch the planner. On branch-4.2, branch-4.2:[fix](cost model) scale partitioned scalar aggregate cost by BE number #66354 #68368 disables the rule in this case together with the cost model change it backports, which master has from [fix](cost model) scale group-by-less global agg cost by BE number when partitioned #66354. This PR disables it the same way; the expected output does not change.Results.
PrimaryKeyConstraintTest(new),IndexTest(one new case),ConstraintManagerTest,ConstraintPersistTestgsonPostProcesswithout its null checksPrimaryKeyConstraintTestgetColumnUniqueIdswithoutremoveNamePrefixIndexTest#testGetColumnUniqueIdsForShadowColumnexpected [107] but was []query1as on master, row count ofhitscached (local cluster)shape_check/clickbench/query1PhysicalOneRowRelationquery1with this PR, same cacheshape_check/clickbench/query1On the local cluster,
explain shape plan SELECT COUNT(*) FROM hitsgiveshashAgg[GLOBAL]the first time andPhysicalOneRowRelationthree seconds later, once the asynchronous row count load has finished; with the rule disabled it stayshashAgg[GLOBAL].Original authors: @englefly (#68206), @airborne12 (#68636), @feiniaofeiafei (#68368)
Release note
None
Check List (For Author)
Test
PrimaryKeyConstraintTest(new),IndexTest(3 cases, 1 new),ConstraintManagerTestandConstraintPersistTestpass, and fe-core Checkstyle reports nothing. The mutations in the table above were run against them.shape_check/clickbench/query1was run on a local cluster before and after this change, as in the table. This PR changes no production code.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)