storage: Honor write.data.path when placing Iceberg data files

[patrickwwbutler]

Top of a three-PR stack. Base is patrick/iceberg-vended (#38181), which sits on patrick/iceberg-unity-catalog (#38373). Merge order: #38373, #38181, then this.

The Iceberg sink hardcoded its data-file location to <table location>/data, ignoring the write.data.path property the catalog advertises. Catalogs that manage their own storage layout reject a commit whose data files sit outside that path.

How this surfaced

Unity Catalog's Iceberg tables are backed by Delta, so an Iceberg commit must be translated into a Delta commit that registers the incoming data files. Files written under <location>/_iceberg/data are outside the path it manages, and the commit fails server-side with nothing in the response identifying the cause:

500 Internal Server Error
{"error":{"message":"Could not process table operation. [ErrorCode: 2012]",
          "type":"ServiceFailureException","code":500}}

The sink retries five times and then stalls, so the symptom is a sink that writes data files happily and never commits.

Comparing against a client whose commits Unity Catalog accepts (DuckDB, writing to the same catalog) showed the difference: its data file sat at the table root, matching the advertised write.data.path, and its commit produced a new _delta_log entry. Ours sat a directory deeper under _iceberg/data, and no Delta commit was ever created.

The request body was not the problem — with the mz-* snapshot properties removed and the assert-ref-snapshot-id requirement dropped, an otherwise byte-standard commit still failed. Only the file placement mattered.

The change

Prefer write.data.path, then write.folder-storage.path, and fall back to the previous <location>/data heuristic only when neither is set. That keeps the S3 Tables workaround intact, since it exists precisely for a catalog that does not advertise a data path.

DefaultLocationGenerator::new reads the same two properties, but its fallback lacks the S3 Tables correction, so the choice is made explicitly here rather than delegated to the library.

Also logs the resolved location at debug, because a wrong data path otherwise only manifests as an opaque catalog error at commit time.

Testing

No new automated test. The existing test/iceberg Polaris setup exercises only the fallback path — Polaris does not set write.data.path, so those workflows cover the unchanged branch and should be unaffected. Verifying the new branch needs a catalog that advertises a data path, which the harness has no way to stand up today. Confirmed manually against a Unity Catalog managed table.

Release notes

This release will fix Iceberg sinks failing to commit against catalogs that manage their own data-file layout, such as Databricks Unity Catalog.

🤖 Generated with Claude Code

[peterdukelarsen]

Just to confirm this doesn't show up for AWS? What testing/validation did we have covering this

[peterdukelarsen]

style nit: Can we shorten this a bit? Too verbose for my taste. Not a blocking issue though.

Proposed replacement:

// Unity Catalog required respecting "write.data.path" while AWS does
// not provide "write.data.path" and required a hacky workaround.

[def-]

QA LLM Review

1. MEDIUM -- write.data.path is used verbatim, so a trailing slash writes data files to a key that does not match the path recorded in the manifest

src/storage/src/sink/iceberg.rs:1560

write.data.path is now taken from catalog-advertised table properties and passed straight to DefaultLocationGenerator::with_data_location, which joins with a literal /. If the property carries a trailing slash, every data file path gets an empty segment (s3://b/p//mz_data-00000-<uuid>.parquet). OpenDAL collapses the // when it writes, but the manifest records the un-collapsed string, so the committed snapshot references an object key that was never created. The sink reports success and no error appears anywhere in Materialize; readers that do not normalize duplicate slashes (Trino's native S3 filesystem, DuckDB's httpfs, anything driving the S3 API directly) get NoSuchKey for those files.

Details

The chain, all in the pinned iceberg-rust / opendal revisions:

• DefaultLocationGenerator::generate_location → format!("{}/{}", self.data_location, file_name) (crates/iceberg/src/writer/file_writer/location_generator.rs). No trailing-slash handling.
• RollingFileWriter::new_output_file → file_io.new_output(<that string>); OutputFile stores the string as given.
• Write path: OpenDalStorage::writer → create_operator strips only the s3://<bucket>/ prefix, leaving p//f.parquet, then op.writer(..) → opendal::Operator::writer_with → normalize_path, which filters empty segments (opendal-0.55.0/src/raw/path.rs:109-113, documented as "Internal // will be replaced by /"). Actual key: p/f.parquet.
• ParquetWriter::close builds the DataFile with self.output_file.location().to_string() (parquet_writer.rs:675), i.e. the un-normalized p//f.parquet. That is what lands in the manifest.

The old code derived the location from table_metadata.location() via format!("{}/data", ..), so a trailing slash was much less likely to reach the generator. The new code reads a free-form property, and this is exactly why Java Iceberg's LocationProviders wraps the configured value in LocationUtil.stripTrailingSlash(..) before handing it to the location provider. iceberg-rust does not, so the sink has to.

Fix, at the point the property is read:

let data_location = table_metadata
    .properties()
    .get("write.data.path")
    .or_else(|| table_metadata.properties().get("write.folder-storage.path"))
    // The property is free-form. A trailing slash would put an empty segment in
    // every data file path: object stores treat `a//b` and `a/b` as distinct keys,
    // and the write path normalizes the slash away while the manifest keeps it,
    // leaving the committed snapshot pointing at a key that was never written.
    .map(|path| path.trim_end_matches('/').to_string())
    .unwrap_or_else(|| {
        // ... unchanged S3 Tables fallback
    });