[feature](iceberg) Support Iceberg position deletes system table#65135
[feature](iceberg) Support Iceberg position deletes system table#65135hubgeter wants to merge 16 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for one correctness issue in the new Iceberg position_deletes system table path.
Subagent results: optimizer-rewrite and tests-session-config both completed their initial passes without candidates. After MAIN-1 was added to the proposed final comment set, convergence round 1 asked both live agents to review the same ledger/comment set; both replied exactly NO_NEW_VALUABLE_FINDINGS.
User focus: no additional user-provided review focus.
Checkpoint conclusions: static review covered the FE table function/system-table resolution path, Iceberg split construction, Thrift handoff, BE position-delete reader, and the new regression coverage. No existing inline comments were present, so this is not a duplicate. git diff --check passed against the fetched base. I did not run FE/BE builds or tests because this checkout is missing thirdparty/installed and thirdparty/installed/bin/protoc.
| Map<String, String> partitionJson = new LinkedHashMap<>(); | ||
| List<PartitionField> fields = partitionSpec.fields(); | ||
| for (int i = 0; i < fields.size(); i++) { | ||
| partitionJson.put(fields.get(i).name(), partitionValues.get(i)); |
There was a problem hiding this comment.
The partition payload here loses the actual JSON types. getPartitionValues() returns strings for every Iceberg partition value, and Gson serializes this Map<String, String> as e.g. {"id":"1"} for an integer partition. On BE, _append_partition_column() deserializes that object straight into the partition struct column; the struct serde forwards "1" to the numeric field serde, which does not strip JSON quotes, and nullable struct fields then turn the parse failure into NULL. So a table partitioned by an integer/date field will expose wrong partition values in $position_deletes even though the new tests only cover string dt partitions. Please preserve typed JSON values here, and add a regression with at least one non-string partition column.
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
FE UT Coverage ReportIncrement line coverage |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 30288 ms |
TPC-DS: Total hot run time: 174067 ms |
ClickBench: Total hot run time: 25.53 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
I found two correctness issues in the new Iceberg system-table changes: the native $position_deletes partition payload is still incompatible with partition spec evolution, and the JNI metadata scanner regresses Kerberos auth property propagation for existing Iceberg metadata tables.
Critical checkpoints: goal/behavior is partially achieved; scope is focused on Iceberg system-table plumbing; compatibility has the JNI auth regression; tests miss partition-spec evolution and Kerberos JNI metadata-table coverage; no separate concurrency/lifecycle issue was found. Validation was static review against the GitHub PR patch and live PR metadata; I did not run build/tests.
| split.setPartitionSpecId(deleteFile.specId()); | ||
| PartitionSpec partitionSpec = icebergTable.specs().get(deleteFile.specId()); | ||
| if (partitionSpec != null && partitionSpec.isPartitioned() && deleteFile.partition() != null) { | ||
| split.setPartitionDataJson(getPartitionDataObjectJson( |
There was a problem hiding this comment.
This builds partition_data_json from only the delete file's own PartitionSpec, but the $position_deletes output schema is the metadata table schema, whose partition column uses Iceberg's unified partition type (the union of partition fields across specs). After ALTER TABLE ... ADD PARTITION FIELD, older delete files still have the old spec and this JSON omits the new field. BE then deserializes it into the output STRUCT, and DataTypeStructSerDe requires every field in order, so select partition from tbl$position_deletes fails with Actual struct field number ... is less than schema field number .... Please generate the JSON against the metadata table partition struct, filling fields absent from this delete file's spec with null, and add a regression that writes deletes before and after partition-spec evolution.
| .collect(Collectors | ||
| .toMap(kv1 -> kv1.getKey().substring(HADOOP_OPTION_PREFIX.length()), kv1 -> kv1.getValue())); | ||
| this.preExecutionAuthenticator = PreExecutionAuthenticatorCache.getAuthenticator(hadoopOptionParams); | ||
| this.preExecutionAuthenticator = PreExecutionAuthenticatorCache.getAuthenticator(params); |
There was a problem hiding this comment.
This drops the transport-prefix stripping that the BE side still relies on. IcebergSysTableJniReader prefixes every scan property with hadoop.; for HDFS Kerberos the FE/backend properties already contain keys like hadoop.security.authentication, hadoop.kerberos.principal, and hadoop.kerberos.keytab. Passing the raw params map here turns those into doubled keys such as hadoop.hadoop.security.authentication, while AuthenticationConfig only checks the normal Hadoop keys before creating the Kerberos authenticator. Existing $snapshots/$files scans against a Kerberized Iceberg/HDFS catalog will therefore fall back to simple auth. Please keep building the stripped Hadoop option map for PreExecutionAuthenticatorCache (as Paimon/Hudi do), and keep scanner-only keys out of the cache input.
TPC-H: Total hot run time: 30069 ms |
TPC-DS: Total hot run time: 173742 ms |
ClickBench: Total hot run time: 25.22 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
Gabriel39
left a comment
Gabriel39
left a comment
There was a problem hiding this comment.
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:993
partition_data_json 仍按 delete file 自己的 PartitionSpec 生成，但 $position_deletes 的 partition 输出列来自 Iceberg metadata table schema，可能是跨多个 partition spec 的 unified struct。表发生 partition spec evolution 后，旧 delete file 的 JSON 会缺少新字段，BE 反序列化 struct 时可能报字段数不匹配，导致 select partition from tbl$position_deletes 失败。建议按 metadata table 的 partition struct 生成 JSON，缺失字段填 null，并补一个“delete 前后发生 partition spec evolution”的回归测试。
| "file"); | ||
| } | ||
| _delete_file_desc = &_iceberg_file_desc->delete_files[0]; | ||
| _delete_file_kind = is_iceberg_deletion_vector(*_delete_file_desc) |
There was a problem hiding this comment.
if (is_iceberg_deletion_vector(...)) {
_delete_file_kind = DELETION_VECTOR;
} else if (content == POSITION_DELETE) {
_delete_file_kind = POSITION_DELETE;
} else {
return Status::InternalError(...);
}
|
[P1] fe/be-java-extensions/iceberg-metadata-scanner/src/main/java/org/apache/doris/iceberg/IcebergSysTableJniScanner.java:66 |
|
Thanks for the work — the native 1. (High) Kerberos /
|
| delete file partition | FE JSON | Doris partition |
|---|---|---|
| (a=p1, b=q1) | {"a":"p1","b":"q1"} |
{"a":"p1", "b":"q1"} ✅ |
| (a=p1, b=NULL) | {"a":"p1"} (b dropped) |
NULL ❌ |
spec0 file after ALTER TABLE ADD PARTITION FIELD id |
{"dt":...} |
NULL ❌ |
The query succeeds but the partition column silently loses data — any table that has undergone partition evolution shows NULL partitions for delete files written under older specs. This isn't covered by the "Preserve typed partition values" commit (that fixed value typing, not the struct shape). Suggest emitting the partition in the table's unified partition type (coerce PartitionData, as Iceberg's own metadata scan does) and enabling null serialization so the field count always matches the schema.
4. (Latent crash, low reachability) optional row column + schema evolution
When a position-delete file actually contains the optional row struct and the table schema later evolves, the standalone ParquetReader init (no column_descs → on_before_init_reader skipped → ConstNode table_info_node whose children_column_exists() is always true) can default-insert and dereference a null child reader in StructColumnReader::read_column_data → SIGSEGV. Mainstream Spark MoR deletes don't populate row (this PR's own test asserts it's always NULL), so it's hard to hit in practice, but it's a latent crash for files written by engines that do populate row. (Code trace only — not reproduced e2e.)
Minor / nits
file_scanner.cpp: the ~10-line position-deletes reader-creation block is byte-identical in theFORMAT_PARQUETandFORMAT_ORCarms and is format-independent (the reader dispatches on the delete file's own format); consider hoisting it once before the switch. Mapping PUFFIN →FORMAT_PARQUETingetNativePositionDeleteFileFormatexists only to land DV ranges on an arm containing that block.iceberg_meta(query_type=...)resolves viatbl + "$" + queryTypesplit on the last$, soquery_type='foo$snapshots'silently resolves to the snapshots table and returns data instead of erroring (reproduced:count=2). Consider looking the type up directly in the supported sys-tables map._init_deletion_vector_readermaterializes the whole roaring bitmap into astd::vector<uint64_t>up front; iterating the compressed bitmap per batch would bound memory for large deletion vectors.doGetPositionDeletesSystemTableSplitsiteratesplanFiles()(one split per delete file, no size-based split viaplanTasks/TableScanUtil.splitFiles), so a large delete file gets no intra-file parallelism unlike the data path.
### What problem does this PR solve? Issue Number: close #xxx Related PR: apache#64886 Problem Summary: Address review feedback for the Iceberg position deletes system table. The position deletes split now reuses the normal Iceberg table format and BE chooses the native reader from Iceberg delete content, partition JSON is generated against the metadata table unified partition struct so partition spec evolution fills missing fields with null, and the JNI metadata scanner restores Hadoop option prefix stripping for Kerberos authentication. The extra iceberg_meta TVF path is removed from this PR scope. The regression framework Spark JDBC helper from PR apache#64886 is picked into this branch and the position_deletes suite now cross-checks Spark and Doris metadata table results. ### Release note None ### Check List (For Author) - Test: Regression test / Unit Test / Manual test - ENABLE_PCH=OFF ./build.sh --be --fe -j 16 - ./run-regression-test.sh --run -d external_table_p0/iceberg -s test_iceberg_position_deletes_sys_table ... - ./run-regression-test.sh --run -d external_table_p0/iceberg -s test_iceberg_sys_table ... - EXTRA_FE_MODULES=iceberg=be-java-extensions/iceberg-metadata-scanner ./run-fe-ut.sh --run org.apache.doris.iceberg.IcebergSysTableJniScannerTest - Behavior changed: No - Does this need documentation: No
Design notes (for discussion, not blockers)Separating these from the bugs above since they're about the integration seams, not clear defects. The core direction is right:
These are design opinions, not blockers — happy to defer to your and the committers' judgment. |
### What problem does this PR solve? Issue Number: None Related PR: apache#64886 Problem Summary: After rebasing onto master, Iceberg scans can use FileScannerV2. Iceberg position delete system table ranges still need the dedicated position delete reader path instead of the normal Iceberg data reader or JNI metadata reader path. This change adds a FileScannerV2 table reader adapter that reuses the existing position delete system table reader, keeps the FileScannerV1 path unchanged, and extends the regression suite to cover both scanner paths plus Doris/Spark alternating Iceberg operations. ### Release note Support Iceberg position_deletes system table queries when FileScannerV2 is enabled. ### Check List (For Author) - Test: Regression test / Build / Manual test - `./build.sh --be --fe` - `./run-regression-test.sh --conf /tmp/doris-position-deletes-regression-conf.groovy --run -d external_table_p0/iceberg -s test_iceberg_position_deletes_sys_table` - Deployed `/mnt/disk1/changyuwei/doris-deploy`; `SHOW FRONTENDS` and `SHOW BACKENDS` both reported Alive=true. - Behavior changed: Yes. Iceberg position_deletes system table queries now work through FileScannerV2. - Does this need documentation: No
hubgeter
force-pushed
the
support_iceberg_sys_pos_del
branch
from
9d441a1 to
80e4825
Compare
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#65135 Problem Summary: PR review raised concern that ORC Iceberg position delete system table splits could be routed through FileScannerV2, whose position delete reader only supports Parquet delete files. FileScannerV2 already rejects ORC ranges and the V1 position delete system table reader owns ORC support, but the boundary was not covered by a focused unit test. Add a FileScannerV2 test that locks Parquet position delete and deletion-vector routing to V2 while proving ORC position delete ranges make the mixed split source fall back to V1. Also document why the V2 deletion-vector reader reports EOF only on the next empty call: FileScannerV2 treats eof=true as advancing to the next split without returning the current block. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-be-ut.sh --run --filter=FileScannerV2Test.IcebergPositionDeletesOrcFallsBackToV1 -j 8 - Behavior changed: No - Does this need documentation: No
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 30026 ms |
TPC-DS: Total hot run time: 180413 ms |
ClickBench: Total hot run time: 25.04 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
|
/review |
Gabriel39
left a comment
Gabriel39
left a comment
There was a problem hiding this comment.
Requesting changes for two correctness gaps in the native Iceberg $position_deletes path: partition-field renames silently lose values from older delete files, and valid Avro position-delete files fail during FE planning.
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at 10:57 AM. 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#65135 Problem Summary: Iceberg preserves a partition field ID when a partition field is renamed, while the position_deletes metadata table exposes the latest field name. Doris matched historical partition values by name, so delete rows written under an older spec returned NULL for the renamed field. Match partition values by partition field ID, add focused FE coverage, and add a Spark/Doris regression that writes deletes before and after the rename. Keep unsupported AVRO position delete files fail-fast and cover that boundary explicitly. ### Release note Fix partition values in Iceberg position_deletes metadata queries after partition-field renames. ### Check List (For Author) - Test: Unit Test / Regression Test - ./run-fe-ut.sh --run org.apache.doris.datasource.iceberg.source.IcebergScanNodeTest - ./run-regression-test.sh --run -d external_table_p0/iceberg -s test_iceberg_position_deletes_sys_table - ./build.sh --fe - Behavior changed: Yes. Historical position delete rows retain values after a partition field rename. - Does this need documentation: No
|
run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#65135 Problem Summary: Merge the latest master without restoring the removed per-split FileScanner V1 fallback. Route Iceberg position-delete system table reads for both Parquet and ORC through FileScannerV2, while keeping AVRO explicitly unsupported. ### Release note Support Iceberg ORC position-delete files in the position_deletes system table FileScannerV2 path. ### Check List (For Author) - Test: Manual test - clang-format 16 dry-run check for the changed C++ files - Targeted Ninja compilation of the reader and FileScannerV2 unit-test objects - Behavior changed: Yes. ORC position-delete files now use the V2 reader; AVRO remains rejected. - Does this need documentation: No
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
Automated review completed. I found two issues that should be addressed before this lands: one compatibility issue in the native $position_deletes scan range protocol during smooth/rolling upgrade, and one regression-test assertion gap for ORC row projection.
Checkpoint summary: FE planning/schema, BE native reader behavior, FE/BE thrift routing, permissions, partition evolution/rename/type handling, deletion vectors, scanner v1/v2 paths, and test coverage were reviewed statically. Existing review threads for Avro support and prior partition/auth concerns were treated as already-known and not duplicated. No additional user-provided focus was present.
| private void setIcebergPositionDeleteSysTableParams(TFileRangeDesc rangeDesc, IcebergSplit icebergSplit, | ||
| TTableFormatFileDesc tableFormatFileDesc, TIcebergFileDesc fileDesc) { | ||
| rangeDesc.setFormatType(icebergSplit.getPositionDeleteFileFormat()); | ||
| tableFormatFileDesc.setTableLevelRowCount(-1); |
There was a problem hiding this comment.
This native $position_deletes path needs a BE support/smooth-upgrade guard before it emits Parquet/ORC/Puffin ranges. The range format is switched to the delete file format here and the only signal that it is a system-table range is the top-level iceberg_params.content set below; old BEs do not have the new is_iceberg_position_deletes_sys_table routing, so they will take the ordinary Iceberg Parquet/ORC reader path against the metadata-table slots if FederationBackendPolicy assigns the split to them during rolling/smooth upgrade. Other new plan shapes already skip Backend.isSmoothUpgradeSrc() BEs before dispatch; this should similarly fall back, gate on all selected BEs supporting the new route, or fail during planning instead of sending a range old BEs can misread.
| assertEquals(orcUnpartitionedCount, (long) sql( | ||
| """select `row` from pd_orc_unpartitioned\$position_deletes""").size()) | ||
| assertTrue(unpartitionedRows.every { it[0] == null }) | ||
| try { |
There was a problem hiding this comment.
This assertion is still checking unpartitionedRows, which is the Parquet result populated above. The ORC query on the preceding lines only has its row count checked, so a regression in pd_orc_unpartitioned$position_deletes.row values would still pass this test. Please store the ORC row result in its own variable and assert that variable here.
### What problem does this PR solve? Issue Number: None Related PR: apache#65135 Problem Summary: The native Iceberg position_deletes system table range requires the new BE reader routing. During smooth upgrade, the external scan backend pool could still include an old source BE and dispatch a Parquet, ORC, or deletion-vector range that the old process would misread. Reject native position_deletes planning when a selected backend is a smooth-upgrade source. Also fix the ORC regression assertion so it validates the ORC row result instead of reusing the Parquet result. ### Release note Prevent Iceberg position_deletes scans from being dispatched to smooth-upgrade source backends. ### Check List (For Author) - Test: Unit Test / Regression test attempted - ./run-fe-ut.sh --run org.apache.doris.datasource.iceberg.source.IcebergScanNodeTest passed 6 tests. - ./run-regression-test.sh --conf /tmp/doris-position-deletes-regression-conf-9030.groovy --run -d external_table_p0/iceberg -s test_iceberg_position_deletes_sys_table was attempted; the available cluster failed before assertions with an FE NullPointerException on DESC because it does not contain the PR implementation. - Behavior changed: Yes. Native position_deletes planning now fails while a selected backend is a smooth-upgrade source. - Does this need documentation: No
|
/review |
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
| if (_delete_file_desc->file_format == TFileFormatType::FORMAT_PARQUET) { | ||
| auto parquet_reader = ParquetReader::create_unique( | ||
| _profile, *_range_params, _range, _batch_size, &_state->timezone_obj(), | ||
| &_io_context.io_ctx, _state, _meta_cache); |
There was a problem hiding this comment.
This V1 system-table reader is passing the nested Parquet reader a private _io_context instead of the scanner IO context. The same member is used for the ORC and Puffin DV paths, but FileScanner::try_stop() only flips the scanner _io_ctx->should_stop, and the scanner counters/profile are populated from the scanner-owned reader/cache stats. Existing V1 Iceberg delete-vector reads pass this->get_io_ctx() into the helper, so this new path can keep reading large delete files after a LIMIT/cancellation stop and underreport the actual delete-file IO. Please thread the scanner IOContext into this reader, or mirror should_stop and merge the private stats back before counter collection.
5 participants
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)