Skip to content

Fix ReferenceFileSystem.cat for unsorted lists of paths #1158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 19, 2023

Conversation

ap--
Copy link
Contributor

@ap-- ap-- commented Jan 19, 2023

Hello everyone,

here's a potential fix for the issue revealed in zarr-developers/zarr-python#1324

I added a test to check the order of returned data when using FSMap.getitems
Without the fix, it errors with

    def test_mapping_getitems(m):
        m.pipe({"a": b"A", "b": b"B"})
    
        refs = {
            "a": ["a"],
            "b": ["b"],
        }
        h = fsspec.filesystem("memory")
        fs = fsspec.filesystem("reference", fo=refs, fs=h)
        mapping = fs.get_mapper("")
>       assert mapping.getitems(["b", "a"]) == {"a": b"A", "b": b"B"}
E       AssertionError: assert {'a': b'B', 'b': b'A'} == {'a': b'A', 'b': b'B'}
E         Differing items:
E         {'b': b'A'} != {'b': b'B'}
E         {'a': b'B'} != {'a': b'A'}
E         Full diff:
E         - {'a': b'A', 'b': b'B'}
E         + {'a': b'B', 'b': b'A'}

Regarding the fix: I'm not sure if this should be solved differently, since the non-merging codepath is probably the more common. Also I haven't tested if DFReferenceFileSystem.cat has the same issue...

Cheers,
Andreas 😃

@martindurant
Copy link
Member

Don't worry about DFReferenceFileSystem, it is strictly experimental.

@ap-- ap-- changed the title Fix reference fs cat path list Fix ReferenceFileSystem.cat for unsorted lists of paths Jan 19, 2023
@martindurant martindurant merged commit 6fbdbce into fsspec:master Jan 19, 2023
@ap-- ap-- deleted the fix-reference-fs-cat-path-list branch January 20, 2023 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants