Skip to content
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

Url attributes #91

Closed
wants to merge 75 commits into from
Closed

Url attributes #91

wants to merge 75 commits into from

Conversation

cmhulbert
Copy link
Contributor

No description provided.

bogovicj and others added 30 commits November 15, 2022 09:51
@cmhulbert
Copy link
Contributor Author

Regarding Open Question 6:

Ideally, the multiple setAttributes behavior would be identical to multiple calls of the singular setAttribute behaviors. The reason I didn't implement it that way is because that is a departure from the existing behavior, which does not parse attribute keys as paths. This way, the setAttributes behavior works as before, and does not break existing functionality, while setAttribute does extend the previous functionality, albeit in a breaking way.

@bogovicj
Copy link
Contributor

bogovicj commented Feb 17, 2023

Notes from discussion with @axtimwalde and @cmhulbert

(1) + (3)

/ in attribute keys are allowed.

  • Escape them in paths with \.
    • The path for key "/" is "\/"
      • The path for key "\" is "\\"

Whitespace in keys is allowed

(2)

Indexing out of an arrays bounds returns null

(5)

Hdf5 and Zarr will just have different behavior with respect to dataset attributes. This is okay.

(6)

Change methods that return / accept Map<String,?>to use paths

  • getAttributes will return one JsonElement
    • same for setAttributes in GsonAttributesParser
  • consider renaming GsonAttributesParser

Don't want to loop over attrs one-by-one when setting many at once.
Want to be able to set an entire set of HashMap<String,Object> where the strings are paths.

other

  • Add removeAttribute( String attributePath)
    • removeAttribute whose path is an array index, removes that index, decreasing the array's size
  • setting attribute to null no longer removes the attribute, but actually sets to null

#89

Making a Reader on a folder

  • with no attributes.json is okay
  • whose attributes.json exists but no version okay
  • that does not exist > fails
    (Don't check that attributes.json exist because getAttribute checks already)

@bogovicj
Copy link
Contributor

bogovicj commented Feb 24, 2023

with @cmhulbert and @axtimwalde

writing dataset attributes with set attributes

Stephan leaning most permissive behavior with name forwarding in zarr (and hdf5?).

  • allowed to set dimensions and blocksize and compression attributes (or their child paths).
  • don't defend against breaking your container?
  • forward these attributes to the appropriate zarr and hdf5 attributes when applicable

listAttributes

future behavior:

  • preserve current method
  • add new method with two string args listattributes( grouppath, attrpath )
    • current impl will fwd to the new method at root attribute path
    • and if the group is empty - use root group

attribute map methods

  • make a new class responsible for json parsing with new json-specific methods
    • using JsonObjects rather than maps
  • n5reader and writer inherit both gson attr parser and new class
    • deprecate gson attr parser?
    • we will end up with redundant methods
      • but can remove in an upcoming major release

@axtimwalde
Copy link
Collaborator

Should we have deepListAttributes?

@bogovicj
Copy link
Contributor

re deepListAttributes That seems like a good idea.

Here's what I think I'd expect for its behavior. For the attributes:

{
    "a" : [ "x", "y" ],
    "b" : {
        "c" : "cow",
        "d" : "dog"
    }
}

I'd expect the output to be the list:

[
  "a",
  "a[0]",
  "a[1]",
  "b",
  "b/c",
  "b/d"
]

or similar - basically whatever @cmhulbert says is canonical. Specifically, not sure if "a[0]" , "/a[0]" , "a/[0]", "/a/[0]" should be preferred.

This happens to be what jq's paths does. It's output is:

[
  [ "a" ],
  [ "a", 0 ],
  [ "a", 1 ],
  [ "b" ],
  [ "b", "c" ],
  [ "b", "d" ]
]

@cmhulbert
Copy link
Contributor Author

cmhulbert commented Feb 27, 2023

for deepListAttributes would we want it to return the entire flattened tree for a given attributes.json? I think that would be the most equivalent to how deepList works. As for how it would return the attributePaths, I would suggest returning the keys exactly how @bogovicj listed in his output example (e.g. arrays as a[0], not a/[0]/ though they are equivalent).

Would we really want to explode all entries for an array? Or perhaps just list the index of the last element of the array? Do we want only terminal json nodes, or all intermediate paths as well? Is there a particular use-case in mind for a deepListAttributes method?

I would also suggest that similar to how deepList has a path parameter to start from we likely will want to add an attribute path parameter (in addition to a group path) to start the deepListAttributes from.

@cmhulbert
Copy link
Contributor Author

Behavior merged into #77

@cmhulbert cmhulbert closed this Mar 9, 2023
@cmhulbert cmhulbert deleted the urlAttributes branch December 4, 2024 17:28
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.

3 participants