Repository navigation
[test](fe) Pin the partition values and null flags of external file ranges - #69001
Merged
Merged
Conversation
…anges ### What problem does this PR solve? Issue Number: None Related PR: apache#66012, apache#62821, apache#68124, apache#68786 Problem Summary: branch-4.1 apache#66012 made external scans and file loads send each file's partition values with explicit NULL flags. Master has the same behavior through apache#62821 and the connector plugins, but three parts of it have no test: - FilePartitionUtils.parseColumnsFromPathWithNullInfo lowercases a copy of the keys rather than the caller's list, sends the Hive default partition as "" with the null flag, keeps a literal \N as data, and skips the delta directory of an ACID layout. - An Iceberg range sends its identity partition values in spec order, with a NULL value through the null flag. - After the spec evolves from (Region, Dt) to (Dt, Category), a file of either spec sends the identity values of its own spec. Port the matching tests of apache#66012. Its other tests are not ported: master maps the Hive default partition in the connectors, keeps the union of identity columns across specs (apache#68124), and sends UUID and TIMESTAMPTZ values (apache#68786); the load-path handling of a literal \N needs a code change and is left to a separate change. ### Release note None ### Check List (For Author) - Test: Unit Test - BrokerUtilTest#parseColumnsFromPathPreservesNullMetadataAndInputKeys - IcebergScanPlanProviderTest#planScanSendsIdentityPartitionsInSpecOrderWithNullFlags - IcebergScanPlanProviderTest#planScanSendsEachFilesOwnIdentityPartitionsAfterSpecEvolution - BrokerUtilTest (2) and IcebergScanPlanProviderTest (187) pass. - Behavior changed: No - Does this need documentation: No Co-authored-by: Socrates <[email protected]> Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Contributor
Author
|
run buildall |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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.
Complete static review of PR #69001 at ed4c79c. One new P2 test-coverage finding is included inline: the caller-key preservation fixture cannot detect in-place lowercasing. No P0/P1 finding was identified, and no existing inline findings require carry-forward.
Critical checkpoint conclusions:
- Goal/proof: the three tests pin existing partition value/null and historical-spec metadata. Input-key preservation needs the inline correction; current production correctly copies the caller's keys.
- Scope/reuse: focused additions to two test files; existing planning/table/range helpers are reused, with a small typed PartitionData helper for genuine nulls.
- Concurrency/locking: no new threads, shared mutable state, locks or lock-order changes; fixtures are local to each test.
- Lifecycle/static initialization: in-memory metadata fixtures follow neighboring tests, and existing planning closes its iterables; no new static state or resource-lifetime issue found.
- Configuration: no runtime settings added; format-version=2 is confined to the evolution fixture and supports its two-field layout.
- Compatibility: production symbols, storage behavior and connector SPI are unchanged; no rolling-upgrade adaptation is needed.
- Thrift/protobuf: no definition or field-ID changes; tests inspect existing fields.
- Parallel paths: traced case-insensitive query parsing, both legacy/Nereids load modes, and Iceberg eager/streaming/count mapping. Shared production helpers and complementary existing tests cover these paths; no mirrored implementation change is missing.
- Conditions/errors: added tests propagate unexpected failures, and byPath fails if either expected file is absent. Existing malformed/missing/incomplete path cases remain covered.
- Coverage: inspected all changed hunks, parser negative cases, Iceberg null/evolution/splitting tests and both BE consumers. The new Iceberg fixtures catch wrong file-spec selection, extra union keys and lost/misaligned null flags. These unit tests do not establish end-to-end BE decoding coverage.
- Expected results: Hive sentinel -> empty string plus true; literal \N -> literal string plus false; ACID parsing skips the delta directory. Iceberg key/value/null lists match the current serializer. No golden result files are changed.
- Observability: no runtime path added, so additional logs or metrics are unnecessary.
- Transactions/persistence: Iceberg append/spec commits create fixtures only; Doris visibility, MoW, EditLog and failover logic are unchanged.
- Writes/atomicity/crashes: no production data-write or recovery behavior changes; fixture commits complete before planning.
- FE-to-BE variables: no new fields or send paths. Both BE scanners associate each existing partition value/null flag by its corresponding key and index.
- Performance/memory: only tiny local metadata fixtures and linear helper loops are added; no production hot-path overhead or significant tracked-allocation change.
- Other concerns: fixture isolation, null varargs, type/cardinality, historical specs, map ownership and ordering were rechecked. Production uses an all-spec key union filtered to each file; aligned ordering remains safe for keyed consumers. No additional actionable issue found. Release note None and no documentation change fit this test-only scope.
No additional user focus was supplied. Review completed after two rounds: both normal reviewers and the separate risk-focused reviewer returned NO_NEW_VALUABLE_FINDINGS against the unchanged final comment set. Every candidate was independently validated and accepted, deduplicated or dismissed with code evidence; the final changed-file/unresolved-candidate sweep is complete.
Validation was static only. No builds or tests were run and no repository source files were modified, as required by this invocation. Author-reported test runs were not treated as reviewer execution evidence.
| public void parseColumnsFromPathPreservesNullMetadataAndInputKeys() throws Exception { | ||
| // A case-insensitive parse lowercases its own copy of the keys, not the caller's list, and turns the Hive | ||
| // default partition into SQL NULL. | ||
| List<String> partitionKeys = Lists.newArrayList("region", "dt"); |
Contributor
There was a problem hiding this comment.
[P2] Use mixed-case keys to exercise input-list preservation
Both caller keys are already lowercase, so normalizing the caller's list in place (for example, with columnsFromPath.replaceAll(String::toLowerCase)) would still produce the expected values/null flags and pass the assertion that partitionKeys is unchanged. This leaves the input-key preservation invariant named by this test unprotected. Initialize partitionKeys with mixed-case names such as Region and Dt, then assert those original spellings after the case-insensitive parse so that this regression fails the test.
Contributor
Author
There was a problem hiding this comment.
Agreed, fixed in fe841c0. The caller now passes Region and Dt, the path spells them REGION and dt, and the test asserts the list still reads Region, Dt after the case-insensitive parse.
Checked against the regression you describe: with FilePartitionUtils lowercasing the caller's list in place (columnsFromPath.replaceAll(String::toLowerCase)), the new assertion fails (expected: <[Region, Dt]> but was: <[region, dt]>), while the old all-lowercase fixture passed under the same change.
### What problem does this PR solve? Issue Number: None Related PR: apache#69001 Problem Summary: parseColumnsFromPathPreservesNullMetadataAndInputKeys checks that a case-insensitive parse lowercases its own copy of the partition keys and leaves the caller's list alone. The caller's keys were already lowercase, so a parse that lowercased the caller's list in place would still have passed. The caller now passes Region and Dt, the path spells them REGION and dt, and the test asserts the list still reads Region, Dt after the parse. ### Release note None ### Check List (For Author) - Test: Unit Test - BrokerUtilTest passes. With FilePartitionUtils changed to lowercase the caller's list in place, the new assertion fails; the old fixture passed under the same change. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Contributor
TPC-H: Total hot run time: 28900 ms |
Contributor
TPC-DS: Total hot run time: 151840 ms |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
ClickBench: Total hot run time: 24.34 s |
Contributor
There was a problem hiding this comment.
Complete static review of PR #69001 at fe841c0. No new actionable finding was identified. Existing P2 comment 4255391430 is resolved: the parser test now supplies Region/Dt, parses REGION/dt, and asserts the caller retains its original spelling. No existing P0/P1 finding applies, so no blocking IDs are carried forward.
Critical checkpoint conclusions:
- Goal and proof: the three added tests correctly assert existing partition key/value/null metadata, per-file identity keys after spec evolution, and caller-key preservation. The mixed-case fixture now exposes in-place lowercasing. This conclusion comes from static tracing, not an executed test run.
- Scope and reuse: focused additions to two test files reuse the existing in-memory table, recording catalog, scan and Thrift-population helpers. The small PartitionData helper represents genuine null values directly.
- Concurrency and locking: no new threads, locks or lock-order changes. Fixture operations are sequential and catalog/table indexes are local to each fresh catalog.
- Lifecycle and initialization: no new static initialization dependency or production resource ownership. Existing scan planning closes its task iterables. Shared in-memory FileIO storage does not cause fixture collisions: Iceberg generates UUID metadata filenames, as confirmed in the pinned Iceberg 1.11.0 source.
- Configuration: no runtime setting added or dynamic-setting behavior changed. format-version=2 is confined to the evolution fixture; Iceberg 1.11.0's update implementation confirms that Region is removed and Category appended after Dt.
- Compatibility: production symbols, storage behavior and connector SPI are unchanged; no plugin API bump or rolling-upgrade adaptation is required.
- Thrift and protobuf: no definition or field-ID changes; tests inspect existing fields.
- Parallel paths: traced query parsing, legacy and Nereids file-load producers, and Iceberg eager/streaming range mapping. Shared range builders and adjacent streaming/spec-evolution tests support these paths; no corresponding implementation change is missing.
- Conditions and errors: unexpected exceptions fail the tests; byPath throws if either expected file is absent. Existing malformed, missing-key and incomplete-path cases remain covered. No silent error handling or speculative defensive branch is introduced.
- Coverage: inspected the full diff, fixture helpers, parser negative cases, adjacent Iceberg null/evolution/splitting tests and both BE consumers. Exact list assertions detect missing/extra per-file keys, cross-file values and lost or misaligned null flags. These metadata unit tests do not execute BE decoding or a full cluster scan.
- Expected results: Hive's default sentinel produces an empty placeholder with a true null flag; literal backslash-N remains data with a false flag; the ACID layout parses its partition value. Iceberg old/new tuples match the serializer. No golden result files change.
- Observability: no production path is added, so new logs or metrics are unnecessary.
- Transactions and persistence: only offline Iceberg fixture metadata commits are exercised. Doris visible versions, MoW, EditLog and failover behavior are unchanged.
- Writes and atomicity: no production data-write or crash-recovery behavior changes. Fixture append/spec commits complete before planning; data-file paths are metadata and are not remotely read.
- FE-to-BE variables: no transmitted variable is added. Both BE scanners associate each existing partition value/null flag with its corresponding key and list index.
- Performance and memory: small finite test fixtures and a linear two-field helper add no production hot-path overhead or significant BE allocation change.
- Other concerns: rechecked varargs nulls, type/arity, historical specs, map ownership, enumeration order and fixture isolation. Production orders an all-spec key union filtered to each file; aligned keyed consumers make that safe. Additional order permutations or an exclusive ACID branch discriminator would broaden coverage but do not establish an introduced defect. Release note None and no documentation change fit this test-only PR.
No additional user focus was supplied. Review completed after one round: the normal full-review agent and the separate risk-focused agent both returned NO_NEW_VALUABLE_FINDINGS against the unchanged empty final comment set. The main agent independently verified every recorded risk, read the ledger after each result, and completed an explicit sweep of both changed files and the unresolved-candidate list. No unresolved suspicious point remains. Recovery preserved this invocation's round count; the prior GitHub review was for an older head and was not resubmitted. Current PR reviews/comments and exact head/base were refreshed before submission.
Validation was static only. No builds or tests were run, and no repository source files were modified, as required by the review contract. Author-reported test runs were not treated as reviewer execution evidence.
Contributor
TPC-H: Total hot run time: 29270 ms |
Contributor
TPC-H: Total hot run time: 29328 ms |
Contributor
TPC-DS: Total hot run time: 152791 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
ClickBench: Total hot run time: 24.42 s |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
924060929
approved these changes
Oct 14, 2026
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
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: #66012, #62821, #68124, #68786
Problem Summary:
In short. This PR ports to master the tests of branch-4.1 #66012 whose behavior master already has, so that behavior is pinned. It changes no production code. #66012 made external scans and file loads send each file's partition values with explicit NULL flags; on master the same behavior arrived through #62821 and the connector plugins, but nothing tests three parts of it.
Background. A file scan range tells BE the partition values of its file in
columns_from_path_keys,columns_from_pathandcolumns_from_path_is_null, and BE fills the partition columns from them instead of reading them from the file. Two producers matter here:FilePartitionUtils.parseColumnsFromPathWithNullInfoparseskey=valuedirectories for Hive-style tables, TVFs and file loads. The Hive default partition (__HIVE_DEFAULT_PARTITION__) becomes SQL NULL through the null flag, and a literal\Nstays data.The problem, and what it cost. These are the parts of #66012's tests that hold on master and that no master test covers. Each of them decides what BE writes into a partition column:
__HIVE_DEFAULT_PARTITION__→""with the null flag; a literal\N→\Nwithout it; an ACID layout skips its delta directory\Nvalue reads as NULLHow this PR fixes it. It adds three tests:
BrokerUtilTest#parseColumnsFromPathPreservesNullMetadataAndInputKeys(branch-4.1: [fix](multi-catalog) Preserve external partition metadata #66012's test of the same name, callingFilePartitionUtils).IcebergScanPlanProviderTest#planScanSendsIdentityPartitionsInSpecOrderWithNullFlags(from branch-4.1: [fix](multi-catalog) Preserve external partition metadata #66012'sIcebergScanNodeTest#testSetPartitionValuesBuildsStableAlignedMetadata).IcebergScanPlanProviderTest#planScanSendsEachFilesOwnIdentityPartitionsAfterSpecEvolution(from#testSetPartitionValuesUsesPerSpecMetadataWithFileScannerV2; it also checks the old-spec file). The Iceberg tests plan a scan of an in-memory table, as the neighboring [fix](iceberg) keep identity partition values of old specs after evolving to unpartitioned #68124 test does.#66012 changes six more tests that are not ported, because master differs on purpose:
BrokerUtilTest#normalizeColumnsFromPathPreservesNullInfoHiveScanRangePartitionValuesTest), so fe-core's normalization only maps a Java nullIcebergUtilsTest#testGetCommonIdentityPartitionColumnsUsesSafeIntersection,IcebergScanNodeTest#testSetPartitionValuesKeepsCommonMetadataWithLegacyFileScannerIcebergUtilsTest#testMappedTypesAreExcludedFromPartitionMetadata,IcebergScanNodeTest#testSetPartitionValuesSkipsValuesUnsupportedByMappedTypesBrokerUtilTest#parseColumnsFromPathForLoadKeepsLegacyNullSemantics\Npath value as NULL, master stores the two characters\N. It needs a code change and comes in a separate PRResults. The three tests pass on master without any code change, and each catches a regression of what it pins:
FilePartitionUtilsstops flagging the Hive default partition as NULLparseColumnsFromPathPreservesNullMetadataAndInputKeysFilePartitionUtilslowercases the caller's key list in placeparseColumnsFromPathPreservesNullMetadataAndInputKeysIcebergScanRangesends a NULL identity value without the null flagplanScanSendsEachFilesOwnIdentityPartitionsAfterSpecEvolution(and the existingplanScanKeepsUnpartitionedSpecIdentityAfterEvolvingToPartitioned)Original author: @suxiaogang223
Release note
None
Check List (For Author)
Test
BrokerUtilTest(2) andIcebergScanPlanProviderTest(187) pass.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)