mmadzia-sfsh opened a new pull request, #67552:
URL: https://github.com/apache/doris/pull/67552
### What problem does this PR solve?
Issue Number: close #67544
Related PR: #xxx
Problem Summary:
On an Iceberg catalog whose storage URI the FE rewrites on the way to the
BE, `UPDATE` / `DELETE`
committed the position-delete file into the manifest under Doris's
**internal, normalized** path
instead of the catalog's own URI. The commit succeeds, and every subsequent
read of the table then
fails permanently:
```
ERROR 1105 (HY000): errCode = 2, detailMessage =
Failed to create LocationPath for location:
s3://<container>/<uuid>/data/delete_pos_<uuid>_125613.zstd.parquet
```
The table is not recoverable through SQL — the bad path is durable Iceberg
metadata and has to be
rolled back to the previous snapshot out-of-band.
This is the asymmetry between the two write lanes:
| lane | writer | reports to the manifest |
|---|---|---|
| data | `VIcebergPartitionWriter` | `original_write_path` ✅ |
| position delete | `VIcebergDeleteFileWriter` | `_output_path` (normalized)
❌ |
| deletion vector (v3) | `VIcebergDeleteSink::_write_deletion_vector_files`
| the normalized puffin path ❌ |
The data writer keeps the normalized path for I/O and a second, original
path for the manifest.
The delete lanes only ever had one path, and used it for both. So the *same*
`UPDATE` writes a
correct `abfss://` data file and a broken `s3://` delete file.
Reproduced on ADLS Gen2
(`abfss://[email protected]/key`, normalized to
`s3://container/key` by `AzurePropertyUtils.validateAndNormalizeUri()`), but
it is not
Azure-specific: it applies to any storage whose location is rewritten for
the BE. The FE's own
`IcebergWritePlanProviderTest.planWriteBuildsDeleteSinkWithTableDerivedFields`
pins exactly this
pair for OSS —
```java
Assertions.assertEquals("s3://bucket/wh/db1/t1/data", sink.getOutputPath(),
"delete output path is the normalized data location (legacy
LocationPath.toStorageLocation)");
Assertions.assertEquals("oss://bucket/wh/db1/t1/data",
sink.getTableLocation(),
"table_location stays the raw data location (legacy
IcebergUtils.dataLocation)");
```
### The invariant this relies on
`TIcebergDeleteSink.output_path` and `.table_location` are **the same
directory in two URI forms** —
both are derived from a single `resolveLocationFields(table,
schemaContext.getDataLocation())` call,
in `IcebergWritePlanProvider#buildDeleteSink` (delete) and `#buildMergeSink`
(update/merge), and
`IcebergWritePlanProviderTest.planWriteBuildsDeleteSinkWithTableDerivedFields`
pins both literal
values. They are never two different directories, and `table_location` is
never the table root.
### The fix
BE-only, no thrift change and no new FE plumbing — as suggested during
triage.
`TIcebergDeleteSink` **already** carries both forms of the same directory:
`output_path`
(normalized, for I/O) and `table_location` (raw, as the catalog reports it).
`VIcebergDeleteSink`
now generates one file *name* and joins it to both bases:
- `DeleteFileLocation::write_path` → filesystem I/O and the abort-time
cleanup list,
- `DeleteFileLocation::original_path` → `TIcebergCommitData.file_path`, i.e.
the manifest.
`VIcebergDeleteFileWriter` gained an `original_output_path` alongside
`_output_path` and reports
*that* in `commit_data`. The v3 deletion-vector / Puffin lane gets the same
treatment — it was
code-identical and would have failed the same way once a v3 table saw a DML
on rewritten storage.
Both `DELETE` and the delete half of `UPDATE` / `MERGE` go through
`VIcebergDeleteSink`
(`VIcebergMergeSink::_build_inner_sinks` synthesizes the inner delete sink
and propagates both
`output_path` and `table_location`), so one change covers all three
statements.
When only one of the two fields is present, it stands in for both. That
keeps the previous behaviour
for any location that was never rewritten, and makes the change safe against
an FE that does not
send both. Note the fallback cannot do better than "unchanged from today" in
the rewritten case:
`abfss://<container>@<account>.dfs.core.windows.net/<key>` is not
recoverable from
`s3://<container>/<key>`, because normalization drops the account. (The data
lane does not fall back
at all — `viceberg_table_writer.cpp` concatenates an unset
`original_output_path` as an empty
string. The delete lane is deliberately more defensive here; happy to match
the data lane instead if
you prefer the consistency.)
#### Why not a new `original_output_path` field on `TIcebergDeleteSink`?
For symmetry with the data lane that would be the tidier shape, and I will
add it if you prefer. I
chose not to because a thrift field makes the fix new-FE-only and reopens an
old-FE/new-BE window,
whereas this BE-only change corrects the manifest against **every**
already-deployed FE, needs no
wire-format change, and backports to `branch-4.1` unchanged (its legacy
`planner/IcebergDeleteSink.java:135-138` sets the same
raw-data-location/normalized pair).
### Release note
Fix `UPDATE` / `DELETE` on an Iceberg table whose storage location is
rewritten for the backend
(e.g. ADLS `abfss://`, OSS `oss://`) committing position-delete and
deletion-vector files under
Doris's internal normalized path, which left the table permanently
unreadable.
### Check List (For Author)
- Test
- [ ] Regression test
- [x] Unit Test
- [ ] 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
New / updated BE unit tests:
- `be/test/exec/sink/writer/iceberg/viceberg_delete_file_writer_test.cpp`
(new) — the commit data
carries the original path, not the normalized one; the two coincide when
the location was never
rewritten; the factory forwards both.
- `be/test/exec/sink/viceberg_delete_sink_test.cpp`
- `TestDeleteFilePathsKeepCatalogUriForManifest` — position-delete and
Puffin paths both keep the
`abfss://` prefix for the manifest and the `s3://` prefix for I/O, with
an identical file name.
- `TestDeletionVectorCommitsCatalogUriAndWritesNormalizedPath` —
end-to-end on the v3 lane: the
puffin bytes land under the write path while the commit data names the
catalog URI.
- `TestSharedPuffinCommitsCatalogUriForEveryBlob` — several blobs sharing
one puffin: *every*
commit entry carries the catalog URI, not just the first.
- `TestDeleteFilePathsFallBackWhenOneFormIsMissing` — one form absent
(empty, and genuinely
unset) means both paths agree. A rolling-upgrade guard; it passes
pre-fix too.
- The fixture at the top of the file previously used two *different*
directories for
`output_path` / `table_location`, which does not model anything the FE
can send. It now uses a
normalized/raw pair of one directory.
Not covered, stated plainly: there is no end-to-end test of the **v2
position-delete** lane
(`_write_position_delete_files`) equivalent to the v3 one — it needs a
`RuntimeState` and expression
harness, and I could not build the BE locally to validate such a test (no
thirdparty toolchain), so
I did not want to push an unverified test into your CI. A transposition of
the two arguments at the
`VIcebergDeleteFileWriterFactory::create_writer` call site would therefore
not be caught by a test
today, only by review. Happy to add it if you would like it in this PR.
No regression test: reproducing this needs a catalog on storage whose URI
the FE rewrites (ADLS or
OSS), which the local/S3 regression environments do not exercise — an
`s3://` location is
normalized to itself. I have a live ADLS Gen2 + Iceberg REST (Lakekeeper)
cluster and am happy to
run a patched build against the reproduction in #67544 and report back.
- Behavior changed:
- [x] Yes. Position-delete and deletion-vector files are now recorded in
the Iceberg manifest
with the catalog's own URI instead of Doris's internal normalized
path. On storage that is
not rewritten (HDFS, S3, local) the recorded path is unchanged.
- Does this need documentation?
- [x] No.
--
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]
