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

Issue with 'errata-query-11' (no need to change 'Evaluation of Aggregation') #94

Closed
hartig opened this issue Jun 9, 2023 · 1 comment · Fixed by #109
Closed

Issue with 'errata-query-11' (no need to change 'Evaluation of Aggregation') #94

hartig opened this issue Jun 9, 2023 · 1 comment · Fixed by #109

Comments

@hartig
Copy link
Contributor

hartig commented Jun 9, 2023

I am looking at errata-query-11. The suggested fix is that ...

The definition of Aggregation() should replace P by Group(exprlist, P) giving:

eval(D(G), Aggregation(exprlist, func, scalarvals, Group(exprlist, P))) =
       Aggregation(exprlist, func, scalarvals, eval(D(G), Group(exprlist, P)))

The current version of the definition looks as follows.

eval(D(G), Aggregation(exprlist, func, scalarvals, P)) =
       Aggregation(exprlist, func, scalarvals, eval(D(G), P))

While I can see that using the symbol P in this definition can cause confusion, I don't think there is anything wrong with the current definition per se. Here is why:

Given how any expression of the form Aggregation(exprlist, func, scalarvals, P) is produced by the translation algorithm in Section 18.2.4.1 Grouping and Aggregation, it is guaranteed that P is an expression of the form Group(exprlist, P') where P' is "the algebra translation of the GroupGraphPattern of the query level" (i.e., also an expression). The part that may cause the confusion is that the symbol P here is named G in the translation algorithm in Section 18.2.4.1 Grouping and Aggregation (and our P' is named P in the algorithm).

Then, given such an expression P of the form Group(exprlist, P'), the aforementioned current version of the definition applies the eval function to it, eval(D(G), P). Now, since P is of the form Group(exprlist, P'), the relevant part of the definition of the eval function is given by the Evaluation of Group for which the corresponding Group operator "produc[es] a set of partial functions from keys to solution sequences." Hence, such a set is the result of calling eval(D(G), P) in the aforementioned current version of the definition, and that's exactly what the Aggregation operator expects as its fourth argument.

So, I don't see a need to apply the aforementioned suggested fix.*
Perhaps what should be done instead is to rename the symbol P in the definition, ideally using the same symbol as used by the aforementioned translation algorithm in Section 18.2.4.1 Grouping and Aggregation. What makes this renaming tricky is that, as mentioned above, the translation algorithm uses the symbol G but that is already used for something else here (namely, for the active graph G of the dataset D). Therefore, my proposal is to use the new symbol Grp in both places. That is, in the translation algorithm in Section 18.2.4.1 Grouping and Aggregation, rename G to Grp, and in the current version of the definition of Evaluation of Aggregation, rename P to Grp.

*footnote: In fact, applying the aforementioned suggested fix may be problematic later if we want to fix errata-query-21 in the way as proposed by Andy (which I agree with). That proposal introduces a "dummy key special marker 'group-all'" that "isn’t an exprlist", in which case we cannot always assume P/Grp an expression of the form Group(exprlist, P'); it may then also be of the form Group('group-all', P').

@hartig
Copy link
Contributor Author

hartig commented Jun 29, 2023

Sidenote: In addition to the main change suggested addressing for errata-query-11---as discussed in this issue as unnecessary or even problematic---the errata also includes a smaller change suggestion:

In Definition: Aggregation replace "multiset of partial functions" by "set of partial functions".

This change has been implemented in PR #98.

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 a pull request may close this issue.

1 participant