fix: fall back to Spark for native Iceberg writes to gs:// through HadoopFileIO - #5935
zhangfengcdt wants to merge 4 commits into
Conversation
455bffb to
f014058
Compare
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.
142db33 to
f12167c
Compare
sunchao
left a comment
There was a problem hiding this comment.
The previous delegate-selection finding is fixed. The gate now inspects the delegate instantiated by ResolvingFileIO.io(location), and reflection failures still fail closed.
One new P2 remains in the regression test. The invalid chunk-size property does not make GCSFileIO.initialize fail on Iceberg 1.10/1.11. I independently reproduced this with both real runtimes and the exact current Comet helper. It returns GCSFileIO, making the new HadoopFileIO assertion false. Details are inline.
The focused GCS cases passed in the current Spark 3.4/3.5/4.0/4.1 CI profiles. Spark 4.2 canceled them because Iceberg was unavailable. Current CI has finished with 49 successful and 12 skipped checks, but three failed: the Maven-bootstrap HTTP 403, the resulting missing shard evidence, and aggregate Required Checks. No full local Spark suite or native GCS write was run.
…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
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.
ResolvingFileIO.ioClass only maps the scheme to a class; io(location) instantiates it and falls back to HadoopFileIO when that fails, so the gate must inspect the instantiated delegate.
Iceberg 1.10+ defers gcs.* parsing to client construction, so an unparseable chunk size no longer fails GCSFileIO.initialize there.
3c6e055 to
251fb7d
Compare
sunchao
left a comment
There was a problem hiding this comment.
The fixture P2 is fixed in 251fb7d4a69f0602b86f1ac816868f202233a092 (reviewed against 4479e722efebd33620f29a246f7863bd04f5df1a). On Iceberg 1.10+, it now compares the delegate with an unaffected control and expects compatibility when Iceberg retains GCSFileIO. The previous production delegate-selection fix is unchanged. No new or remaining P1/P2 findings.
I reran the exact helper, fixture and guard component checks with real Iceberg 1.5.2, 1.8.1, 1.10.0 and 1.11.0 runtimes. All four revised assertions pass. The original 1.10/1.11 reproduction still makes the old assertion false and now passes the revised one. Older runtimes resolve to HadoopFileIO on this classpath. Missing Google authentication classes prevent crediting direct eager-GCS-initialization coverage.
The focused GCS cases passed in current Spark 4.0 and 4.1 CI. Spark 4.2 skipped them because Iceberg is unavailable. Executed merge ab0aa7bd has parents [base, head] and an identical tree to this head. Native artifact upload/download digests match. CI still has 14 running checks, alongside 36 successful and 11 skipped, at September 15, 19:37 UTC. No full local Spark suite or native GCS write was run.
comphead
left a comment
There was a problem hiding this comment.
Thanks @zhangfengcdt looks good, it would be beneficial to update PR description to include Spark fallback message when dealing with GS
Sure, thanks for the review and I have updated the pr description to include the fallback message. |
sunchao
left a comment
There was a problem hiding this comment.
The added fallback examples match the current gate at 251fb7d4. My existing approval stands. The source is unchanged, and the resolved fixture P2 remains fixed.
Please align two description details with the reviewed behavior: the helper invokes io(location) to inspect the instantiated delegate, and the resolver tests exercise delegate resolution and write planning. On pre-1.10 runtimes, their passing result alone does not prove that GCSFileIO.initialize ran. The prior 1.5.2/1.8.1 component controls lacked Google authentication classes, so that direct initialization coverage remains unverified.
Required Checks is now green: 53 successful checks and 12 skips. The completed Spark 3.4/3.5 scan jobs also passed the focused GCS cases on merge ab0aa7bd, whose full tree matches this head. No unchanged broad suites were rerun locally, and no native GCS write is claimed.
I have updated the following to align with the reviewed behavior.
AND
|
sunchao
left a comment
There was a problem hiding this comment.
The io(location) correction matches the implementation, and the description now acknowledges that direct pre-1.10 initialization-failure coverage remains unverified. My existing approval stands on unchanged head 251fb7d4. No new or remaining P1/P2 findings.
Missing Google authentication classes were directly observed in the component controls. I have not independently verified that cause in the hosted CI classpath, and passing resolver tests alone do not establish which failure path ran. Required Checks remains green. No unchanged broad suites were rerun.
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. For aResolvingFileIOit invokesio(location)and inspects the delegate Iceberg actually instantiates, including the fallback toHadoopFileIOwhen the scheme'sFileIOcannot be loaded or initialized, so a table that resolves toGCSFileIOstays eligible. Reflection failures fail closed.iceberg-writes.md: documents the new condition.Spark fallback message
When the gate declines, the write runs on Spark's Iceberg writer and Comet's extended EXPLAIN reports one of these reasons:
The effective
FileIOis not aGCSFileIO(typically aHadoopFileIO):gs:// data location gs://bucket/warehouse/db/t/data is written through org.apache.iceberg.hadoop.HadoopFileIO, whose fs.gs.* Hadoop configuration is not forwarded to the native writer
The delegate could not be resolved (for example, a
ResolvingFileIOwhoseGCSFileIOfails to construct):could not resolve the FileIO for the gs:// data location gs://bucket/warehouse/db/t/data
How are these changes tested?
New and updated tests in
CometIcebergWriteDetectionSuite:gs://table under the Hadoop catalog to beCompatiblenow covers onlymemory; that expectation was the bug.HadoopFileIO+gs://. The write is planned but not executed, so the test does not touch the network.GCSFileIOaccepted,HadoopFileIOrejected, unresolved rejected.ResolvingFileIOdelegate resolution and write planning. One uses an unparseablegcs.channel.read.chunk-size-bytes, which failsGCSFileIO.initializeon Iceberg versions before 1.10 and is ignored at initialization on 1.10+; the expectation is version-aware. In CI the pre-1.10 profiles lack the Google authentication classes, soGCSFileIOnever constructs there and the test passes through the class-load fallback. Direct coverage of the initialization-failure path therefore remains unverified in CI; it holds by source inspection of 1.5.2 and 1.8.1.