feat(model) Add ContainerPath aspect model #7774
Conversation
chriscollins3456
requested review from
asikowitz,
hsheth2,
jjoyce0510 and
aditya-radhakrishnan
| "delimiter": "/" | ||
| } | ||
| } | ||
| path: array[ContainerPathEntry] |
There was a problem hiding this comment.
I know we discussed this previously, but are we SURE that this modeling restriction won't get us into trouble?
There was a problem hiding this comment.
Is there any world in which it might?
There was a problem hiding this comment.
hmm @jjoyce0510 do you have anything in mind? i'm definitely going to keep thinking about it before merging... and get approvals from at least one of the ingestion folks
|
Is the eventual plan to show containers and datasets together in browse? |
metadata-models/src/main/pegasus/com/linkedin/common/ContainerPathEntry.pdl
Outdated
Show resolved
Hide resolved
| * The ID of the container path entry - what gets stored in the index, URL encoded | ||
| * If there's an urn associated with this entry, id and urn will be the same | ||
| */ | ||
| id: string |
There was a problem hiding this comment.
Could we ever want to use container paths to support a more custom "folder" type experience, where users can choose what path they want entities in, especially those that don't fit in containers? I think the lines are a little blurred because ContainerPaths aren't just for storing the path of containers for an entity, it's the path of containers for an entity + another path for entities that aren't stored in containers.
I know this is a significant change, but what do you think about making container paths be composed of only containers (thus can be a list of urns), then having a second aspect like CustomBrowsePath that we use for the non-container paths?
There was a problem hiding this comment.
hmm yeah i definitely get what you're saying. Thinking about implementation, I personally would rather not have to know whether a given entity has containerPaths or some other aspect in order for us to operate browse.
The other thing is that we're going to need to filter on this aspect across all entity types (when they click a group on the sidebar of browse) so having that custom logic to filter for one field on elastic or another may be frustrating.
Maybe we can come up with a better name other than containerPaths? however the word "container" is already generic enough - it's just a little overloaded since we have actual "container" entities.
what do you think?
There was a problem hiding this comment.
I think the name is ok, maybe there's a better one but I don't feel too strongly about it. Thinking over it more, this seems reasonable enough to me!
Co-authored-by: Harshal Sheth <hsheth2@gmail.com>
@xiphl yes! we're working towards a new browse experience that's based on containers for datasets |
This adds the new
containerPathaspect model. This will be used for our updated browse experience to show a different type of path.Feel free to think more about the model and how it will be used and have a discussion here on this PR!
Checklist