morningman opened a new pull request, #70163:
URL: https://github.com/apache/doris/pull/70163
### 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 `query1` shape case from
failing whenever the exact row count of `hits` is cached. The PR changes tests
only.
**Background, and what each test covers.**
- *Old primary key metadata (#61342, #68206).* A `PrimaryKeyConstraint`
written before its foreign table collections existed has no `ft`, `ftn` or
`ftni` in its JSON. Gson builds the object without its field initializers, so
these stay null unless `gsonPostProcess` fills them in, and an FE that replays
the journal from the start (without an image, or with
`metadata_failure_recovery`) failed with a NullPointerException. #61342 added
the null checks without a test; #68206 added one on branch-4.2 for `ft`, the
only collection there. The new `PrimaryKeyConstraintTest` removes all three
from a serialized constraint, loads it, and adds, renames and removes a foreign
table on it.
- *Shadow index columns (#57006, #68636).* During a schema change the new
schema names a changed column `__doris_shadow_<name>`.
`Index.getColumnUniqueIds` compares the index's column names with the schema's
after `Column.removeNamePrefix`, so the new tablet metadata keeps the index
column's unique id. #57006 has no test; #68636 added one to `IndexTest`, ported
here to JUnit 5.
- *ClickBench query1 shape (#66354, #68368).*
`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
when it runs. It did on P0 for #69830 (build 1087320: expected
`--hashAgg[GLOBAL]`, real `--PhysicalOneRowRelation`), a PR that does not touch
the planner. On branch-4.2, #68368 disables the rule in this case together with
the cost model change it backports, which master has from #66354. This PR
disables it the same way; the expected output does not change.
**Results.**
| Change | Test | Result |
|---|---|---|
| none | `PrimaryKeyConstraintTest` (new), `IndexTest` (one new case),
`ConstraintManagerTest`, `ConstraintPersistTest` | pass |
| `gsonPostProcess` without its null checks | `PrimaryKeyConstraintTest` |
fails: NullPointerException while loading |
| `getColumnUniqueIds` without `removeNamePrefix` |
`IndexTest#testGetColumnUniqueIdsForShadowColumn` | fails: `expected [107] but
was []` |
| `query1` as on master, row count of `hits` cached (local cluster) |
`shape_check/clickbench/query1` | fails: `PhysicalOneRowRelation` |
| `query1` with this PR, same cache | `shape_check/clickbench/query1` |
passes |
On the local cluster, `explain shape plan SELECT COUNT(*) FROM hits` gives
`hashAgg[GLOBAL]` the first time and `PhysicalOneRowRelation` three seconds
later, once the asynchronous row count load has finished; with the rule
disabled it stays `hashAgg[GLOBAL]`.
Original authors: @englefly (#68206), @airborne12 (#68636), @feiniaofeiafei
(#68368)
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [x] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
`PrimaryKeyConstraintTest` (new), `IndexTest` (3 cases, 1 new),
`ConstraintManagerTest` and `ConstraintPersistTest` pass, and fe-core
Checkstyle reports nothing. The mutations in the table above were run against
them. `shape_check/clickbench/query1` was run on a local cluster before and
after this change, as in the table. This PR changes no production code.
- Behavior changed:
- [x] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
