Repository navigation
Conversation
beam pipe --from SOURCE --to SINK reads CSV, JSON, Parquet or Avro files and writes them as CSV, JSON or Avro by building a two-step Beam YAML pipeline. The format comes from the file extension (also behind a glob or shard suffix such as users.parquet*) unless a flag sets it. Existing output is never overwritten, the files written are listed, and --dry-run prints the pipeline as YAML to save and run with beam run.
There was a problem hiding this comment.
📋 Review Summary
This PR cleanly adds beam pipe --from SOURCE --to SINK (including --dry-run, format inference/overrides, and overwrite protection) on top of the existing Beam YAML CLI runner. However, there are two issues in how _refuse_existing_output and _matching_files interact with FileSystems for remote URIs (gs://, s3://, azfs://, hdfs://).
🔍 General Feedback
_refuse_existing_outputcurrently runs beforePipelineOptionsare parsed and registered withFileSystems.set_options(...), leavingFileSystems._pipeline_optionsasNonein a fresh CLI process._matching_filesonly catchesValueErrorfromFileSystems.match, whereas missing filesystem extras ([gcp],[aws],[azure],[hadoop]) and remote I/O failures raiseBeamIOError.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
|
||
| # A dry run makes every check a real run makes, including building the | ||
| # pipeline, so that it predicts whether `beam run` on its output will work. | ||
| _refuse_existing_output(pipe.output_prefix) |
There was a problem hiding this comment.
🔴 Critical: _refuse_existing_output(pipe.output_prefix) is called before _run_yaml parses pipeline_args into PipelineOptions and constructs beam.Pipeline(options=options, ...) (which calls FileSystems.set_options(self._options)).
In a fresh CLI invocation, FileSystems._pipeline_options is still None at this point, which breaks remote filesystems even when their extras are installed:
s3://:S3IO.__init__(options=None)raisesValueError('Must provide one of client or options'), which_matching_filescatches and misreports asEXIT_MISSING_PACKAGE.azfs://:BlobStorageIO.__init__(pipeline_options=None)raisesAttributeError: 'NoneType' object has no attribute 'view_as'.hdfs://:HadoopFileSystem.__init__(pipeline_options=None)raisesValueError('pipeline_options is not set'), ignoring--hdfs_host/--hdfs_port/--hdfs_userinpipeline_args.gs://:GcsIOfalls back to an emptyPipelineOptions(), ignoring GCP flags inpipeline_args(such as--projector--impersonate_service_account).
Move _refuse_existing_output(output_prefix) into _run_yaml after pipeline = beam.Pipeline(options=options, display_data=display_data) (re-raising _CliError before except Exception in that block) so FileSystems is configured with the parsed PipelineOptions first.
| os.sep if '://' not in prefix and not os.path.dirname(prefix) else '') | ||
| try: | ||
| match = FileSystems.match([dot + prefix + '*'])[0] | ||
| except ValueError as e: |
There was a problem hiding this comment.
🟠 High: _matching_files only catches ValueError, but FileSystems.match raises BeamIOError both when a remote filesystem extra ([gcp], [aws], [azure], [hadoop]) is not installed and when a remote listing fails (e.g., auth errors or nonexistent buckets):
GCSFileSystem,S3FileSystem,BlobStorageFileSystem, andHadoopFileSystemare all imported unconditionally inapache_beam/io/filesystems.pybecause their underlying modules (gcsio,s3io,blobstorageio,hadoopfilesystem) guard third-party imports withtry...except ImportError.- When the extra is missing at runtime,
GcsIO/S3IO/BlobStorageIOraiseRuntimeError('<X> dependencies are not installed...')(orNameError) inside_list(), andHadoopFileSystem.__init__raisesImportError('Failed to import hdfs...'), all of whichFileSystem.match/FileSystems.get_filesystemwrap intoBeamIOError.
Catch BeamIOError here as well (inspecting its nested exception details or message to distinguish missing dependencies -> EXIT_MISSING_PACKAGE from general I/O errors -> EXIT_USAGE/EXIT_PIPELINE_FAILED, while respecting --verbose) so FileSystems.match failures do not crash with an unhandled traceback.
|
Assigning reviewers: R: @shunping for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41040 +/- ##
============================================
+ Coverage 57.44% 57.47% +0.02%
Complexity 2492 2492
============================================
Files 1203 1204 +1
Lines 171109 171201 +92
Branches 2103 2103
============================================
+ Hits 98294 98392 +98
+ Misses 70161 70155 -6
Partials 2654 2654
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
beam pipe --from SOURCE --to SINK reads CSV, JSON, Parquet or Avro files and writes them as CSV, JSON or Avro by building a two-step Beam YAML pipeline. The format comes from the file extension (also behind a glob or shard suffix such as users.parquet*) unless a flag sets it. Existing output is never overwritten, the files written are listed, and --dry-run prints the pipeline as YAML to save and run with beam run.