fix: Fall back to Spark for native Iceberg writes to gs:// through HadoopFileIO - #5935
zhangfengcdt wants to merge 2 commits into
Conversation
github-actions
Bot
added
bug
…doopFileIO HadoopFileIO takes its GCS configuration from fs.gs.*, which is not forwarded to the native writer, so a native write could run with a different storage identity or endpoint than the JVM writer. Decline a gs:// data location unless the FileIO Iceberg resolves for it is a GCSFileIO. Closes apache#5637
455bffb to
f014058
Compare
ResolvingFileIO.ioClass throws a NoClassDefFoundError when the GCS client libraries are absent, which aborted the suite on the Spark 3.4 and 3.5 CI jobs. Resolve the delegate the way the gate does and expect a decline when it cannot be resolved.
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Correctness
This addresses #5637: the native Iceberg writer accepts GCS data locations but its Hadoop configuration bridge translates only S3 settings. The new trigger correctly rejects a direct HadoopFileIO at a gs:// data location before native conversion, leaving the JVM write operator in place. It uses the table's data-location provider, so a separate write.data.path is covered; other schemes retain their existing behavior.
One P2 remains: ResolvingFileIO.ioClass(location) reports the scheme-mapped class rather than the instantiated delegate. Initialization can select HadoopFileIO while this helper reports GCSFileIO, admitting the same unsupported Hadoop GCS configuration. The inline comment gives a concrete Iceberg 1.8.1 case. This distinction also exists in the inspected 1.5.2, 1.10.0 and 1.11.0 resolver implementations.
Validation
Reviewed head 44b6a9bdf77b3a19a532b7d34178247236b7aa32 against base 683c7219e39928d267dc45c664d8656a4157da78, including all four changed files and current public discussion. CI checked out merge 298abf56e26de79dd99e65973d4d374eb6a22af6, with identical changed files and relevant dependency/dispatch sources. The new GCS tests passed on Spark 3.4, 3.5, 4.0 and 4.1; Spark 4.2 canceled them because Iceberg is unavailable on that profile. The checks report 53 successes and 10 skips.
A local component probe against the actual Iceberg 1.8.1 runtime confirmed that the invalid chunk-size property throws NumberFormatException, which is an IllegalArgumentException. The resulting delegate selection and native eligibility were traced through source; no local end-to-end GCS write was executed. The planning-only test helper agrees with CommandExecutionMode.SKIP in the maintained Spark 3.5 and 4.0 branches. Maintained 3.4, 4.1 and 4.2 source branches were unavailable, so CI evidence is separate from canonical source coverage.
Performance
The new work occurs during write planning: another data-location lookup, class-hierarchy checks, and reflective class resolution for GCS locations. The existing method cache avoids repeated method discovery; there is no new per-row or per-file data-path work. No material performance claim or measured regression is established here, so a throughput microbenchmark is not needed for this guard.
Design
Adding the check to the existing eligibility rules is a small, appropriate way to preserve JVM storage behavior without implementing an incomplete credential translation. Earlier storage-scheme and FileIO checks still fail closed. The resolver helper needs to inspect the effective delegate before this design fulfills its stated guarantee. The regression should compare the helper with that actual delegate and exercise its initialization fallback.
Abstraction & complexity
The shared scheme parser and package-visible decision function are modest additions that fit the existing structure. The helper's contract currently promises more than the Iceberg API provides; correcting that contract and testing it independently is the substantive simplification needed. No additional abstraction or broader property translation is required.
There was a problem hiding this comment.
Correctness
[P2] Inspect the instantiated ResolvingFileIO delegate
ioClass(location) does not return the delegate that opens the file: it maps the scheme to a class and calls Class.forName, without consulting ioInstances. The actual io(location) separately initializes that class and catches IllegalArgumentException to cache a HadoopFileIO. See Iceberg 1.8.1.
For example, with the GCS libraries present, gcs.channel.read.chunk-size-bytes=invalid makes 1.8.1's GCSFileIO.initialize throw NumberFormatException, so JVM writes use the Hadoop fallback. This helper still returns GCSFileIO, and the new gate admits a native write that drops that delegate's fs.gs.* configuration. This recreates the identity/endpoint mismatch this PR is intended to prevent.
Please inspect the actual delegate (including initialization fallback), or decline the unresolved case. Add a regression where the scheme-mapped class is GCSFileIO but the effective delegate is HadoopFileIO; deriving the test expectation from this same helper misses that distinction.
2 participants
Which issue does this PR close?
Closes #5637.
Rationale for this change
The native Iceberg write path accepts
gs://data locations, but the only bridge from the HadoopConfigurationinto the nativeFileIOtranslatesfs.s3a.*keys. AHadoopFileIOtakes its GCS credentials, endpoint and project fromfs.gs.*, so none of that reached the native writer and it could resolve a different storage identity or endpoint than the JVM writer would. Following the issue's recommendation, this change fails closed for that combination rather than attempting anfs.gs.*togcs.*bridge, which is not a simple key rename.What changes are included in this PR?
CometIcebergNativeWrite: a new trigger rule that declines ags://data location unless theFileIOopening it is aGCSFileIO. The decision is a small package-visible function so it can be unit-tested directly.IcebergReflection: a helper that returns the effectiveFileIOclass for a location. AResolvingFileIOis asked for its delegate viaioClass(location), so a table that resolves toGCSFileIOstays eligible. Reflection failures fail closed.iceberg-writes.md: documents the new condition.How are these changes tested?
New and updated tests in
CometIcebergWriteDetectionSuite