Skip to content

[fix](build) Unbreak master: stale unity-skip entry (BE) and dropped count probe (FE) - #66831

Merged
morningman merged 2 commits into
apache:masterfrom
morningman:fix-stale-unity-skip-entry
Aug 17, 2026
Merged

[fix](build) Unbreak master: stale unity-skip entry (BE) and dropped count probe (FE)#66831
morningman merged 2 commits into
apache:masterfrom
morningman:fix-stale-unity-skip-entry

Conversation

@morningman

@morningman morningman commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

What

Two independent breakages that each make current master fail to build — one in BE configure, one in FE compile.

They share a shape: a pair of PRs that never conflict textually, merge cleanly, and only break once combined, so each PR's own pipeline was green.

Breakage Colliding PRs
BE cmake configure aborts #66052 moved a file, #66789 made a dangling unity-skip entry fail loud
FE fe-connector-iceberg does not compile #66778 deleted getCountFromSnapshot(), #66413 added a caller for it

CI merges each PR into the latest master before building, so every PR pipeline that picks up current master is red on one or both.


1. BE — stale STORAGE_UNITY_SKIP entry for a moved file

Remove the stale STORAGE_UNITY_SKIP entry (and its comment block) for compaction/collection_statistics.cpp, which no longer exists.

Why — master configure is currently broken

#66052 moved storage/compaction/collection_statistics.{cpp,h} to storage/index/inverted/similarity/ (rewritten), but left behind the unity-skip entry that #66789 had added for the old path. The fail-loud validation introduced by #66789 turns a dangling skip entry into a configure-time error — which is exactly what it is designed to catch (a skip list rotting after a file move), so BE configure on current master fails immediately:

CMake Error at CMakeLists.txt:1002 (message):
  unity skip entry does not exist (renamed or moved?):
  .../be/src/storage/compaction/collection_statistics.cpp

#66826, #66824, #66819, #66820 were the first hits — same error on multiple independent agents.

Why deletion (not a path update) is correct

The old entry existed because the old collection_statistics_test #included the .cpp into a second TU (unity batching would then produce a duplicate definition at link time). The rewritten file at the new location is not #included by any test (grep -rn 'collection_statistics.cpp' be/test/ is empty on master), so the new path needs no skip entry.

Verification


2. FE — the metadata-only COUNT(*) probe calls a deleted method

#66778 replaced the snapshot-summary COUNT(*) pushdown with a manifest-derived count and deleted getCountFromSnapshot(). canServeMetadataOnlyCount(), added by #66413, still calls it, so FE compilation fails:

[ERROR] .../connector/iceberg/IcebergScanPlanProvider.java:[505,16] cannot find symbol
[ERROR]   symbol:   method getCountFromSnapshot(org.apache.iceberg.TableScan,org.apache.doris.connector.spi.ConnectorSession)
[ERROR]   location: class org.apache.doris.connector.iceberg.IcebergScanPlanProvider

Why re-express the probe instead of restoring the method

Bringing getCountFromSnapshot() back would reintroduce precisely what #66778 removed: a query result derived from optional, writer-provided snapshot summary fields.

The probe is rebuilt in #66778's own terms instead. It reuses that PR's delete gate and additionally requires the data manifests to carry aggregate row counters, so the answer is proved from the manifest list alone — O(manifests), no data-file enumeration, which is what a probe running before planning can afford.

Manifest lists that omit those aggregates now answer false, where count planning still serves them through its bounded per-file fallback. A capability probe should under-promise rather than over-promise; the planner itself is untouched.

Verification

  • mvn package over the full FE reactor (74 modules): all green. This also confirms no second semantic break is hiding behind the first — CI's maven stops at fe-connector-iceberg and never reaches the rest.
  • IcebergScanPlanProviderTest: 153 tests, 0 failures, including a new case pinning that the probe follows the same delete gate as count planning.
  • FE checkstyle: 0 violations.

Release note

None (both fixes only restore a buildable master; no user-visible behavior change).

Check List (For Author)

  • Test
    • Regression test — not applicable: neither change alters query behavior
    • Unit test: IcebergScanPlanProviderTest (153 tests, 0 failures)
    • Manual test: full BE configure/build and full FE reactor build
  • Behavior changed: No
  • Does this need documentation: No

