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

Feature request: DEFAULT SRC/TGT atoms for RELATION statement #1189

Closed
Michiel-s opened this issue Jul 12, 2021 · 50 comments
Closed

Feature request: DEFAULT SRC/TGT atoms for RELATION statement #1189

Michiel-s opened this issue Jul 12, 2021 · 50 comments
Assignees

Comments

@Michiel-s
Copy link
Member

Michiel-s commented Jul 12, 2021

Context

Working with Ampersand prototype in practices, I recognize that often I add an exec-engine rule to fill a default tuple for a relation when a new src or tgt is instantiated. This holds in particular for reification of relations. Let me give an example:

Suppose I have a membership relation between Person and Project. Now I want to register the role and startdate for that membership. What I do is I reify the membership relation into a Membership concept like this:

CONCEPT ProjectMembership ""
RELATION person[ProjectMembership*Person] [UNI,TOT]
RELATION project[ProjectMembership*Project] [UNI,TOT]

And then I add the additional data requirements, like this:

RELATION role[ProjectMembership*Role] [UNI,TOT]
RELATION startdate[ProjectMembership*DateTime] [UNI,TOT]

Problem statement

A problem now is that I have to deal with the two new functional relationships and provide editable fields in the UI to allow for new ProjectMemberships to be created.
However, the start date can be filled automatically and the role can have a default value (e.g. 'Participant', 'Projectleader').

Then I create a new exec-engine rule to fill those in, like this:

ROLE ExecEngine MAINTAINS "Set default role for new ProjectMemberships"
RULE "Set default role for new ProjectMemberships": I[ProjectMembership] |- role;role~
VIOLATION (TXT "{EX}InsPair;role;ProjectMembership;", SRC I, TXT ";Role;Participant")

ROLE ExecEngine MAINTAINS "Set tsAdded for new ProjectMemberships"
RULE "Set tsAdded for new ProjectMemberships": I[ProjectMembership] |- tsAdded;tsAdded~
VIOLATION (TXT "{EX}InsPair;tsAdded;ProjectMembership;", SRC I, TXT ";DateTime;{php}date(DATE_ISO8601)")

This requires additional queries (=performance) to check/verify these rules and more boilerplate code in the ADL files.

Feature request

How about allowing to specify default src/tgt values at the relation definitions, e.g.

RELATION role[ProjectMembership*Role] [UNI,TOT] DEFAULT TGT "Participant"

This means that when a ProjectMembership is instantiated, by default the pair ("","Participant") is populated in the relationship.

Both default SRC and TGT can be specified, e.g.:

RELATION role[ProjectMembership*Role] [UNI,TOT] DEFAULT SRC "Test" TGT "Participant"

In this case, additionally, if a new Role is instantiated, the Test project is bound.

Allow for defaults by the exec-engine

To support the case where the current date timestamp needs to be inserted, I propose to accept also TXT statements that can be parsed by the exec-engine just like with the rule violations. Like this:

RELATION tsAdded[ProjectMembership*DateTime] DEFAULT TGT TXT "{EX}{php}date(DATE_ISO8601)"

Note the TXT in between. This is the same as we do in the VIOLATION and INTERFACES statements.
This allows for flexibility.

Considerations

  • I think it is best to NOT add the specified atoms in the default statements to the default population of a script.
@Michiel-s
Copy link
Member Author

Michiel-s commented Jul 12, 2021

@hanjoosten, what do you think? Any idea about implementation effort?

I think the output is best added to the relations.json like this:

{
        "affectedConjuncts": [],
        "inj": false,
        "mysqlTable": {
            "name": "ProjectMembership",
            "srcCol": {
                "name": "ProjectMembership",
                "null": false,
                "unique": true
            },
            "tableOf": "src",
            "tgtCol": {
                "name": "tsAdded",
                "null": true,
                "unique": false
            }
        },
        "name": "tsAdded",
        "prop": false,
        "signature": "tsAdded[ProjectMembership*DateTime]",
        "srcConceptId": "ProjectMembership",
        "sur": false,
        "tgtConceptId": "DateTime",
        "tot": true,
        "uni": true,
        "defaultSrc" : null,
        "defaultTgt" : "{EX}{php}date(DATE_ISO8601)"
    },

See fields defaultSrc and defaultTgt. No need to distinguish between Atoms or TXT statements. If it starts with {EX} then the exec-engine kicks in (just like with VIOLATION statements). Otherwise, the value is considered an Atom.

@hanjoosten
Copy link
Member

This is an interesting idea. However, since it is an addition to the syntax of Ampersand, we have to be careful. In general, syntax is added easily, but removal of it is hard.

