morningman opened a new pull request, #69001:
URL: https://github.com/apache/doris/pull/69001
### 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_path` and
`columns_from_path_is_null`, and BE fills the partition columns from them
instead of reading them from the file. Two producers matter here:
- `FilePartitionUtils.parseColumnsFromPathWithNullInfo` parses `key=value`
directories 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 `\N` stays data.
- The Iceberg connector sends a file's identity partition values from the
file's own partition spec and partition data, not from its path. After
partition evolution, files of different specs carry different keys.
**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:
| Behavior | What a regression would do |
|---|---|
| A case-insensitive parse lowercases a copy of the keys, not the caller's
list | The caller's column list changes under it |
| `__HIVE_DEFAULT_PARTITION__` → `""` with the null flag; a literal `\N` →
`\N` without it; an ACID layout skips its delta directory | A NULL partition
reads as an empty string, or a `\N` value reads as NULL |
| An Iceberg range sends its identity values in spec order, a NULL value
through the null flag | Values land in the wrong columns, or a NULL partition
reads as an empty string |
| After the spec evolves from (Region, Dt) to (Dt, Category), a file of each
spec sends its own spec's keys | Files of one spec get the other spec's
columns, which BE then fills as NULL |
**How this PR fixes it.** It adds three tests:
- `BrokerUtilTest#parseColumnsFromPathPreservesNullMetadataAndInputKeys`
(#66012's test of the same name, calling `FilePartitionUtils`).
-
`IcebergScanPlanProviderTest#planScanSendsIdentityPartitionsInSpecOrderWithNullFlags`
(from #66012's
`IcebergScanNodeTest#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 #68124 test does.
#66012 changes six more tests that are not ported, because master differs on
purpose:
| branch-4.x test | Why it does not apply to master |
|---|---|
| `BrokerUtilTest#normalizeColumnsFromPathPreservesNullInfo` | The
connectors map the Hive default partition to NULL themselves
(`HiveScanRangePartitionValuesTest`), so fe-core's normalization only maps a
Java null |
|
`IcebergUtilsTest#testGetCommonIdentityPartitionColumnsUsesSafeIntersection`,
`IcebergScanNodeTest#testSetPartitionValuesKeepsCommonMetadataWithLegacyFileScanner`
| branch-4.x keeps only the identity columns every spec shares; master keeps
the union, so a file written before the spec evolved to unpartitioned keeps its
values (#68124) |
| `IcebergUtilsTest#testMappedTypesAreExcludedFromPartitionMetadata`,
`IcebergScanNodeTest#testSetPartitionValuesSkipsValuesUnsupportedByMappedTypes`
| Since #68786, master sends UUID and TIMESTAMPTZ identity values too |
| `BrokerUtilTest#parseColumnsFromPathForLoadKeepsLegacyNullSemantics` | A
real difference: on branch-4.x a file load stores a literal `\N` path value as
NULL, master stores the two characters `\N`. It needs a code change and comes
in a separate PR |
**Results.** The three tests pass on master without any code change, and
each catches a regression of what it pins:
| Change to master | Fails |
|---|---|
| `FilePartitionUtils` stops flagging the Hive default partition as NULL |
`parseColumnsFromPathPreservesNullMetadataAndInputKeys` |
| `IcebergScanRange` sends a NULL identity value without the null flag |
both new Iceberg tests |
| The Iceberg planner sends every identity key of every spec for each file |
`planScanSendsEachFilesOwnIdentityPartitionsAfterSpecEvolution` (and the
existing `planScanKeepsUnpartitionedSpecIdentityAfterEvolvingToPartitioned`) |
Original author: @suxiaogang223
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- `BrokerUtilTest` (2) and `IcebergScanPlanProviderTest` (187) pass.
- [ ] 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:
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]