…_statistics.cpp

apache#66052 moved storage/compaction/collection_statistics.{cpp,h} to
storage/index/inverted/similarity/ (rewritten) but left behind the
SKIP_UNITY_BUILD_INCLUSION entry that apache#66789 had added for the old
path. The fail-loud validation from apache#66789 turns the dangling entry
into a configure-time error, so BE configure on current master fails:

  CMake Error at CMakeLists.txt:1002 (message):
    unity skip entry does not exist (renamed or moved?):
    .../be/src/storage/compaction/collection_statistics.cpp

The CI pipelines merge each PR into the latest master before building,
so every PR pipeline that picked up master after apache#66052 landed is red
as well (apache#66826, apache#66824, apache#66819, apache#66820 were the first hits).

The old entry existed because the old test #included the .cpp into a
second TU; nothing #includes the rewritten file, so the new path needs
no skip entry. Remove the entry and its comment block.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ND7L1ZVTJf91TBpLwYSqct
@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?

@morningman

Copy link
Copy Markdown
Contributor Author

run buildall

airborne12
airborne12 previously approved these changes Aug 17, 2026

@airborne12 airborne12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions github-actions Bot added the approved Indicates a PR has been approved by one committer. label Aug 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@github-actions

Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

@morningman

Copy link
Copy Markdown
Contributor Author

run compile

@morningman

Copy link
Copy Markdown
Contributor Author

skip buildall

shuke987
shuke987 previously approved these changes Aug 17, 2026

@shuke987 shuke987 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

…ggregates

apache#66778 replaced the snapshot-summary COUNT(*) pushdown with a
manifest-derived count and deleted getCountFromSnapshot, but
canServeMetadataOnlyCount (added by apache#66413) still calls it. The two PRs
never conflict textually, so master merged both cleanly and FE stopped
compiling:

  IcebergScanPlanProvider.java:[505,16] cannot find symbol
    symbol: method getCountFromSnapshot(org.apache.iceberg.TableScan,
            org.apache.doris.connector.spi.ConnectorSession)

Every pipeline that merges current master hits it; apache master
35fea58 is still red.

Re-express the probe in apache#66778's terms instead of resurrecting the
deleted method: reuse its delete gate and additionally require the data
manifests to carry aggregate row counters, so the answer is proved from
the manifest list alone (O(manifests), no data-file enumeration) and
never from writer-provided snapshot summary fields. Manifest lists that
omit those aggregates now answer false, where count planning still
serves them through its bounded per-file fallback -- a capability probe
that runs before planning should under-promise rather than over-promise.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Gabriel39

Copy link
Copy Markdown
Contributor

I did not find a blocking correctness, memory-safety, or concurrency issue in the current patch. I have three review suggestions:

  1. Please update the PR title, description, and verification section to cover the second Iceberg commit. The current metadata describes only the unity-skip fix, while the PR now also changes FE COUNT(*) capability logic and tests. Please include the targeted Iceberg connector test and Checkstyle results for the final head.

  2. Could we extract the exact-count proof shared by canProveCountFromManifests and planCountPushdown into one helper that returns OptionalLong? Repeating the delete gate, position-delete handling, aggregate validation, and subtraction creates two sources of truth. If they drift, the capability could return true while actual planning falls back to a normal file scan, which is precisely the mismatch the pre-planning capability is meant to prevent.

  3. The documented complexity is not strictly O(manifests) when delete state is not NONE: livePositionDeleteRowCount opens delete manifests and iterates their live delete-file entries. Please document that cost accurately, and consider avoiding it in a pre-planning capability probe for delete-heavy tables.

@github-actions github-actions Bot removed the approved Indicates a PR has been approved by one committer. label Aug 17, 2026

@airborne12 airborne12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions github-actions Bot added the approved Indicates a PR has been approved by one committer. label Aug 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@morningman

Copy link
Copy Markdown
Contributor Author

skip buildall

@morningman morningman changed the title [fix](build) Drop the stale unity-skip entry for the moved collection_statistics.cpp [fix](build) Unbreak master: stale unity-skip entry (BE) and dropped count probe (FE) Aug 17, 2026
@morningman
morningman merged commit 168d077 into apache:master Aug 17, 2026
46 of 57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by one committer. reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants