morningman commented on PR #69392: URL: https://github.com/apache/doris/pull/69392#issuecomment-6359396264
@xylaaaaa I pushed three commits on top of yours (after your 32552aa, which already covers the CI review bot's Arrow memory pool finding, so I left that test as you wrote it). Your fix is right for DELETE/UPDATE/MERGE today; these make the same mistake impossible to repeat silently in the next Iceberg writer: 1. `[refactor](be) Take both paths of every Iceberg file from one output location`: a new header-only `IcebergOutputLocation` holds the data location in both forms (the normalized `output_path` BE writes under, and the table's own form it commits under). The data writer, `VIcebergDeleteFileWriter` and the Puffin path all take both of their paths from it, so `_commit_path_of` is gone, and the delete file writer no longer reports its write path by default. A sink without both location fields is now rejected instead of committing a wrong path (every FE on 3.1 / 4.0 / 4.1 / 4.2 / master sets them). 2. `[improvement](fe) Reject Iceberg commit data reported outside the sink's location`: `planWrite` registers the raw location(s) each sink carries on the transaction, and `addCommitData` rejects a reported file outside them. FE then does not accept the report, so BE deletes its files and the statement fails instead of committing an unreadable path. This also means BE has to be upgraded before FE: an upgraded FE rejects the `s3://` delete file paths an unfixed BE reports. 3. `[test](regression) Check that every Iceberg write records its files under the table location`: `test_iceberg_write_file_locations` runs every write operation (INSERT, OVERWRITE PARTITION, branch INSERT, DELETE, UPDATE, MERGE, rewrite_data_files; v2 and v3) and checks every file in `$all_files` against the table's LOCATION. The REST fixture's warehouse is `s3a://`, which FE normalizes to `s3://`, so CI already ran into this bug; the existing suites just did not check the prefix. About the red BE UT on `486c9b0`: it failed to compile `be/test/exprs/function/function_string_test.cpp`, which this PR does not touch. That was a master break, fixed by #69487 (merged a few hours ago), not this change. The PR description is updated to cover the whole change. -- 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]
