fix(filesystem): fail missing concrete sources - #265
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #265 +/- ##
==========================================
+ Coverage 57.63% 57.80% +0.16%
==========================================
Files 227 227
Lines 10489 10531 +42
==========================================
+ Hits 6045 6087 +42
Misses 4444 4444 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
0e55712 to
5899f54
Compare
- Fail ingestion when a concrete filesystem selection matches no file instead of reporting false success. - Keep unmatched globs, zero-row files, and unchanged incremental reruns valid by counting matches before incremental filtering. - Apply the behavior across local, remote, and locator-based filesystem sources, with regression and integration coverage. - Document concrete-path requirements and supported glob syntax.
5899f54 to
df73a80
Compare
amotl
left a comment
amotl
left a comment
There was a problem hiding this comment.
Excellent, thank you so much for improving this detail thoroughly. 💯
I just found one nit, but it can easily be addressed in a later iteration.
| def has_glob_magic(path: str) -> bool: | ||
| """Return whether ``path`` contains fsspec's ``*``, ``?``, or ``[`` syntax.""" | ||
| return any(char in path for char in _GLOB_CHARS) | ||
|
|
||
|
|
There was a problem hiding this comment.
This other patch just uses glob.has_magic from Python's standard library. Is it applicable to also use it here?
There was a problem hiding this comment.
Yes, applicable, done in 8cf4658. glob.has_magic tests the same *?[ class the local constant did, so the classification is unchanged (verified over the concrete/*/?/[ab]/{a,b}/**/empty/literal-# cases the predicate tests already cover), and one less local copy of the rule to keep in sync.
Worth noting the ? caveat from the review you linked does not bite here, and the asymmetry is why. In source_selects_single_file, magic means glob means an unmatched selection still succeeds, so counting ? as magic is the conservative direction; dropping it from the set (as fsspec/filesystem_spec#2080 ended up doing for URLs) would reclassify bar/baz?.csv as concrete and turn it into a hard failure. The query-delimiter problem that review raises is handled separately and more narrowly here: a trailing tail made entirely of key=value segments is stripped as connection parameters before classification, so ? only survives when it is genuinely part of the selection.
Use the standard library's glob.has_magic instead of a local copy of the *?[ character class, so concrete-vs-glob classification tracks the rule Python and fsspec globbing already share rather than one this package has to keep in sync by hand. Behaviour is unchanged: has_magic tests the same three characters, and brace expansion stays non-magic, matching fsspec, which does not expand braces.
2 participants
Summary
Fixes #260.
Review
Changes
NoFilesFoundError.Test plan