Skip to content

branch-4.2 [fix](constraint) Fix NPE in PrimaryKeyConstraint when loading old metadata - #68206

Merged
yiguolei merged 2 commits into
apache:branch-4.2from
englefly:pick-61342-4.2
Sep 20, 2026
Merged

yiguolei merged 2 commits into
apache:branch-4.2from
englefly:pick-61342-4.2

Conversation

@englefly

Copy link
Copy Markdown
Contributor

Cherry-pick of #61342 to branch-4.2

fix Fix NPE in PrimaryKeyConstraint when loading old metadata

On branch-4.2 the class only has the foreignTables collection (the newer
foreignTableNameStrs/foreignTableInfos fields come from a later refactor that is
not part of this branch), so only that field is guarded. A unit test covering
deserialization of metadata without the ft field was added.

apupier and others added 2 commits September 18, 2026 14:09
macos-13 is no more available

https://github.blog/changelog/2025-09-19-github-actions-macos-13-runner-image-is-closing-down/

it is causing a lot of PR jobs to wait in queue for a long time when
there is a PR on 4.2 branch

it has been already upgraded on main branch

### What problem does this PR solve?

Issue Number: close #xxx

Related PR: #xxx

Problem Summary:

### Release note

None

### Check List (For Author)

- Test <!-- At least one of them must be included. -->
    - [ ] Regression test
    - [ ] 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?  -->

- Behavior changed:
    - [x] No.
    - [ ] Yes. <!-- Explain the behavior change -->

- Does this need documentation?
    - [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
apache/doris-website#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 -->

Signed-off-by: Aurélien Pupier <[email protected]>
…ding old metadata (apache#61342)

### What problem does this PR solve?

When FE starts with -r (metadata_failure_recovery) or without a
checkpoint image, it replays all journal logs from the beginning. Old
journal entries may contain PrimaryKeyConstraint objects that were
serialized before the `foreignTables` field was added to the class.

Gson uses Unsafe to instantiate objects during deserialization,
bypassing the constructor and its field initializers. If a field is
absent from the JSON, Gson leaves it as null rather than using the
declared initializer (e.g. `= new HashSet<>()`). Calling .isEmpty() or
any method on a null collection then throws a NullPointerException.

Fix: remove `final` from the `foreignTables` collection field (so it can
be reassigned in gsonPostProcess) and add a null-guard at the top of
gsonPostProcess() to initialize it to an empty collection when absent.

Note: on branch-4.2 the class only has `foreignTables`; the newer
`foreignTableNameStrs` / `foreignTableInfos` fields come from a later
refactor that is not part of this branch, so only that field is guarded.

Impact on -r mode: when FE is started with --metadata_failure_recovery
it replays every journal from journal #1, encountering the oldest
serialized PrimaryKeyConstraint objects. Without this fix, FE always
crashes during -r recovery if any PrimaryKeyConstraint was persisted
before the field existed.

### Release note

None
@englefly
englefly requested a review from yiguolei as a code owner September 18, 2026 15:35
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@englefly

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 100.00% (3/3) 🎉
Increment coverage report
Complete coverage report

@yiguolei
yiguolei merged commit bcc204e into apache:branch-4.2 Sep 20, 2026
28 of 30 checks passed
morningman added a commit that referenced this pull request Nov 5, 2026
…0163)

### 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]`.

Co-authored-by: minghong <[email protected]>
Co-authored-by: Jack <[email protected]>
Co-authored-by: feiniaofeiafei <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants