From b3a60b7cf8536257abc0e35b8caa8c928672903f Mon Sep 17 00:00:00 2001 From: Patrick Butler Date: Fri, 21 Aug 2026 16:00:59 -0400 Subject: [PATCH] storage: Honor write.data.path when placing Iceberg data files The Iceberg sink hardcoded its data-file location to `/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. Unity Catalog is the case that surfaced this. Its Iceberg tables are backed by Delta, so an Iceberg commit has to be translated into a Delta commit that registers the incoming data files. Files written under `/_iceberg/data` are outside the path it manages, and the commit fails server-side: 500 Internal Server Error {"error":{"message":"Could not process table operation. [ErrorCode: 2012]", "type":"ServiceFailureException","code":500}} Nothing in the request identifies the cause, and the sink retries until it stalls. Comparing against a client whose commits Unity Catalog accepts showed its data files at the table root, matching the `write.data.path` the table advertises, while ours sat a directory deeper. Prefer `write.data.path`, then `write.folder-storage.path`, and only fall back to the previous `/data` heuristic 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 that correction, so the choice is explicit here rather than delegated. Also logs the resolved location at debug, since a wrong data path otherwise only manifests as an opaque catalog error at commit time. Co-Authored-By: Claude Opus 5 (1M context) --- src/storage/src/sink/iceberg.rs | 37 ++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/src/storage/src/sink/iceberg.rs b/src/storage/src/sink/iceberg.rs index aed8ad817272f..e1f6063dfdc9b 100644 --- a/src/storage/src/sink/iceberg.rs +++ b/src/storage/src/sink/iceberg.rs @@ -1548,16 +1548,33 @@ fn write_data_files<'scope, H: EnvelopeHandler + 'static>( .context("Failed to merge Materialize metadata into Iceberg schema")?, ); - // WORKAROUND: S3 Tables catalog incorrectly sets location to the metadata file path - // instead of the warehouse root. Strip off the /metadata/*.metadata.json suffix. - // No clear way to detect this properly right now, so we use heuristics. - let location = table_metadata.location(); - let corrected_location = match location.rsplit_once("/metadata/") { - Some((a, b)) if b.ends_with(".metadata.json") => a, - _ => location, - }; - - let data_location = format!("{}/data", corrected_location); + // A catalog that manages where data files live advertises it through + // `write.data.path`. Honor it: catalogs backing an Iceberg table with + // their own storage layout reject a commit whose data files sit outside + // that path. Unity Catalog, for one, has to register the files in the + // Delta log that actually backs the table, and answers a commit + // referencing files under `/data` with a 500. + // + // `DefaultLocationGenerator::new` reads these same properties, but its + // fallback misses the S3 Tables correction below, so choose explicitly. + let data_location = table_metadata + .properties() + .get("write.data.path") + .or_else(|| table_metadata.properties().get("write.folder-storage.path")) + .cloned() + .unwrap_or_else(|| { + // WORKAROUND: S3 Tables catalog incorrectly sets location to the + // metadata file path instead of the warehouse root. Strip off the + // /metadata/*.metadata.json suffix. No clear way to detect this + // properly right now, so we use heuristics. + let location = table_metadata.location(); + let corrected_location = match location.rsplit_once("/metadata/") { + Some((a, b)) if b.ends_with(".metadata.json") => a, + _ => location, + }; + format!("{}/data", corrected_location) + }); + debug!(%data_location, "iceberg sink data file location"); let location_generator = DefaultLocationGenerator::with_data_location(data_location);