Repository navigation
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
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.
Request changes for one P1 recovery bug: after a successful pre-upgrade LakeSoul DROP, an older catalog image can still prevent FE startup before the committed drop is replayed. This is distinct from the declared removal of LakeSoul read support and incompatibility of catalogs intentionally left present.
Reviewed all 143 changed paths at 518be1a, using two reviewers covering the full diff and a separate reviewer for the three initial risks. No existing inline findings were present, including at live pre-submission readback; no additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and proof: removed readers are deleted and new FE/BE unit cases cover explicit rejection, including full ACID and insert-only Hive. The dropped-catalog image recovery case is missing and exposes the P1.
- Scope: implementation, callers, dependencies, packaging, fixtures and examples are consistently removed; no unrelated production behavior change was substantiated.
- Concurrency: existing metadata synchronization and asynchronous split/cache structure remain; no new lock order, heavy locked operation or shared-state race was found. Checkpoint timing is relevant to the P1.
- Lifecycle/static initialization: removed Hive query transaction and JNI reader owners have no surviving consumers; ordinary reader cleanup and Hudi lifecycle remain. No new static initialization dependency was introduced.
- Configuration: no new setting; the obsolete ACID session flag is removed, with retained session/global variable readers tolerating absent fields.
- Compatibility: protocol symbols remain; persisted LakeSoul subtype removal has the reported recovery flaw even after the documented catalog drop.
- Thrift: every touched definition was compared with HEAD, branch-3.1, branch-4.0, branch-4.1, branch-4.2 and master. Changes are comments only; IDs, meanings and optional fields are preserved.
- Parallel paths: legacy/V2 scanners, lazy ranges, metadata counts, rowid fetch, schema RPC, Hive/Hudi/Iceberg/Paimon wrappers and legacy/Nereids load callers were traced. Surviving ORC decoding and position/equality-delete paths retain the former non-ACID behavior. Iceberg Avro metadata parsing and required Avro dependencies remain.
- Conditions: removed-format guards document transaction/merge semantics and fail explicitly before file readers or count shortcuts; ordinary Hive path/null parsing remains equivalent.
- Test coverage: inspected new negative tests and adjusted surviving tests. No regression covers image-before-DROP replay; retained connector runtime behavior was assessed statically.
- Expected results: obsolete ORC output labels are deleted with their queries; no new expected output values were introduced.
- Observability: unsupported requests fail with explicit diagnostics; no new operational path requiring metrics was found.
- Persistence/failover: image loading precedes journal replay and exposes the P1. Deprecated init-catalog/database journal records are already ignored on this branch; historical CREATE rejection predates this PR.
- Data writes/atomicity: no new data-write or commit path; supported external write transaction managers remain independent. The reported committed metadata drop cannot be recovered from an older image.
- FE/BE variables: no new transmitted variables; deprecated wire structures and IDs remain intact.
- Performance/memory: guards do constant-size setup checks; removed ACID branches simplify ORC processing. No new untracked allocation or retained-file-cache growth was found.
- Further sweep: deleted symbol/module/fixture searches and upstream/downstream rechecks produced no second verified finding.
Validation is static only: no builds, tests, source changes or submodule initialization were performed. Runtime recovery and connector execution remain for CI or a dedicated test environment.
Review converged in 2 rounds: all three reviewers returned NO_NEW_VALUABLE_FINDINGS on the unchanged single finding; all candidates and initial risks have evidence-backed dispositions and no unresolved static coverage gaps remain. The P1 requires changes despite review convergence.
Review converged after 2 round(s).
| .registerSubtype( | ||
| TrinoConnectorExternalCatalog.class, TrinoConnectorExternalCatalog.class.getSimpleName()) | ||
| .registerSubtype(LakeSoulExternalCatalog.class, LakeSoulExternalCatalog.class.getSimpleName()) | ||
| .registerSubtype(LanceExternalCatalog.class, LanceExternalCatalog.class.getSimpleName()) |
Contributor
There was a problem hiding this comment.
[P1] Keep old catalog images readable until DROP replay
A successful DROP CATALOG before upgrading does not guarantee that the latest image no longer contains the LakeSoul catalog: CatalogMgr.dropCatalog() removes the live entry and journals the drop, while checkpoints run asynchronously. If the last image predates that drop, startup calls Env.loadImage() before opening/replaying the edit log, and removing this subtype makes RuntimeTypeAdapterFactory.read() throw on LakeSoulExternalCatalog. The FE therefore cannot start or replay the already-committed drop, even when the operator followed the release note. Keep a metadata-only deserialization path, including nested LakeSoul metadata, so startup can replay dropped catalogs without retaining LakeSoul read support.
4 of 8 tasks
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Gabriel39
added a commit
that referenced
this pull request
Oct 26, 2026
…69333) ### What problem does this PR solve? Add `LoadTableReader` so supported traditional file loads use FileScannerV2 with the existing FE/Thrift source and destination tuples. Source defaults, pre-filters, destination conversion, strict/nullability checks, and flexible partial-update bitmaps run in the table-reader finalization hook. CSV/JSON malformed records are rejected with rollback and load counters. JSON pipes consume every message until EOF; empty arrays and invalid configured roots contribute to the load quality gate without losing valid siblings. Master-specific adaptations: - Preserve master's ADBC/Fluss V2 query selection and Lance exclusion. - Match master's single-argument `TableReader::refresh_conjuncts()` and `RowDescriptor` constructors, and use `SlotDescriptor::type()`. - Omit the physical-split override and shared split-cache field, which are absent on master. Loads retain the existing FE split boundaries. Avro and Arrow IPC loads keep their existing path. No FE or Thrift changes. This port is independent of #69312. ### Validation - Passed the repository clang-format 16 check, covering all 17 changed C++ source/header files. - Passed `build-support/check-build-hygiene.sh` and `git diff --cached --check`. - Passed focused Clang syntax checks for all 11 changed C++ translation units and `table_reader.cpp`, using this master's generated headers and UT build configuration. The standalone scanner check suppresses two pre-existing unused-constant warnings from master. - Carried over the source PR's BE unit tests, JSON pipe tests, and Stream Load regression fixtures. - `run-be-ut.sh` completed configuration and started the full build. The fresh build requires about 14,750 targets, so it was stopped and replaced with the focused syntax checks above. No master runtime unit-test or cluster-regression pass is claimed; full build and runtime validation remain for CI. ### Release note Route supported traditional file loads through FileScannerV2 with a table reader for load conversion and validation.
4 tasks done
Gabriel39
added a commit
that referenced
this pull request
Oct 26, 2026
…69320) ### What problem does this PR solve? Traditional file loads with source/destination tuples always select FileScanner V1. Add `LoadTableReader` as a `TableReader` subclass so supported loads can use FileScannerV2 without FE or Thrift changes. `LoadTableReader::finalize_chunk()` uses base source materialization, then applies per-record defaults, source pre-filters, destination expressions, strict-mode conversion checks, nullability checks, and flexible partial-update skip bitmaps and sequence-column marks. `customize_file_scan_request()` enables malformed-record rejection. The generic TableReader owns file and split lifecycle, with no load-specific flag, branches, or presence-mask storage. Scanner continues to evaluate destination conjuncts before ordinary output projections. Source readers receive no destination predicate, aggregate pushdown, or condition-cache snapshot. Missing physical source aliases fail before reader open unless the FE default map declares a fallback; an explicit NULL default remains valid, and path columns are exempt. CSV/JSON readers reject malformed load records with row rollback and error accounting. JSON reports field-presence masks separately from NULL values so missing defaults and partial updates retain their meaning. Empty outer arrays and array elements with missing or non-object configured roots count as rejected load records so the existing filter-ratio quality gate sees them. Valid siblings remain intact. Fully rejected batches do not end a split. Non-line JSON streams consume each pipe message until pipe EOF, including Kafka routine loads; chunked HTTP buffers remain one document and regular file ranges retain single-document behavior. Supported load formats select V2 independently of the query session option: the Thrift default does not distinguish an omitted option from false in load plans. Query selection remains unchanged. Avro and Arrow IPC stream loads keep their existing path; the current V2 Arrow implementation only supports remote-Doris Flight scans. Load ranges keep their FE split boundaries. This PR targets `branch-4.2` and is independent of #69312. ### Validation - Passed 28 targeted tests in a local ASAN/UBSAN harness, linking rebuilt changed translation units against existing project libraries. Coverage includes strict/non-strict conversion, pre-filter order, explicit NULL versus missing defaults, non-nullability, flexible updates and sequence marks, rejected-batch EOF, scanner selection, real CSV/JSON readers through the load reader, missing physical sources versus explicit NULL defaults and path columns, and ten JSON pipe cases: multiple Kafka messages, batch boundaries, malformed messages, fixed-length HTTP, chunked HTTP, empty pipe EOF, empty outer arrays, missing/null roots, valid siblings around invalid roots, and fully rejected root messages followed by valid messages. The harness also executes two existing scanner-selector scenarios against the production selector. - Recompiled the changed BE implementation units and syntax-checked the affected existing scanner, CSV, JSON, and TableReader test suites. - Passed clang-format 16 checks for all 17 affected C++ source/header files and `git diff --check`. - Added parser unit tests and a Stream Load regression covering target-column WHERE, rejected/unselected counts, JSON defaults, flexible partial updates, and rejection of empty arrays or invalid configured roots with max_filter_ratio=0 versus 1. Full BE build and cluster regression execution remain for CI; they were not run locally. ### Release note Route supported traditional file loads through FileScannerV2 with a table reader for load conversion and validation. ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - Behavior changed: - [x] Yes. Supported traditional loads select V2 independently of the query-only session switch. - Does this need documentation? - [x] Yes. Updated the in-repository FileScannerV2 design document. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
Gabriel39
added a commit
to Gabriel39/incubator-doris
that referenced
this pull request
Oct 26, 2026
…aster ### What problem does this PR solve? Related PR: apache#69312 Problem Summary: Port the removal of unsupported external readers to master. Adapt Hive transaction cleanup to the connector modules and preserve the unified reader initialization interfaces. Reject deprecated reads before file access or metadata-only count shortcuts, and remove obsolete fixtures and examples. Preserve Iceberg and Hudi Avro metadata dependencies. ### Release note Remove Hive transactional table reads, including insert-only tables, and standalone Avro file reads. LakeSoul remains unsupported. Iceberg and Hudi Avro metadata parsing is preserved. All Thrift definitions and IDs remain unchanged. ### Check List (For Author) - Test: Hive connector and HMS unit tests; targeted FE core unit tests; FE Checkstyle; clang-format 16; BE syntax checks; build hygiene; shell syntax and Thrift definition checks. - Behavior changed: Yes, reject unsupported external reads explicitly. - Does this need documentation: No, this ports the existing removal. (cherry picked from commit 518be1a, adapted to master)
Gabriel39
added a commit
that referenced
this pull request
Oct 27, 2026
…aster (#69601) ### What problem does this PR solve? Related PR: #69312 Port #69312 from `branch-4.2` to current `master`. - Remove Hive ACID read transactions, shared read locks, write-ID snapshots, delta-file listing and the BE transactional reader. Reject full ACID and insert-only reads in the Hive connector. - Adapt the changes to master's connector modules and unified reader initialization interfaces. Preserve ordinary Hive scanning and Iceberg/Paimon ORC position-delete and deletion-vector handling. - Reject standalone Avro and deprecated table markers before file access or metadata-only counts. Keep LakeSoul unsupported and remove its obsolete test service and examples. - Keep the retired `skip_checking_acid_version_file` name as a compatibility no-op for client session initialization, using the existing empty-string fallback for variable reads. - Preserve Iceberg/Hudi Avro metadata dependencies. The Thrift changes add deprecation comments only; every definition and numeric ID is unchanged. ### Release note Remove support for Hive transactional reads, including insert-only tables, and standalone Avro file reads. LakeSoul remains unsupported. Iceberg and Hudi Avro metadata parsing remains supported. The retired `skip_checking_acid_version_file` session variable remains accepted as a compatibility no-op. ### Validation - Hive connector: all 490 tests passed; HMS client: all 155 tests passed. - 20 targeted FE core tests passed: deprecated-read rejection, partition-path parsing, generic file scan planning and Hive streaming scans. - Review follow-up: reproduced the legacy ACID session-variable failure before the fix; all 8 focused compatibility, variable-export and unsupported-read tests passed after the fix. - FE Checkstyle passed with zero violations for every affected FE module. - All 12 affected C++ source/header files passed clang-format 16; build hygiene and shell syntax checks passed. - Syntax-only compilation passed for all 6 affected BE production translation units and all 4 affected BE test translation units. Isolated checks relaxed pre-existing unused-constant/shadow warnings. Full BE linking and runtime tests remain for CI. - Verified that Thrift changes are comments only and checked the affected definitions against branch-3.1/4.0/4.1/4.2. `git diff --check` passed. ### Check List (For Author) - Test - [x] Unit Test - [x] Manual test: formatting, syntax, build hygiene and Thrift compatibility checks described above. - Behavior changed: - [x] Yes. Reject unsupported external reads explicitly. - Does this need documentation? - [x] No. This ports the existing removal to master.
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?
branch-4.2still contains Hive transactional, standalone Avro, and LakeSoul readers that are no longer supported. Remove their implementations, dependencies, packaging, examples, and related regression tests.Release note
Remove support for Hive transactional table reads, standalone Avro file reads, and LakeSoul. Existing LakeSoul catalog metadata is no longer deserializable; remove LakeSoul catalogs before upgrading. Iceberg Avro metadata files remain supported.
Validation
bash -n;git diff --checkpassed.Check List (For Author)