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

Roles should be scoped within Relations #107

Closed
flyingsilverfin opened this issue Feb 27, 2020 · 9 comments
Closed

Roles should be scoped within Relations #107

flyingsilverfin opened this issue Feb 27, 2020 · 9 comments

Comments

@flyingsilverfin
Copy link
Member

Problem to Solve

Roles are currenty globally scoped, which not only makes naming more difficult, but also increases complexity of Graql as we can share roles across unrelated parts of the type hierarchy.

Proposed Solution

We will scope roles to belong to relations. In other words, roles will be scoped to a relation and not live independently. This will making naming much simpler, as roles will be interpreted within the context of the relation name the user is providing.

@flyingsilverfin
Copy link
Member Author

Further, we want to add the ability to override a role in a sub-relation using as, which will block the sub-relation from inheriting the role form the parent. Not writing anything should simply inherit the role that is related in the parent.

@flyingsilverfin
Copy link
Member Author

Corner cases:

  • undefine - undefine a parent role will require checking any children inheriting the role are not using it
  • undefine - undefine a parent role that is override with as in children should be blocked as well

@typedb typedb deleted a comment from flyingsilverfin Mar 23, 2020
@haikalpribadi
Copy link
Member

There will also be a syntax change to Graql, @flyingsilverfin :

When you define roles in a relation, such as

marriage sub relation,
  relates husband,
  relates wife;

You will refer to the roles being played by an entity, such as:

man sub person,
  plays marriage:husband;

When a relation inherits a role from a parent relation, such as the role child below:

parenthood sub relation,
  relates parent,
  relates child;

motherhood sub parenthood,
  relates mother as parent;
  # the role 'child' is inherited

You must can only refer to the child role through the relation it was first declared:

person sub relation,
  plays parenthood:child;

@haikalpribadi
Copy link
Member

Can you aggregate all the comments (both of yours and mine) into a clear breakdown of changes included in this issue, in the issue description, @flyingsilverfin? All this should be one big change that goes in together.

@flyingsilverfin
Copy link
Member Author

@haikalpribadi sure - do you think inheriting roles is a prerequisite? the change is going to be big already, might be worth breaking it down where we can and doing first inheritance, then making the syntax changes.

@haikalpribadi
Copy link
Member

Note that some of the changes will be in Graql, and some are in Grakn Core. You can break them into the respective smaller task and put the issue in the correct repo, if that helps you. Make sure to reference them in this issue description - have a checklist of complete ones would be very useful.

@haikalpribadi haikalpribadi changed the title Scope roles to belong to relations Refactor Roles to be Scoped with Relations Mar 23, 2020
@haikalpribadi haikalpribadi changed the title Refactor Roles to be Scoped with Relations Roles should be Scoped within Relations Mar 23, 2020
@haikalpribadi haikalpribadi changed the title Roles should be Scoped within Relations Roles should be scoped within Relations Mar 23, 2020
@flyingsilverfin flyingsilverfin added this to the 1.0.8 milestone Jun 23, 2020
haikalpribadi pushed a commit that referenced this issue Jun 26, 2020
## What is the goal of this PR?

To being changes decided in #107 , we modify the grammar to have `type_scoped` as well as the default `type` that is unscoped. To make this make sense at the object level, we create a new object called `Label` that we will soon remove from `core` and `client-java` that can be scoped and unscoped types - Labels are now used throughout Graql to represent types. Where possible, we provide helper methods that consume `String`, creating the required `Label` objects internally and passing the Labels as required.

## What are the changes implemented in this PR?

* Create `Label`, a class holding the combination of scope + name. `scope` may be null
* The parser creates `Label` classes instead of strings from `label_ref`, `label_scoped`, and `label` rules
* `Compute` targets such as `of` and `in` require `Label` objects under the hood, though we provide convenience helpers that take strings and wrap them as `Label` where possible
@JRWest2000
Copy link

Typo in comment by @haikalpribadi #107 (comment)
person sub relation; should be person sub entity;
this would align it with the latter comment

You will refer to the roles being played by an entity, such as:

man sub person,

haikalpribadi pushed a commit that referenced this issue Jul 24, 2020
## What is the goal of this PR?

To being changes decided in #107 , we modify the grammar to have `type_scoped` as well as the default `type` that is unscoped. To make this make sense at the object level, we create a new object called `Label` that we will soon remove from `core` and `client-java` that can be scoped and unscoped types - Labels are now used throughout Graql to represent types. Where possible, we provide helper methods that consume `String`, creating the required `Label` objects internally and passing the Labels as required.

## What are the changes implemented in this PR?

* Create `Label`, a class holding the combination of scope + name. `scope` may be null
* The parser creates `Label` classes instead of strings from `label_ref`, `label_scoped`, and `label` rules
* `Compute` targets such as `of` and `in` require `Label` objects under the hood, though we provide convenience helpers that take strings and wrap them as `Label` where possible
@flyingsilverfin flyingsilverfin modified the milestones: 1.0.8, 1.1.0 Aug 10, 2020
@lolski lolski removed this from the 2.0.0 milestone Mar 31, 2021
@JRWest2000
Copy link

is this fully implemented, if so should this issue be closed, If not how about a description of what is and is not working and why are the changes to the language not documented in the TypeDB release notes

@flyingsilverfin
Copy link
Member Author

they were documented in the first 2.0 graql/typeql release notes, but you're right it should be closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants