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

Fix GROUP BY definition when no GROUP BY clause is present #105

Open
Tpt opened this issue Jun 23, 2023 · 4 comments
Open

Fix GROUP BY definition when no GROUP BY clause is present #105

Tpt opened this issue Jun 23, 2023 · 4 comments
Labels
spec:editorial Minor issue or proposed change in the specification (markup, typo, informative text)

Comments

@Tpt
Copy link
Contributor

Tpt commented Jun 23, 2023

Currently the "group step" is defined as Let G := Group((1), ToList(P)). This is wrong because the specification states that "in case of implicit grouping the single group must exist and might be empty". Indeed, Group does not associate keys with empty groups leading to no group existing when P is empty.

I believe it might be rewritten as Let G := { (1) -> ToList(P) }, skipping the Group call entirely and always ensuring the existence of a single group.

@Tpt Tpt added the spec:editorial Minor issue or proposed change in the specification (markup, typo, informative text) label Jun 23, 2023
@afs
Copy link
Contributor

afs commented Jun 26, 2023

See also

@hartig
Copy link
Contributor

hartig commented Jun 28, 2023

I agree that this is an issue that needs to be fixed, but I don't think the proposed solution ("skipping the Group call entirely" by rewriting Let G := Group((1), ToList(P)) to Let G := { (1) -> ToList(P) } ) is going to work.

The issue with this proposed solution is that G needs to be an expression that can be evaluated (i.e., the eval function can be applied to it), which is not the case for G := { (1) -> ToList(P) }.

In contrast to this proposed solution, I think the correct way to fix the issue is to i) keep the translation algorithm as is (i.e., in particular, keep Let G := Group((1), ToList(P))) and, then, ii) extend the definition of the Group operator such that it handles this edge case differently.

@Tpt
Copy link
Contributor Author

Tpt commented Jun 29, 2023

The issue with this proposed solution is that G needs to be an expression that can be evaluated (i.e., the eval function can be applied to it), which is not the case for G := { (1) -> ToList(P) }.

That's a good point. My bad. Thank you!

In contrast to this proposed solution, I think the correct way to fix the issue is to i) keep the translation algorithm as is (i.e., in particular, keep Let G := Group((1), ToList(P))) and, then, ii) extend the definition of the Group operator such that it handles this edge case differently.

I am not sure it's a great idea because I believe SELECT (COUNT(*) AS ?c) WHERE { FILTER(false) } and SELECT (COUNT(*) AS ?c) WHERE { FILTER(false) } GROUP BY (1) translation will both contain Group((1), ToList(P)) but must exhibit different behaviors (the first should return 0 and the second nothing).

What about introducing a new algebra operator instead?

@hartig
Copy link
Contributor

hartig commented Jun 29, 2023

In contrast to this proposed solution, I think the correct way to fix the issue is to i) keep the translation algorithm as is (i.e., in particular, keep Let G := Group((1), ToList(P))) and, then, ii) extend the definition of the Group operator such that it handles this edge case differently.

I am not sure it's a great idea because I believe SELECT (COUNT(*) AS ?c) WHERE { FILTER(false) } and SELECT (COUNT(*) AS ?c) WHERE { FILTER(false) } GROUP BY (1) translation will both contain Group((1), ToList(P)) but must exhibit different behaviors (the first should return 0 and the second nothing).

What about introducing a new algebra operator instead?

I don't think that's necessary.

Note that @afs has proposed a fix for errata-query-21 which includes introducing a "dummy key special marker 'group-all'". Once this is implemented, we will have

  • Group('group-all', ToList(P)) in the case of SELECT (COUNT(*) AS ?c) WHERE { FILTER(false) } and
  • Group((1), ToList(P)) in the case of SELECT (COUNT(*) AS ?c) WHERE { FILTER(false) } GROUP BY (1).

Hence, there will be a clear way to distinguish the special case from the regular cases: all that needs to be done in the definition is to check whether the first argument of the Group operator is the dummy key.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:editorial Minor issue or proposed change in the specification (markup, typo, informative text)
Projects
None yet
Development

No branches or pull requests

3 participants