That said, here are some initial thoughts about this proposal:

  • We need to have @stefjoosten on board.
  • I see no need to specify the concepts again in the DEFAULT part. Maybe because I am confused by your example. Did you make a typo there: ?
RELATION role[ProjectMembership*Role] [UNI,TOT] DEFAULT SRC "Test"[Project] TGT "Participant"[Role]
  • We have to take care of the typechecker too: We have to check that the TYPE of the concept and the given value match.
  • The extention to relations.json is very much the least part of the implementation.

@RieksJ
Copy link
Contributor

RieksJ commented Jul 12, 2021

While I can see that this could be useful, I'm not convinced that this is a serious problem. Of course, there is additional queries and boilerplate code here, but if that is going to be a real problem, then my guess is there are lots of other problems as well.

More to the point perhaps, it isn't clear (yet) what 'DEFAULT' means.

  • It could mean 'initial', which means that when the relation is populated, then so is its SRC or TGT value, and (if the relation isn't TOT) then depopulation wouldn't lead to a re-initialization.
  • It could also mean 'when there is no population', which obviously is the case straight after the creation of the relation, but also when the relation is completely depopulated.
  • What should happen if the value with which the SRC/TGT atom is to be filled doesn't exist, e.g. if it is an expression? Would that lead to continuous attempts to write it (consuming CPU cycles)? And what if the expression is populated? And what if the expression isn't necessarily UNI?
  • And then there is the database creation, where it matters if the DEFAULT is executed before or after installing the (default) population.

Also, default values are not always that simple. Consider the following:

CLASSIFY Person, Organization ISA Party
claimPhoneNr :: Claim * PhoneNr  -- 'Claim' is a case in which a party (person or organization) claims some damages.
claimHolder :: Claim * Party [UNI,TOT]  -- the party that has entered the claim and wants it processed.
personPhoneNr :: Person * PhoneNr -- phone number by which the Person can be reached.

ROLE ExecEngine MAINTAINS "InsPair claimPhoneNr "
RULE "InsPair claimPhoneNr ": (I-claimPhoneNr ;claimPhoneNr ~);claimHolder;personPhoneNr |- claimPhoneNr 
VIOLATION (TXT "{EX} InsPair;claimPhoneNr ;ClaimCase;", SRC I, TXT ";PhoneNr;", TGT I)

Here, initialization of claimPhoneNr is initialized conditionally, i.e. only if claimHolder;personPhoneNr exists. Do we need to instruct developers that they cannot use DEFAULT for this?

I would much rather have EQV constructs (I'm sure there has been an issue for this but I cannot find it any more), which computes the value of a relation as the contents of an expression, e.g. r EQV s;t /\ u;v.

@Michiel-s
Copy link
Member Author

@hanjoosten, you're right, I made a mistake, the line I meant is: RELATION role[ProjectMembership*Role] [UNI,TOT] DEFAULT SRC "Test"[ProjectMembership] TGT "Participant"[Role]

@Michiel-s
Copy link
Member Author

@RieksJ, I think you are missing the point here. It is not about when RELATIONs are populated, but how to populate certain relations when either the src or tgt atoms are inserted in the CONCEPT population.

I recognize that the default values are not always that simple; for situations that require a more complex expression, the regular way using the exec-engine still works.
My issue/proposal here is to add syntax for those situations where a simple default value is applicable. Like you can specify in database schema constructs.

In reaction to specific questions:

What should happen if the value with which the SRC/TGT atom is to be filled doesn't exist, e.g. if it is an expression?

I think we shouldn't allow expressions, only atom values (singletons) or TXTs that can be processed by backend (incl. exec-engine).

And then there is the database creation, where it matters if the DEFAULT is executed before or after installing the (default) population.

DEFAULTS as I propose them now are directly inserted after inserting a new atom.

@RieksJ
Copy link
Contributor

RieksJ commented Jul 13, 2021

Thanks for clarifying. I'm still not convinced though that the benefits of having it outweigh the trouble of implementing and maintaining it.

I'm also not sure I understand the actual problem that you are trying to solve:

  • In the first line of the problem statement, you say "A problem now is that I have to deal with the two new functional relationships and provide editable fields in the UI to allow for new ProjectMemberships to be created." I'm quite sure that is not the problem at hand.
  • If performance is the issue, I'd like to see data to underpin that there's really some significant improvement to be gained e.g. when initializing an application (but even then, you do this once and it's gone), or what a user would notice when he creates some atom. From what I see in logfiles, I have trouble imagining any significant improvements, but I'm open to other data.
  • If the amount of boilerplate code is the issue, I'd like to see who had actual trouble with that, or where actual trouble might arise in the (near) future. And why the suggested feature would resolve that issue better than anything else.
  • If developer effort is the issue (i.e. developers do not want to write the associated exec-engine rule), then there is significantly much more to be gained by implementing the EQV construct as I mentioned earlier.

I'm curious to find out what @stefjoosten and @hanjoosten have to say about this, as they are the ones to decide what they do (not) implement.

@hanjoosten
Copy link
Member

hanjoosten commented Jul 13, 2021

@RieksJ, I guess that the issue you couldn't find is #311. The value of that issue is higher than this issue, however, the estimated implementation effort is much higher as well.
For your understanding: The default for this issue can be thought of as similar to the default in SQL statements. However, the intention here is to implement it not at the database level, but at the frontend level. This has no effect at all for the initial population, but it affects the runtime behavior.

@hanjoosten hanjoosten self-assigned this Jul 13, 2021
@sjcjoosten
Copy link
Contributor

Having defaults at the frontent level is a great idea and that's also where they belong. I predict that we'll want different defaults for different interfaces soon, but that's all to be part of #311 and this could be a more manageable small step.

For syntax, I observe that this ties to the TOT (or SUR) rule that is implicitly created in the declaration of the relation. One could imagine having some notation to indicate which atoms to use for any existential quantifier (the implicit \exists that arise at ;), but that would be a big step instead of a small step, seeing this work first might make it easier to think of how to do that big step too. I also have no idea on how to make the connection with the TOT rule more visible in the syntax, so I'm happy to go with what is proposed.
I do suggest dropping the 'TXT' token in the syntax of DEFAULT TGT TXT "{EX}{php}date(DATE_ISO8601)". If you wish to only allow atoms, then there is no need for TXT: there are equally many occurrences of 'TXT' in the VIOLATION statement that this proposal tries to mimic.

@RieksJ
Copy link
Contributor

RieksJ commented Jul 14, 2021

Since default values are going to be simple values, we could perhaps go for syntax such as [UNI, TOT, SRC="somevalue"] which would suggest there's a multiplicity relation implied.

Syntax such as [UNI, TGT=EVAL(date(DATE_ISO8601)] might be used to specify the result of a function as a default value (which I think is appropriate).

@hanjoosten
Copy link
Member

Glad you give the syntax some thought too. However, the place you suggest for the default values is a list of properties, which in fact are restrictions on the population of the relation. The default values for atoms are something else. So we start with the keyword "DEFAULT". this will be followed by an optional SRC part, then an optional TGT part.
The SRC and TGT parts are identical, exept for their keywords.

After the keyword we will expect either:

  • a value of an atom, which will be typechecked to match the TYPE of that relation.
  • the keyword EVAL, followed by a double-quoted string that could be processed by the exec-engine

Some examples:

RELATION role[ProjectMembership*Role] [UNI,TOT] DEFAULT TGT "Participant"

RELATION membership[Project*ProjectMembership] [INJ,SUR] DEFAULT SRC "Test"

RELATION startdate[ProjectMembership*DateTime] [UNI,TOT] DEFAULT TGT EVAL "{php}date(DATE_ISO8601)"

notes:

  1. The optional DEFAULT part will be after the optional property list, and before the optional PRAGMA part.
  2. If the relation is declared multiple times (for example in different files) an error will be raised if the defaults in those statements conflict with each other.
  3. The prefix {EX} is not part any more of this syntax. If used, the exec-engine is likely to choke on it.

@RieksJ
Copy link
Contributor

RieksJ commented Jul 14, 2021

I know that the place I suggest is in the existing list of properties, thereby following up on the remark by @sjcjoosten who stated that having a default value implies such properties. If (not saying: 'when') we follow up on that remark, would it make more sense to consider syntax such as [INJ, SUR="src-value", TOT="tgt-value"]?

I appreciate that having separate syntax as you suggest is easier to implement, and that could be a valid consideration too.

@hanjoosten
Copy link
Member

hanjoosten commented Jul 14, 2021

Ah, good point. I didn't get that before. If we go this way, it seems to me we could also skip the keywords DEFAULT, SRC, and TGT. The properties TOT and SUR will then get an optional part, being either the value of an atom or the keyword EVAL followed by a string. My earlier examples would then be denoted as:

RELATION role[ProjectMembership*Role] [UNI,TOT "Participant" ] 

RELATION membership[Project*ProjectMembership] [INJ,SUR "Test"]

RELATION startdate[ProjectMembership*DateTime] [UNI,TOT EVAL "{php}date(DATE_ISO8601)"]

Notes: The value given with TOT is by definition meant to populate the target concept, and the value given with SUR is by definition meant for the source concept. Hence, there is no need for a specific keyword.

I like this idea. What do @Michiel-s, @sjcjoosten, and @stefjoosten think?

@hanjoosten
Copy link
Member

I am not sure about the amount of work of implementing Rieks's idea vs my initial idea. I will give that a thought, but beside of that, I would like to know what the others like most.

@sjcjoosten
Copy link
Contributor

What I like is that this ties in very well with the UNI/TOT rules. I also very much like changing the prefix "{EX}" within the string to a keyword EXEC. What I dislike is that it encourages defaults to be specified at the place in the script where RELATIONs are declared: the conceptual model is conceptually far away from tweaking interface behavior so it would be nice if the syntax would naturally separate the two as well. If I as a modeler decide to change a relation to no longer have TOT, I'd like to see a warning/error about the DEFAULT rule I wrote in another file (and not have a DEFAULT rule add the TOT property back because of syntax).

Given these likes and dislikes, I'd say let's go with the EXEC idea regardless. Apart from that, the two are about equal to me.

@hanjoosten
Copy link
Member

This issue very much relates with the implementation of the ENFORCE statement (Issue #1204). This is currently in the review phase, and I expect it to be into development shortly.
I expect that the new implementation of the ENFORCE statement has made it quite easy to implement the ideas I wrote down in in this comment earlier.

@Michiel-s , Do you think such syntax will solve your original problem?
@stefjoosten , can you bless that syntax?

@hanjoosten
Copy link
Member

hanjoosten commented Sep 6, 2021

Just for a reference, I now think the syntax boils down to:

Property
    ::= ("SUR" | "TOT") ( '<AtomValue>' | "EVALPHP" '<DoubleQuotedString>')?
      | ("ASY" | "INJ" | "IRF" | "PROP" | "RFX" | "SYM" | "TRN" | "UNI") 

Which can be visualized as
image

Some notes:

  1. I'd rather have a keyword EVALPHP, not EVAL. Then the modeller is not required to prefix the DoubleQuotedString with {php}. If required by the backend, the generator will take care of that. This leaves room for other keywords, depending on the language supported by the backend.
  2. The provided value for AtomValue will be typechecked against the TTYPE of it's Concept.
  3. The typechecker will check that no different default values are given, even when the RELATION is declared multiple times.
  4. When using the "EVALPHP" keyword, the user is on it's own to make sure that the type is correct.

@hanjoosten
Copy link
Member

I just had a chat with @stefjoosten . He suggested to enable the use of an expression, at the place where the atomvalue can be given. So we still need some more thinking of the syntax.

@sjcjoosten
Copy link
Contributor

sjcjoosten commented Sep 6, 2021

I like the idea of allowing an expression in the place of '<AtomValue>'.
I believe that the expression corresponding to '<AtomValue>' is also '<AtomValue>'.
In other words, if we compare these two syntaxes:

Property
    ::= ("SUR" | "TOT") ( '<AtomValue>' | "EVALPHP" '<DoubleQuotedString>')?
      | ("ASY" | "INJ" | "IRF" | "PROP" | "RFX" | "SYM" | "TRN" | "UNI") 

(Han's earlier proposal)

and:

Property
    ::= ("SUR" | "TOT") (Expression | "EVALPHP" '<DoubleQuotedString>')?
      | ("ASY" | "INJ" | "IRF" | "PROP" | "RFX" | "SYM" | "TRN" | "UNI") 

then the second would subsume the first. We might be inclined to think that building the first of these two will not hinder us from ever building the second of these two. That suggests, however, that the type of Expression and the type of '<AtomValue>' will match, and I think they might not.

For r :: A * B [TOT], I believe the type of Expression should be A * B', where B' denotes that B may be generalized to allow for new concepts to be inserted. The A is there so that all violations of the TOT rule (these are of type A*A) can each get their own atom from B': without A we cannot make such distinctions. Here's an example use (using expression syntax as above):

company :: Account * Company [UNI, TOT EVALPHP '"getSessionCompany()"'] -- every account belongs to a company, the getSessionCompany() is custom-built for this application
defaultAccount :: Company * AccountType -- allow companies to set a default account type
accountType :: Account * AccountType [TOT company;defaultAccount]

The generalization of B is tricky: we might want things to be inserted that don't belong to any concept yet, i.e. that aren't properly typed Atoms. For that reason, I'd suggest to implement this in steps, i.e. deal with expressions (and possibly EVALPHP) later. Regardless of when we implement what, what would the equivalent to '<AtomValue>' look like as an expression? It would be V[A*B'];'<AtomValue>' (user would need to write V;'<AtomValue>' because B' might not be expressible). That's already different from a plain '<AtomValue>'. For this reason, it could be a good idea to give expressions and atom-values a keyword, just like EVALPHP, so we can separate them.

@RieksJ
Copy link
Contributor

RieksJ commented Sep 6, 2021

Cool! But what happens whendefaultAccount has 2 tgt atoms for a single sec atom?

@sjcjoosten
Copy link
Contributor

sjcjoosten commented Sep 6, 2021

Cool! But what happens whendefaultAccount has 2 tgt atoms for a single sec atom?

Good question! I can think of only one kind of behavior, which is consistent with what the exec-engine would do: both defaults would get inserted. If accountType is additionally UNI (while defaultAccount is not) then this could result in a bug that is horribly difficult to debug, claiming a violation of an UNI rule while the user did not insert anything, but perhaps we'll see some decent feedback as these defaults float to the interface.

@RieksJ
Copy link
Contributor

RieksJ commented Sep 7, 2021

I agree that this should be the behaviour.

Regarding error messages, I can see that in an implementation, such exceptions could somehow be caught and the necessary guidance for helping out the developer out could be added. One way would be to handle ENFORCE statements (and thereafter any associated UNI/INJ checks - but also any SUR/TOT checks, because there is also no guarantee that there will be a TGT atom - in a designated context (we used to call a SERVICE), which has its own execengine run and error message generation.

@hanjoosten
Copy link
Member

hanjoosten commented Sep 7, 2021

@sjcjoosten, good point, allowing an <Expression> too does require a keyword to separate the <Expression> from the <AtomValue>. Indeed, it makes sense that when an <Expression> is used, the source of that expression should match the atom that causes the violation. So we need a keyword in front of the <AtomValue>.

Having said that, there is more to think about w.r.t. the <Expression>. Your example of V;<AtomValue> will not work as one might expect, unless <AtomValue> is already an element of the population of the target concept (See issue #166). I think that eventually, we might agree on some type of behaviour for the <Expression> part, but that needs more thinking. (And we currently have an alternative for the expression). I propose not to implement the <Expression> part right now, leave that for sometime later. Only ensure we will then be backward compatible by introducing some keyword in front of the <AtomValue> part:

Property
    ::= ("SUR" | "TOT") ( "VALUE" '<AtomValue>' | "EVALPHP" '<DoubleQuotedString>')?
      | ("ASY" | "INJ" | "IRF" | "PROP" | "RFX" | "SYM" | "TRN" | "UNI") 

which can be visualized as
image

@RieksJ
Copy link
Contributor

RieksJ commented Sep 7, 2021

I suggest

Property
    ::= ("SUR" | "TOT") ( "VALUE" '<AtomValue>' | "EVALPHP" '<DoubleQuotedString>' | "EXPR" '<expression>' )?
      | ("ASY" | "INJ" | "IRF" | "PROP" | "RFX" | "SYM" | "TRN" | "UNI") 

@sjcjoosten
Copy link
Contributor

Having said that, there is more to think about w.r.t. the <Expression>. Your example of V;<AtomValue> will not work as one might expect, unless <AtomValue> is already an element of the population of the target concept

I was thinking of this when I wrote that the generalization of B is tricky, but we can generalize it all the way to its Ttype. I also like the VALUE keyword.

@hanjoosten
Copy link
Member

@Michiel-s You have been quiet for a long time. Do you think this is going the right way?

@hanjoosten
Copy link
Member

@RieksJ wrote earlier:

I would much rather have EQV constructs (I'm sure there has been an issue for this but I cannot find it any more), which computes the value of a relation as the contents of an expression, e.g. r EQV s;t /\ u;v.

This is done! See issue #1204. It is in development, will be released next friday.

@hanjoosten
Copy link
Member

hanjoosten commented Sep 10, 2021

  • Add this feature in the Ampersand repository
  • Add this feature in the Prototype repository

@hanjoosten
Copy link
Member

@Michiel-s , I realized that the violation itself will probably be required, as well as the fact that in these cases there must be a Role. I added ExecEngine as Role. See the branch called feature/issue1189.

@Michiel-s
Copy link
Member Author

@Michiel-s , I realized that the violation itself will probably be required, as well as the fact that in these cases there must be a Role. I added ExecEngine as Role. See the branch called feature/issue1189.

This is a comment regarding #1204 I think. Not for here

@Michiel-s
Copy link
Member Author

@Michiel-s You have been quiet for a long time. Do you think this is going the right way?

I really like this. I still have to take a look at the generated output in json files and implement the prototype framework part.

@Michiel-s
Copy link
Member Author

Ok, I just had time to start implementing this feature in the prototype framework, but I conclude that we are not there yet for the compiler part. Some rework is needed:

  1. Somehow in our conversation above the link is make between TOT/SUR and a default value. My original request was to be able to specify a default value independently from TOT/SUR constraints. Why not have a default for Non-TOT relations?
  2. I also noted that the TOT/SUR flags are broken now in the relations.json output for those relations that have a default defined
  3. The output of a default VALUE specification in the relations.json file looks like "defaultSrc": "AAVString {aavhash = 723817982351009968, aavtyp = OBJECT, aavtxt = \"test2\"}", This is not what is needed. I would like to see only the atom value "test2" here.

@hanjoosten, regarding 1, I took a look at the code changes in #1210 and noticed that you indeed coupled the default value to the property definition, that's why there are also so many code changes:

type AProps = Set.Set AProp
data AProp
  = -- | univalent
    Uni
  | -- | injective
    Inj
  | -- | surjective
    Sur (Maybe APropDefault)
  | -- | total
    Tot (Maybe APropDefault)
  | -- | symmetric
    Sym
  | -- | antisymmetric
    Asy
  | -- | transitive
    Trn
  | -- | reflexive
    Rfx
  | -- | irreflexive
    Irf
  deriving (Eq, Ord, Data, Typeable)

I think this is not a good design decision and propose to undo this. I would say to couple the default src or tgt value to the RELATION definition instead.

@hanjoosten
Copy link
Member

hanjoosten commented Oct 20, 2021

I think we all didn't understand your feature request. In the middle of the discussion you probably missed out. So let's see what can/should be done now

  1. These fixes are already merged into main. Removing it is possible, but maybe we should discuss this first. I still think that the current implementation serves a purpose. But the resolving of the default should be handled by using the execengine instead.
  2. I assume now, in retrospect, that your intention was to have a feature to be able to inform the frontend (not the execengine) that a default value should be created every time an empty field would be painted on the screen. If that is indeed what you intend, that could be implemented as well. Now by extending the RELATION statement. This could be done as well. If this is what you intend, then we could discuss that. Maybe in a separate issue.
  3. Your remark about how an atomvalue is shown might not be accurate. The atomvalue could be an ALPHANUMERIC , but also something else. In .json everything has to be cast to a String, So only if it is already a string, you don't want to show it? Then what happens if the string is something like This string contains a nasty " double-quote character.

@Michiel-s
Copy link
Member Author

In the middle of the discussion you probably missed out. So let's see what can/should be done now
That's true, I didn't contribute to large part of the discussion, that's my bad.

  1. Ok, let's discuss indeed. Maybe first by phone. We're almost working towards a 5.0 version of the compiler, so I would say this is the right moment. Btw, I don't think anybody has used this feature, as it doesn't have any affect in the prototypes yet. I agree that there is a purpose to have the default tight to the TOT/SUR rules, but that is a subset of the feature I proposed. My proposal is to keep the feature functionally, but change the implementation to have it also work for non-TOT and non-SUR relations.

  2. Frontend (=UI), backend (= API + backend + database), exec-engine (= part of the backend). My intention is NOT about the frontend as you described, also NOT syntactical sugar for the exec-engine, but a functionality that the backend implementation can take care of.

  3. I understand that there could be different TYPES that all need to be casted to a string. That's no problem. Escaping double quotes is needed (and also part of the json syntax spec) so after decoding the json file those are removed anyway. My issue here is that the value should only be the value. See example output below, the defaultSrc field contains: AAVString {aavhash = 723817982351009968, aavtyp = OBJECT, aavtxt = \"test2\"}.

{
        "affectedConjuncts": [
            "conj_54",
            "conj_56"
        ],
        "defaultSrc": "AAVString {aavhash = 723817982351009968, aavtyp = OBJECT, aavtxt = \"test2\"}",
        "defaultTgt": "AAVString {aavhash = -4513705105538940906, aavtyp = OBJECT, aavtxt = \"test\"}",
        "inj": false,
        "mysqlTable": {
            "name": "r",
            "srcCol": {
                "name": "A",
                "null": false,
                "unique": false
            },
            "tableOf": null,
            "tgtCol": {
                "name": "B",
                "null": false,
                "unique": false
            }
        },
        "name": "r",
        "prop": false,
        "signature": "r[A*B]",
        "srcConceptId": "A",
        "sur": false,
        "tgtConceptId": "B",
        "tot": false,
        "uni": false
    },

@Michiel-s
Copy link
Member Author

Michiel-s commented Oct 21, 2021

Proposed syntax: RELATION r[A*B] DEFAULT SRC VALUE "a1" TGT EVALPHP "func(xyz)"

@Michiel-s
Copy link
Member Author

Michiel-s commented Oct 21, 2021

Not to forget the minor bugs:

  • I noted that the TOT/SUR flags are broken now in the relations.json output for those relations that have a default defined
  • Fix show of defaultSrc and defaultTgt in json output

@hanjoosten
Copy link
Member

Recap: I had a call with @Michiel-s today. Now I understand that this issue is NOT about automatically fixing violations. That would limit the scope of the feature to TOT and SUR relations. As he explained in his comments from yesterday and today (see above) the idea is to have a default value filled in in the backend, outside of the execengine. That's all.

To do this, the following has to be done:

  1. Remove the above proposed extended syntax of the TOT and SUR properties.
  2. Extend the syntax of the RELATION statement:

It currently is:

RelationDef ::= (RelationNew | RelationOld) Props? ('PRAGMA' Text+)? Meaning* ('=' Content)? '.'?

which can be visualized as
image

The proposal is to add an optional default after the optional Props and before the optinal Pragma:

RelationDef ::= (RelationNew | RelationOld) Props? Defaults? ('PRAGMA' Text+)? Meaning* ('=' Content)? '.'?

which can be visualized as
image

The Defaults part will be a like this:

Defaults ::= ( ( 'SRC' | 'TGT' ) ( 'VALUE' '<AtomValue>' | 'EVALPHP' '<DoubleQuotedString>' ) )+

which can be visualized as
image

@Michiel-s
Copy link
Member Author

Michiel-s commented Nov 3, 2021

Hi @hanjoosten, I'm currently testing feature branch feature/RelationDefaultValues_issue1189.

Feedback

  1. Output in generated relation.json is good! I can work with that in the prototype framework.

  2. Testing out the syntax in ADL doesn't look intuitive if you would ask me. What do you think? See below

Line of reasoning for syntax

E.g. this script is how it now looks like:

RELATION projectStatus[Project*Status] [UNI] TGT VALUE "In preparation" MEANING "The current status of a project according to the 4 stages described in our project management handbook"

Somehow to me the part TGT VALUE "In preparation" dangles. The props are nicely between brackets, the relation signature is the identifier paired with the keyword RELATION and the meaning is after the MEANING keyword.

My suggestion is to add the keyword DEFAULT:

RELATION projectStatus[Project*Status] [UNI] DEFAULT TGT VALUE "In preparation" MEANING "The current status of a project according to the 4 stages described in our project management handbook"

Using newlines this can look like:

RELATION projectStatus[Project*Status] [UNI] 
    DEFAULT TGT VALUE "In preparation"
    MEANING "The current status of a project according to the 4 stages described in our project management handbook"

This reads like "the default target value is ..." or if multiple, "the default target values are ..."

Furthermore I propose to add a comma , between the different default values. This is more in line with the syntax we have in VIOLATIONS, VIEWS, INTERFACE and POPULATIONS.

RELATION projectStatus[Project*Status]
    DEFAULT TGT VALUE "In preparation", TGT VALUE "New"
    MEANING "The current status of a project according to the 4 stages described in our project management handbook"

Proposal

This results in:
RelationDef ::= (RelationNew | RelationOld) Props? ('DEFAULT' Defaults)? ('PRAGMA' Text+)? Meaning* ('=' Content)? '.'?
image

and
Defaults ::= ( 'SRC' | 'TGT' ) ( 'VALUE' '<AtomValue>' | 'EVALPHP' '<DoubleQuotedString>' ) ( ',' ( 'SRC' | 'TGT' ) ( 'VALUE' '<AtomValue>' | 'EVALPHP' '<DoubleQuotedString>' ) )*
image

Help

@hanjoosten, I tried to edit the parser myself to demonstrate what I propose, but I got stuck hopelessly. Really need your help, but I do want to learn.

@hanjoosten
Copy link
Member

@Michiel-s , No sweat. This will be a relative easy change. It only affects the parser and prettyprinter. I would gladly help you do this yourself, during a pair programming session.

@sjcjoosten
Copy link
Contributor

RELATION projectStatus[Project*Status]
    DEFAULT TGT VALUE "In preparation", TGT VALUE "New"
    MEANING "The current status of a project according to the 4 stages described in our project management handbook"

Not sure what the meaning is of two defaults? I get how a project can get two statuses, so the example clarifies a lot, but my understanding is that EVALPHP will return a function that yields a list of values (for otherwise it won't be possible to have a default with an arbitrary number of elements, as one would for the RELATION position[CharAtPos*String] MEANING "The string at which a character occurs in a certain position. For example the String 'abc' will have character-positions (a,0), (b,1) and (c,2)."). So what would be the meaning of DEFAULT TGT EVALPHP "f1", TGT EVALPHP "f2"? Is there a compelling reason to take the union (or any other operation) here rather than use DEFAULT TGT EVALPHP "phpUnion(f1,f2)"?

If not, perhaps the syntax could read DEFAULT TGT VALUE "In preparation", "New", to communicate more clearly that one has a set of values as a default (rather than that the computer will make some choice between different values):

Defaults ::= ( 'SRC' | 'TGT' ) ( 'VALUE' <AtomValue> | 'EVALPHP' <DoubleQuotedString> ) ( ',' ( 'SRC' | 'TGT' ) ( 'VALUE' <AtomValue> (',' 'VALUE' <AtomValue>)* | 'EVALPHP' <DoubleQuotedString> ) )*

@hanjoosten
Copy link
Member

The idea here is quite simple: Since the relation is not restricted to be UNI, why then restrict the amount of defaults to one?

@sjcjoosten
Copy link
Contributor

The idea here is quite simple: Since the relation is not restricted to be UNI, why then restrict the amount of defaults to one?

I agree with this point (I should argue that we might even want to allow the default to be defined as zero). I was merely proposing to use the syntax DEFAULT TGT VALUE "In preparation", "New" to express this more clearly.

@hanjoosten
Copy link
Member

hanjoosten commented Nov 4, 2021

Ah, I see. That leads us to something like

Defaults ::= ( 'DEFAULT' 
             ( 'SRC' | 'TGT' ) ( 'VALUE' '<AtomValue>' ( ',' 'VALUE' '<AtomValue>' )* | 'EVALPHP' '<DoubleQuotedString>' )
       ( ',' ( 'SRC' | 'TGT' ) ( 'VALUE' '<AtomValue>' ( ',' 'VALUE' '<AtomValue>' )* | 'EVALPHP' '<DoubleQuotedString>' ) )* )?

image

@hanjoosten
Copy link
Member

I see what you mean. I edited above syntax. This leaves us with the name of the keyword DEFAULT or DEFAULTS Maybe we should accept both.

There is no way that Ampersand itself knows what the result will be of the PHP expression. It could be a single atom, but it could be a set as well. @Michiel-s , can that become a problem? In any case, the modeler should know what she is doing.

@Michiel-s
Copy link
Member Author

Yes no worries, if a eval returns a list, we have multiple defaults.

I like the simpler syntax proposal by @sjcjoosten to specify multiple values. Great!

@Michiel-s
Copy link
Member Author

This leaves us with the name of the keyword DEFAULT or DEFAULTS Maybe we should accept both.

My personal preference goes to DEFAULT (single). Because you default to a single or you default to a list of values. No need to specify plural option, which is not always, and most often not the case.

Other argumentation is possible of course. I wouldn't allow both, that's only for those who can't come to an understanding and I think we can😁

@stefjoosten
Copy link
Contributor

Michiel said: "I think it is best to NOT add the specified atoms in the default statements to the default population of a script."
I agree. See issue #166

@sjcjoosten
Copy link
Contributor

sjcjoosten commented Nov 4, 2021

This leaves us with the name of the keyword DEFAULT or DEFAULTS Maybe we should accept both.

My personal preference goes to DEFAULT (singular). Because you default to a single or you default to a set of values.

I completely agree. I believe we may want to (at some point in the distant future) talk about having a certain default for one interface (which may be a set) and use a different default in a different interface. I'd want to use the word 'DEFAULTs' to refer to those cases (not as part of Ampersand syntax, but in the same informal way we can talk about 'POPULATIONs' when referring to all the different sources from which we get the initial population; Note that the POPULATION keyword is also singular)

@hanjoosten
Copy link
Member

Thanks guys for this discussion. Now @Michiel-s can fix this ;)

@Michiel-s
Copy link
Member Author

Michiel-s commented Nov 7, 2021

Minor change in syntax compaired to @hanjoosten's latest proposal, after discussion with @sjcjoosten because of ambiguously in look ahead after comma..

image

Defaults ::= ( 'DEFAULT' ( ( 'SRC' | 'TGT' ) ( 'VALUE' '<AtomValue>' ( ',' '<AtomValue>' )* | 'EVALPHP' '<DoubleQuotedString>' ) )+ )?

@Michiel-s
Copy link
Member Author

Fixed

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

No branches or pull requests

5 participants