[m-dev.] for review: Aditi updates[6]

[Fergus Henderson]

On 05-Jun-1999, Simon Taylor <stayl at cs.mu.OZ.AU> wrote:
> Index: doc/reference_manual.texi
> + at node Aditi update syntax
> + at subsubsection Aditi update syntax
> +
> +The Melbourne Mercury compiler provides special syntax to specify updates
> +of Aditi base relations.  For a predicate @samp{p/3}, the following update
> +procedures are defined. Variables named @samp{DB at var{N}} are
> + at samp{aditi__state} variables. When occuring as a pair at the end
> +of an argument list, they have mode @samp{aditi_di, aditi_uo}.

This paragraph is a long way away from the actual examples.

Also, the things that follow are builtin goals, not procedures.
(If they were procedures, then you could take their address, etc.,
but you can't.)

If this subsection is describing special syntax, then it doesn't really
belong in the "Implementation-dependent pragmas" section anymore.  So we
may need to rethink the organization of the language reference manual.

> +It is currently up to the application to ensure that any modifications
> +do not change the determinism of a base relation. Updates of relations
> +with unique B-tree indexes are checked to ensure that a key is not given
> +multiple values. The transaction will abort if this occurs.

s/indexes/indices/ ?

> +Note: The implementation of Aditi updates is not yet complete.

What part is not yet complete?

Probably they should not be documented in the reference manual unless/until
you can at least state what will work and what won't work.

> +The examples make use of the following declarations.
> + at example
> +:- pred p(aditi__state::aditi_ui, int::out, int::out) is nondet.
> +:- pragma base_relation(p/3).
> +
> +:- func q(aditi__state::aditi_ui, int::out) = (int::out) is nondet.
> +:- pragma base_relation(q/2).

I suggest naming the function `f' rather than `q'.

> + at subsubheading Deletion
...
> +When updating a predicate with type declaration
> + at w{@samp{:- pred p(aditi__state, @var{Type1}, @dots{})}},
> + at samp{@var{Closure}} must have type
> + at w{@samp{aditi_top_down pred(aditi__state, @var{Type1}, @dots{})}},
> +and inst @w{@samp{pred(unused, in, @dots{}) is semidet}}.
> +
> +When updating a function with type declaration
> + at w{@samp{:- func p(aditi__state, @var{Type1}, @dots{}) = @var{Type2}}},
> + at samp{@var{Closure}} must have type
> + at w{@samp{aditi_top_down func(aditi__state, @var{Type1}, @dots{}) = @var{Type2}}},
> +and inst @w{@samp{func(unused, in, @dots{}) = in is semidet}}.

s/When updating/When deleting from/ ?

> +The @samp{aditi_top_down} annotation on the lambda expression is needed to 
> +tell the compiler to generate code for execution by the
> +Aditi database using the normal Mercury execution algorithm.
> +
> +The @samp{aditi__state} argument of @samp{@var{Closure}} must have
> +mode @samp{unused} -- it is not possible to call an Aditi
> +relation from the deletion condition. All other arguments of
> + at samp{@var{Closure}} must have mode @samp{in}.

s/--/---/

> +If the construction of the lambda expression is in the same conjunction
> +as the @samp{aditi_delete} call, the compiler may be able to do a better
> +job of optimizing the deletion using indexes.
> +
> +Examples:
> + at example
> +        aditi_delete((p(_, X, Y) :- X + Y = 2), DB0, DB).
> +
> +        aditi_delete(q(_, 2) = Y, DB0, DB).
> +
> +        DeleteP = (aditi_top_down
> +               pred(_::unused, X::in, Y::in) is semidet :-
> +                        X = 2
> +               ),
> +        aditi_delete(pred p/3, DeleteP, DB0, DB).
> +
> +        DeleteQ = (aditi_top_down
> +               func(_::unused, X::in) = (Y::in) is semidet :-
> +                        X = 2
> +               ),
> +        aditi_delete(func q/2, DeleteQ, DB0, DB).
> + at end example
> +
> +The type of @samp{Pred} is
> + at w{@samp{aditi_top_down pred(aditi__state, int, int)}}.
> +Its inst is @w{@samp{pred(unused, in, in) is nondet}}, as for a normal
> +lambda expression.

s/Pred/DeleteP/ ?

> + at subsubheading Bulk insertion

There are sufficiently many @subsubheadings that I think there should be
separate @nodes for each @subsubheading here, with an @menu to select
between them.

> + at example
> +aditi_bulk_insert(@var{PredOrFunc} @var{Name}/@var{Arity}, @var{Closure}, @var{DB0}, @var{DB}).
> + at end example
> +
> +Insert all solutions of @samp{@var{Closure}} into the named relation.
> + at samp{@var{PredOrFunc}} must be either @samp{pred} or @samp{func}.
> +There must be a @w{@samp{:- pragma base_relation}} declaration for
> + at w{@samp{@var{Name}/@var{Arity}}}.

Can `Name' be module-qualified?
(Same question applies in a few places around here.)

> +The closure to produce the tuples to insert must have the same

I suggest s/to produce/for producing/

> +Examples:
> + at example
> +        aditi_bulk_insert(pred p/3, ancestor, DB0, DB).
> +
> +        InsertP = (aditi_bottom_up
> +                pred(DB1::aditi_ui, X::out, Y::out) is nondet :-
> +                        ancestor(DB1, X, Y)
> +                ),
> +        aditi_bulk_insert(pred p/3, InsertP, DB0, DB).
> +
> +        InsertQ = (aditi_bottom_up
> +                func(DB1::aditi_ui, X::out) = (Y::out) is nondet :-
> +                        ancestor(DB1, X, Y)
> +                ),
> +        aditi_bulk_insert(pred p/3, InsertQ, DB0, DB).

I think all the examples should be complete clauses.
So I suggest adding

	example_1(DB0, DB) :-
		...

	example_2(DB0, DB) :-
		...

to these examples.

> + at end example
> +
> +The type of @samp{Pred} is
> + at w{@samp{aditi_bottom_up pred(aditi__state, int, int)}}.
> +Its inst is @w{@samp{pred(aditi_ui, out, out) is nondet}},
> +as for a normal lambda expression.

Again, s/Pred/InsertP/ ?

> + at subsubheading Modification
> +
> + at example
> +aditi_modify(
> +        (@var{PredName}(@var{Var1}, @var{Var2}, @dots{}) = @var{Predname}(@var{Var3}, @var{Var4}, @dots{}) :-
> +                @var{Goal},
> +        ),
> +        @var{DB0}, @var{DB}).
> +
> +aditi_modify(
> +        ((@var{FuncName}(@var{Var1}, @var{Var2}, @dots{}) = @var{RetVar0}) =
> +                ((@var{Predname}(@var{Var3}, @var{Var4}, @dots{}) = @var{RetVar})) :-
> +                @var{Goal},
> +        ),
> +        @var{DB0}, @var{DB}).

As I mentioned in previous mail, I think using `=' here is not very nice
syntax.

> +aditi_modify(@var{PredOrFunc} @var{PredName}/@var{Arity}, @var{Closure}, @var{DB0}, @var{DB}).
> +
> + at end example
> +
> +Modify tuples for which @samp{@var{Goal}} or @samp{@var{Closure}}.

That sentence is incomplete.

> +The @samp{aditi__state} arguments of @samp{@var{Closure}} must have
> +mode @samp{unused} -- it is not possible to call an Aditi
> +relation from the modification goal.

s/--/---/

> Index: tests/invalid/aditi.m
> ===================================================================
> RCS file: aditi.m
> diff -N aditi.m
> --- /dev/null	Sat Jun  5 14:25:53 1999
> +++ aditi.m	Thu Jun  3 12:18:06 1999
> @@ -0,0 +1,13 @@
> +:- module aditi.
> +
> +:- interface.
> +
> +:- type aditi__state.
> +:- mode aditi_di :: in.
> +:- mode aditi_uo :: out.
> +:- mode aditi_ui :: in.
> +
> +:- implementation.
> +
> +:- type aditi__state ---> aditi__state.

This file was not mentioned in the log message.

Also it would be helpful if the file had some comments
explaining its purpose.

There was no `.err_exp' file for it, but I presume that was intentional.

> Index: tests/invalid/aditi_update_errors.err_exp
> ===================================================================
> RCS file: aditi_update_errors.err_exp
> diff -N aditi_update_errors.err_exp
> --- /dev/null	Sat Jun  5 14:25:53 1999
> +++ aditi_update_errors.err_exp	Fri Jun  4 11:54:16 1999
> @@ -0,0 +1,210 @@
> +aditi_update_errors.m:112: Error: wrong number of arguments (5; should be 3 or 4)
> +aditi_update_errors.m:112:   in call to predicate `aditi_modify'.

"predicate" is not really the right word here.

> --- ho_type_mode_bug.err_exp	1997/06/16 08:55:31	1.1
> +++ ho_type_mode_bug.err_exp	1999/05/21 09:43:11
> @@ -1,11 +1,9 @@
>  ho_type_mode_bug.m:025: In clause for `my_foldl2((pred(in, in, out) is det), in, in, out, in, out)':
> -ho_type_mode_bug.m:025:   in argument 1 of higher-order predicate call
> -ho_type_mode_bug.m:025:   (i.e. in the predicate term):
> +ho_type_mode_bug.m:025:   in argument 1 (i.e. the predicate term) of call to predicate `call/6':

Hmm, this seems like a small step backwards in terms of readability.
How hard would it be to keep the old message?

> Index: tests/valid/aditi.m
> ===================================================================
> RCS file: aditi.m
> diff -N aditi.m
> --- /dev/null	Sat Jun  5 14:25:53 1999
> +++ aditi.m	Mon May 24 10:32:57 1999
> @@ -0,0 +1,13 @@
> +:- module aditi.
> +
> +:- interface.
> +
> +:- type aditi__state.
> +:- mode aditi_di :: in.
> +:- mode aditi_uo :: out.
> +:- mode aditi_ui :: in.
> +
> +:- implementation.
> +
> +:- type aditi__state ---> aditi__state.

A brief comment explaining the purpose of this module would be helpful.

--------------------

OK, that completes this review (hurray!).  There were quite a few issues
that I mentioned in my review, but in a change this size, that is not
surprising; overall I was pretty happy with the quality of your code.

I'd like to see a relative diff for whatever changes you make to address
the points raised in my review. 

The documentation should be reviewed by at least one other person, I think,
so I suggest you choose someone and ask them to review it.
For the documentation review, it would be useful if, in addition to posting
the source diff, you could post an extract from the `.info' file for the
new stuff that you have added -- the `.info' file is much easier to read
than the TexInfo source.

Cheers,
	Fergus.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>  |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3        |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------