-
Notifications
You must be signed in to change notification settings - Fork 22
List group members level by level instead of enumerating all keys #92
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
Open
konstibob
wants to merge
1
commit into
zarr-developers:main
Choose a base branch
from
konstibob:fix-group-list-levelwise
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3,16 +3,32 @@ | |||||
| import dev.zarr.zarrjava.ZarrException; | ||||||
| import dev.zarr.zarrjava.store.FilesystemStore; | ||||||
| import dev.zarr.zarrjava.store.StoreHandle; | ||||||
| import dev.zarr.zarrjava.utils.Utils; | ||||||
|
|
||||||
| import javax.annotation.Nonnull; | ||||||
| import javax.annotation.Nullable; | ||||||
| import java.io.IOException; | ||||||
| import java.nio.file.Path; | ||||||
| import java.nio.file.Paths; | ||||||
| import java.util.AbstractMap; | ||||||
| import java.util.Arrays; | ||||||
| import java.util.Collections; | ||||||
| import java.util.HashSet; | ||||||
| import java.util.List; | ||||||
| import java.util.Map; | ||||||
| import java.util.Objects; | ||||||
| import java.util.Set; | ||||||
| import java.util.stream.Collectors; | ||||||
| import java.util.stream.Stream; | ||||||
|
|
||||||
| public abstract class Group extends AbstractNode { | ||||||
|
|
||||||
| /** | ||||||
| * Keys that hold metadata of the group itself and never point at a child node. | ||||||
| */ | ||||||
| private static final Set<String> METADATA_KEYS = Collections.unmodifiableSet( | ||||||
| new HashSet<>(Arrays.asList(ZARR_JSON, ZARRAY, ZATTRS, ZGROUP))); | ||||||
|
|
||||||
| protected Group(@Nonnull StoreHandle storeHandle) { | ||||||
| super(storeHandle); | ||||||
| } | ||||||
|
|
@@ -70,13 +86,94 @@ public Node get(String key) throws ZarrException, IOException { | |||||
| return get(new String[]{key}); | ||||||
| } | ||||||
|
|
||||||
| public abstract Stream<Node> list(); | ||||||
| /** | ||||||
| * Lists the immediate children (arrays and subgroups) of this group. | ||||||
| * <p> | ||||||
| * This costs a single listing request on the underlying store, plus one metadata read per | ||||||
| * child. Keys that do not hold a Zarr node are skipped. | ||||||
| * | ||||||
| * @return a stream of the direct children of this group | ||||||
| * @throws UnsupportedOperationException if the underlying store does not support listing | ||||||
| */ | ||||||
| public Stream<Node> members() { | ||||||
| return childKeys(new String[0]).parallelStream() | ||||||
| .map(this::openChild) | ||||||
| .filter(Objects::nonNull) | ||||||
| .collect(Collectors.toList()) | ||||||
| .stream(); | ||||||
| } | ||||||
|
|
||||||
| public Node[] membersAsArray() { | ||||||
| try (Stream<Node> nodeStream = members()) { | ||||||
| return nodeStream.toArray(Node[]::new); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Lists all descendants (arrays and groups) of this group, at any depth. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| * <p> | ||||||
| * The group hierarchy is walked one level at a time, so only group keys are listed and chunk | ||||||
| * keys are never enumerated. Descending into an array is not necessary and does not happen. | ||||||
| * | ||||||
| * @return a stream of all descendants of this group, excluding the group itself | ||||||
| * @throws UnsupportedOperationException if the underlying store does not support listing | ||||||
| */ | ||||||
| public Stream<Node> list() { | ||||||
| return listDescendants(new String[0]); | ||||||
| } | ||||||
|
|
||||||
| public Node[] listAsArray() { | ||||||
| try (Stream<Node> nodeStream = list()) { | ||||||
| return nodeStream.toArray(Node[]::new); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| private Stream<Node> listDescendants(String[] prefix) { | ||||||
| List<Map.Entry<String[], Node>> children = childKeys(prefix).parallelStream() | ||||||
| .map(key -> new AbstractMap.SimpleEntry<String[], Node>(key, openChild(key))) | ||||||
| .collect(Collectors.toList()); | ||||||
|
|
||||||
| return children.stream().flatMap(child -> { | ||||||
| Node node = child.getValue(); | ||||||
| if (node == null) { | ||||||
| // Not a node itself, but it may still contain nodes further down. | ||||||
| return listDescendants(child.getKey()); | ||||||
| } | ||||||
| if (node instanceof Group) { | ||||||
| return Stream.concat(Stream.of(node), listDescendants(child.getKey())); | ||||||
| } | ||||||
| return Stream.of(node); | ||||||
| }); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Lists the keys directly below {@code prefix} that may hold a child node, relative to this | ||||||
| * group. | ||||||
| */ | ||||||
| private List<String[]> childKeys(String[] prefix) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| try (Stream<String> children = storeHandle.resolve(prefix).listChildren()) { | ||||||
| return children | ||||||
| .filter(name -> !METADATA_KEYS.contains(name)) | ||||||
| .map(name -> Utils.concatArrays(prefix, new String[]{name})) | ||||||
| .collect(Collectors.toList()); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Opens the node at {@code key}, or returns null if there is no node there. | ||||||
| */ | ||||||
| @Nullable | ||||||
| private Node openChild(String[] key) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| try { | ||||||
| return get(key); | ||||||
| } catch (IOException e) { | ||||||
| throw new RuntimeException( | ||||||
| "Failed to read node metadata for key '" + String.join("/", key) + "': " + e.getMessage(), e); | ||||||
| } catch (ZarrException e) { | ||||||
| throw new RuntimeException( | ||||||
| "Failed to parse node metadata for key '" + String.join("/", key) + "': " + e.getMessage(), e); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| public abstract GroupMetadata metadata(); | ||||||
| } | ||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks the v2 vs. v3 abstraction. Could this be delegated to the subclasses?