mmadzia-sfsh commented on issue #67544:
URL: https://github.com/apache/doris/issues/67544#issuecomment-5563458105
Thanks for the triage — the pointer to `table_location` changed the shape of
the fix for the better.
PR: #67552 (against `master`).
I dropped my original "add FE plumbing" proposal and took your suggestion
instead: `TIcebergDeleteSink`
already carries both forms of the same directory, so the BE now generates
one file *name* and joins it
to both bases — `output_path` for filesystem I/O, `table_location` for
`TIcebergCommitData.file_path`.
That keeps it backend-only: no thrift change, correct against every
already-deployed FE, and it
backports to `branch-4.1` unchanged (its legacy
`planner/IcebergDeleteSink.java:135-138` sets the same
raw/normalized pair).
Also taken from your triage:
- **The format-v3 deletion-vector / Puffin lane is fixed in the same PR.**
You were right that
`_generate_puffin_file_path()` had the identical defect; it is
code-identical to the v2 lane and
would have failed the same way. Still not runtime-reproduced on my side —
our tables are v2 — so it
is covered by unit tests only.
- **BE tests asserting the filesystem path stays normalized while the commit
metadata keeps the raw
URI**, on both lanes, including a multi-blob case proving every commit
entry that shares one puffin
gets the catalog URI rather than just the first.
The invariant the fix depends on, stated explicitly so it is easy to check:
`output_path` and
`table_location` are the same directory in two URI forms — both derived from
one
`resolveLocationFields(table, schemaContext.getDataLocation())` call in
`IcebergWritePlanProvider#buildDeleteSink` and `#buildMergeSink` — and
`IcebergWritePlanProviderTest.planWriteBuildsDeleteSinkWithTableDerivedFields`
already pins both
literal values. If that ever stops holding, the fix is wrong, so it is worth
a reviewer's attention.
Two things I want to be upfront about rather than have you discover them:
1. **I could not build the BE locally**, so nothing in that PR has been
compiled — only `clang-format`
(16) and `build-support/check-build-hygiene.sh` have run against it.
Please treat the first
`run buildall` as the real first compile.
2. **There is no end-to-end test of the v2 position-delete lane**, only of
the v3 DV lane. Writing one
needs a `RuntimeState` and expression harness I could not validate
without a build, and I did not
want to push an unverified test into your CI. Happy to add it in this PR
if you would like it.
There is also no `regression-test/` case: reproducing this needs a catalog
on storage whose URI the FE
rewrites (ADLS or OSS), which I do not think CI can reach — an `s3://`
location normalizes to itself.
Offer still stands from the original report: I have a live ADLS Gen2 +
Iceberg REST cluster and can run
a patched build against the exact reproduction above and report back, if
someone can point me at a
build artifact or tell me which branch to build.
--
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]
