Repository navigation
branch-4.2: [improvement](datasource) Bound and fairly schedule external scan planning (#69298) - #69986
branch-4.2: [improvement](datasource) Bound and fairly schedule external scan planning (#69298)#69986924060929 wants to merge 5 commits into
Conversation
…nning ### What problem does this PR solve? Issue Number: None Related PR: apache#69298, apache#69177 Problem Summary: Adapt fair and bounded external scan planning to branch-4.2's native datasource and generation ownership APIs. Include the necessary native Hive streaming precursor from apache#69177. Split producers yield after bounded work and when backend queues fill, statement-owned task groups fairly admit query work, and a separate metadata executor prefetches a bounded window of Iceberg manifests. Preserve branch-native manifest cache, delete filtering, snapshot/authentication context, count pushdown, explicit file split sizing and Hudi behavior. Keep generation resources alive until asynchronous work actually terminates and independently cap the initial split response. ### Release note Bound external scan planning queues and schedule concurrent query planning fairly, reducing unnecessary full-file planning before batch scans can begin. ### Check List (For Author) - Test: Unit Test: 293 selected FE tests passed (213 primary and 80 supplemental); full build.sh --fe package build and Checkstyle passed. The external-service Hive streaming regression artifacts are carried verbatim from apache#69177 and were not run locally. - Behavior changed: Yes, external scan planning uses bounded cooperative producers, fair statement task groups and bounded Iceberg manifest prefetch. - Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
1 similar comment
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
### What problem does this PR solve? Issue Number: None Related PR: apache#69298 Problem Summary: Cold Hive scans now plan file splits through the bounded streaming path. With hive.ignore_absent_partitions=false, a missing partition raises a UserException wrapped by the file cache. The streaming completion callback exposed only the outer cache message, losing the missing-partition reason previously shown by synchronous planning. Reuse one Hive scan error conversion in both paths to retain the table name, underlying reason and original cause. The missing-partition policy and error status are unchanged. ### Release note Preserve actionable Hive file-listing errors during streaming scan planning. ### Check List (For Author) - Test: Unit Test: the new cold-streaming initialization test reproduced the reported message loss before the fix; all 94 selected FE tests passed afterward, including synchronous parity, streaming, cancellation and delivery cases. Full build.sh --fe package build and Checkstyle passed. The external-service regression was not run locally and will be re-triggered on the updated PR. - Behavior changed: Yes, asynchronous Hive split-planning errors retain the underlying cause text and table context, matching the existing synchronous path. - Does this need documentation: No
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
### What problem does this PR solve? Issue Number: None Related PR: apache#69298 Problem Summary: The active-read cancellation test for external-file split planning waited for the source-close latch and then immediately required the producer completion counter to be one. SplitProducer closes the source before invoking its completion callback, so the test can legitimately observe zero completions and fail FE CI. Hold the test read active across cancellation, force a controlled close/completion gap, and wait for a callback-completed latch before checking terminal counts. This deterministically verifies exactly one close and one completion without depending on thread scheduling. ### Release note None ### Check List (For Author) - Test: Unit Test - Reproduced the exact CI assertion failure with a deterministic close barrier before fixing the completion wait. - All 62 related FE tests pass: SplitProducerTest, SplitAssignmentTest, SplitSourceTest, HiveStreamingSplitTest and FairTaskExecutorTest. - Full FE packaging and Checkstyle pass through build.sh --fe -j4 (UI build disabled). - Behavior changed: No (test synchronization only) - Does this need documentation: No
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
FE UT Coverage ReportIncrement line coverage |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Converged after 2 static review rounds against PR head 4b882f7. The review covered all 33 changed files, the fair executor and split-fetch lifecycle, Hive/Hudi/FileSplitter batch paths, and Iceberg manifest/cache streaming and partition evolution. Two verified P1 findings are included inline: Hudi can miss cancellation before a blocking filesystem-view sync, and Iceberg batch manifest streaming cannot adapt historical DATE partitions after DATE-to-TIMESTAMP_NANO promotion. The executor wakeup/close paths and normal Hive split/error/empty/cold-cache behavior were traced and no additional issue was substantiated; Hudi retains its eager assignment path and split-count estimates are advisory only. There was no additional user-provided review focus and no existing blocking inline finding to carry forward. This was a static-only review under the supplied contract; builds and tests were inspected but not executed.
Review converged after 2 round(s).
| // Only provide the unique ID of split source to backend. | ||
| splitAssignment = new SplitAssignment(backendPolicy, this, this::splitToScanRange, | ||
| locationProperties, pathPartitionKeys, admissionResult); | ||
| getScanTaskLease(); |
There was a problem hiding this comment.
[P1] Hudi can miss cancellation before syncFsView starts. Registering the statement lease here lets cleanup stop the SplitAssignment while Hudi's BatchFsViewOwner has not yet published its sync thread. SplitAssignment.addCloseable then closes the owner with syncThread still null; Hudi subsequently calls beginSync and the potentially blocking syncFsView unconditionally (lines 738-744), so the stop interrupt is missed and a cancelled query can remain blocked on remote timeline I/O. Check the stop state before entering sync or make owner publication/close atomically interrupt an already-stopping sync.
There was a problem hiding this comment.
Fixed in 0068bcf. Both Hudi fs-view owners publish the sync thread before checking cancellation, so an already-closed owner rejects the remote sync; a later close interrupts the published thread. Batch beginSync is inside the terminal catch/finally to preserve exact lease release. Added deterministic close-before-publication cases alongside the existing blocked-sync tests; all 339 related FE tests and full FE packaging/Checkstyle pass.
| } | ||
| return decimal; | ||
| } | ||
| throw new IllegalArgumentException("Cannot adapt partition value from " + source + " to " + target); |
There was a problem hiding this comment.
[P1] Preserve historical DATE partition values across DATE-to-TIMESTAMP_NANO evolution. DorisDataTableScan rebinds historical manifest specs to the current schema, so ManifestCacheValue.forScan reaches this unsupported conversion for old date-partitioned files and throws. In batch mode this runs lazily from IcebergScanNode's manifest iterator after the manifest-cache fallback boundary, causing the query to fail instead of scanning the historical partition (and dropped promoted fields can also mismatch equality-delete partition keys). Add a correct DATE/TIMESTAMP_NANO adaptation or retain the historical spec/value for matching, with a partitioned streaming regression test.
There was a problem hiding this comment.
Fixed in 0068bcf. Query-local adaptation now converts DATE/nanos exactly and normalizes dropped temporal partition keys without overflow. Delete manifest caching also preserves the manifest's physical partition types, matching the data-file reader, so current schemas cannot erase or reinterpret historical partition values before adaptation. Added partitioned streaming tests using the production cache readers: old DATE data retains its equality delete both before and after the promoted identity field is removed. All 339 related FE tests and full FE packaging/Checkstyle pass.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69298 Problem Summary: Statement cleanup can stop a Hudi split assignment before its filesystem-view owner publishes the sync thread. Reject an already-stopped owner before remote sync and preserve terminal lease cleanup. Iceberg streaming scans adapt historical identity partitions to the selected schema, but DATE-to-nanosecond timestamp conversion was missing and dropped promoted fields could lose equality deletes. Preserve physical partition types in delete manifests, convert temporal values exactly, and normalize dropped partition keys in query-local views. ### Release note Fix cancelled Hudi planning and Iceberg scans after DATE partition columns are promoted to nanosecond timestamps or removed. ### Check List (For Author) - Test: Unit Test: 339 related FE tests; full FE packaging and Checkstyle via build.sh --fe - Behavior changed: Yes; reject cancelled Hudi sync and preserve historical temporal partitions and equality deletes - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static exact-head review of PR 69986 at 0068bcf. The new scheduler, bounded split queues, Hive/Hudi streaming producers, lazy FileSplitter, session/config forwarding, Iceberg manifest prefetch, historical partition adaptation, delete indexing, cancellation, and generation cleanup were reviewed across the complete changed-file list. Three Iceberg issues are verified below. The existing DATE/TIMESTAMP_NANO Iceberg thread and Hudi close-before-sync thread were revalidated as fixed at this head and are not reposted. No additional user review focus was provided. No builds or tests were executed; this was a static-only review. The review converged after 2 rounds.
Review converged after 2 round(s).
| } | ||
| return decimal; | ||
| } | ||
| throw new IllegalArgumentException("Cannot adapt partition value from " + source + " to " + target); |
There was a problem hiding this comment.
[P1] Support DATE to ordinary TIMESTAMP partition promotion. The adapter converts DATE only when the target is TIMESTAMP_NANO; an old identity-partitioned DATE file rebound to an ordinary Iceberg TIMESTAMP (microseconds) reaches the final unsupported-conversion exception. DorisDataTableScan rebinds historical specs by field ID, and the batch manifest iterator evaluates this adaptation lazily, so the producer fails the query instead of reading the historical partition. Add an exact days-to-micros conversion with overflow checks and a partitioned DATE-to-TIMESTAMP regression covering data and delete files.
There was a problem hiding this comment.
Fixed in 68ef8c2. DATE partitions now convert exactly to ordinary without-zone TIMESTAMP microseconds, with overflow checks and exact-midnight checks for reverse historical adaptation. DATE-to-zoned timestamp conversions are rejected for both precisions. Added real partitioned streaming metadata cases and unit tests for positive/negative days, nulls, overflow and lossy narrowing. This is a reader repair; SQL ALTER type support is unchanged. All 253 related FE tests and full FE packaging/Checkstyle pass.
| if (source.typeId() == Type.TypeID.FLOAT) { | ||
| return ((Float) value).doubleValue(); | ||
| } | ||
| if (source.typeId() == Type.TypeID.DATE) { |
There was a problem hiding this comment.
[P1] Normalize dropped DATE and ordinary TIMESTAMP keys consistently. In the UNKNOWN target branch, dropped DATE values become BigInteger days multiplied by NANOS_PER_DAY, while ordinary TIMESTAMP values fall through as Long microseconds; only TIMESTAMP_NANO uses the common BigInteger representation. After a partition field is promoted from DATE to ordinary TIMESTAMP and dropped, an old data file and a current equality-delete file therefore produce unequal keys for DeleteFileIndex, so the delete can be omitted. Convert ordinary TIMESTAMP to the same lossless representation and add a dropped DATE-to-TIMESTAMP equality-delete regression.
There was a problem hiding this comment.
Fixed in 68ef8c2. Dropped DATE, microsecond TIMESTAMP and nanosecond TIMESTAMP keys now use the same lossless BigInteger nanosecond representation, so comparisons and hashes agree without overflowing the source ranges. The dropped DATE-to-microsecond partition test now retains its equality delete, and unit tests compare keys across all three representations. All 253 related FE tests and full FE packaging/Checkstyle pass.
| throw new IllegalArgumentException("Cannot adapt partition value from " + source + " to " + target); | ||
| } | ||
|
|
||
| private abstract static class FileView<F extends ContentFile<F>> implements ContentFile<F> { |
There was a problem hiding this comment.
[P2] Keep adapted Iceberg files serializable for task reuse. ManifestCacheValue.forScan returns DataView/DeleteView when a historical manifest partition type differs from the rebound spec, but FileView implements only ContentFile and is not Serializable. Non-batch manifest planning serializes each BaseFileScanTask; this throws and planFileScanTaskWithoutReuse falls back to a full SDK scan, so adapted DATE/TIMESTAMP or dropped-field tables bypass statement manifest/task reuse. Make the views serializable with serializable state, or materialize concrete serializable Iceberg file objects, and add a non-batch adapted-task reuse regression test.
There was a problem hiding this comment.
Fixed in 68ef8c2. The shared static file-view base and both concrete wrappers are serializable with explicit serialVersionUID; their only state is the native Iceberg content file and query-owned PartitionData. No adapter, cache loader or statement resource is captured. A real supported int-to-long partition evolution test reproduced NotSerializableException before the change and now verifies serialized data/delete partition values and one manifest load across two equivalent non-batch relations. All 253 related FE tests and full FE packaging/Checkstyle pass.
…views ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69298 Problem Summary: Historical DATE identity partitions rebound to ordinary microsecond timestamps fail during lazy manifest planning, and dropped DATE/timestamp partition keys use incompatible units and Java representations that can omit equality deletes. Add exact DATE/microsecond conversions and a common lossless dropped temporal-key representation while rejecting overflow, lossy narrowing and unsupported zoned promotions. Adapted content-file views also lack Serializable, causing non-batch statement task reuse to fail and fall back to SDK planning. Make the static data/delete wrappers serializable without capturing query resources, and cover actual serialized reuse across equivalent relations. ### Release note Fix Iceberg scans and equality-delete matching after DATE partition evolution to microsecond timestamps, and preserve statement task reuse for adapted manifest files. ### Check List (For Author) - Test: Unit Test: 253 related FE tests; full FE packaging and Checkstyle via build.sh --fe - Behavior changed: Yes; read historical temporal partitions correctly and reuse serialized adapted scan tasks - Does this need documentation: No
|
run buildall |
|
/review |
cherry pick from #69298