[#38059] Fix GCS glob matching to support ** and / in object names - #38099
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where GCS filesystem glob patterns were incorrectly filtered due to the standard library's path-aware matching logic. By implementing a custom regex-based matching system, the GCS connector now properly handles object names containing slashes and supports recursive globbing, ensuring consistent behavior across different Apache Beam SDKs. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
utafrali
left a comment
There was a problem hiding this comment.
The core approach of replacing filepath.Match with a custom globToRegex is correct and well-motivated. The main issue is that ? is translated to . (matches /) while * is explicitly restricted to [^/]*, creating an inconsistency that will surprise callers. There are also minor concerns around the hand-maintained isRegexSpecial list (use regexp.QuoteMeta instead) and the silent swallowing of malformed [ patterns.
|
Assigning reviewers: R: @jrmccluskey for label go. 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). |
|
@utafrali all comments addressed, thanks! |
|
Reminder, please take a look at this pr: @jrmccluskey |
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request fixes GCS filesystem glob matching in the Go SDK to correctly handle slashes in object names and support recursive matching with **, aligning its behavior with the Python and Java SDKs. The implementation replaces filepath.Match with a custom globToRegex translator and updates the List operation to use regex-based filtering. Feedback was provided to optimize the generated regular expression by using non-capturing groups for recursive path segments.
jrmccluskey
left a comment
There was a problem hiding this comment.
Modify .github/trigger_files/beam_PostCommit_Go.json to trigger the post-commit, which will run all of the Go tests that interact with Dataflow / GCS.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Thank you @jrmccluskey I did so - apologies for the delay in actioning it. |
|
Reminder, please take a look at this pr: @jrmccluskey |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @lostluck for label go. Available commands:
|
|
Reminder, please take a look at this pr: @lostluck |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @shunping for label go. Available commands:
|
|
Reminder, please take a look at this pr: @shunping |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @lostluck for label go. Available commands:
|
|
@jrmccluskey could you take another look at this one? |
|
I don't know how I missed the follow-up ping, let me trigger the checks and we can get this moving |
|
Thank you for your patience! |
…mes (apache#38099) * fix GCS filesystem glob matching to handle / in object names and support ** * update CHANGES.md * update CHANGES.md * address comments * Update sdks/go/pkg/beam/io/filesystem/gcs/gcs.go Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * modify beam_PostCommit_Go.json as required --------- Co-authored-by: corda.ilaria@gmail.com <ilariac691@gmail.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Jack McCluskey <34928439+jrmccluskey@users.noreply.github.com>
Fix GCS filesystem glob matching to handle
/in object names and support**Fixes #38059
Summary
The GCS filesystem's List() method was using filepath.Match() to filter objects, which incorrectly treats
/as a path separator. Since GCS object names are flat (with/being just another character), patterns likegs://bucket/**failed to match objects containing/.Problem
/as a path separator, so*cannot match across/**for recursive matchingfileio.MatchFiles(scope, "gs://my-bucket/**")silently excluded objects likedir/subdir/file.txtSolution
Replaced filepath.Match with a custom globToRegex() function:
*→ matches any characters except/(single path segment)**→ matches any characters including/(recursive)**/→ matches zero or more path segmentsThis aligns the Go SDK with the Python and Java SDKs.
Testing
Added unit tests for globToRegex() and List() with nested object names.
addresses #123), if applicable.