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

Turn the input to Group(..) and to Aggregation(..) into solution sequences #98

Merged
merged 7 commits into from
Jun 29, 2023

Conversation

hartig
Copy link
Contributor

@hartig hartig commented Jun 14, 2023

... as suggested by @kasei in #95 (comment)


Preview | Diff

… into solution sequences rather then sets/multisets of solutions; see #95 (comment)
@hartig
Copy link
Contributor Author

hartig commented Jun 14, 2023

Currently, this change uses Option 1 of #95 (comment) to convert the input to the Group operator from a multiset of solution mappings into a sequence of solution mappings.

Following this change, this PR changes the definitions of both the Group operator and the Aggregation operator.

In particular, the definition of the Group operator now captures explicitly that the operator produces partial functions from keys to solution sequences.

Similarly, the definition of the Aggregation operator now captures that:

  1. such partial functions from keys to solution sequences are the input to this operator,
  2. the operator internally produces lists of tuples rather than sets of tuples (see, in particular, the change to the function M),
  3. the given "set function" is invoked with such a list of tuples rather than with a set of tuples.

Additionally, I have adapted the example below the definition of the Aggregation operator according to these changes.

However, that's only a part of the changes that are needed to implement @kasei 's proposal. The next step is to adapt the definitions of the set functions. For instance, in Section 18.5.1.1 Set Functions, the symbol M must now denote a lists of lists rather than a multiset of lists, and the same holds for M in Section 18.5.1.2 Count and also in the next sections. As another consequence, the definition of the Flatten function (as used in Section 18.5.1.2 Count) needs to be changed as well.

@kasei any comments so far?

spec/index.html Outdated Show resolved Hide resolved
@kasei
Copy link
Contributor

kasei commented Jun 14, 2023

Other than the comment about needing to define Distinct(sequence), I think this looks good.

hartig added 2 commits June 15, 2023 15:03
…f tuples, as is used in the definition of F(Ψ) for the Aggregation operator; see #98 (comment)
…ed over sequences of lists now, rather than multisets of lists
@hartig
Copy link
Contributor Author

hartig commented Jun 15, 2023

I now have changed the definitions of the set functions such that they are defined over sequences of lists, rather than over multisets of lists.

With this change, the PR is ready to be reviewed.

@hartig hartig changed the title WIP: turn the input to Group(..) and to Aggregation(..) into solution sequences Turn the input to Group(..) and to Aggregation(..) into solution sequences Jun 15, 2023
@hartig hartig marked this pull request as ready for review June 15, 2023 14:08
@hartig hartig requested review from afs, rubensworks, Tpt and kasei June 15, 2023 14:09
@hartig hartig added the spec:substantive Change in the spec affecting its normative content (class 3) –see also spec:bug, spec:new-feature label Jun 15, 2023
spec/index.html Outdated Show resolved Hide resolved
Co-authored-by: Ted Thibodeau Jr <[email protected]>
@TallTed
Copy link
Member

TallTed commented Jun 15, 2023

(tangent... could be moved to a new issue)

I'm probably in the minority, and it certainly would require a chunk of work by editors and other contributors, but I find myself wishing that a lot more content were <code> (or `) tagged, especially all the logical expressions. I find such text a good deal more comprehensible with such typographical adjustments, than with most everything being in the main "body" typeface. I'll cheerfully do a lot if not all of the necessary tag additions, if others are OK with it happening.

@gkellogg
Copy link
Member

(tangent... could be moved to a new issue)

I'm probably in the minority, and it certainly would require a chunk of work by editors and other contributors, but I find myself wishing that a lot more content were <code> (or `) tagged, especially all the logical expressions. I find such text a good deal more comprehensible with such typographical adjustments, than with most everything being in the main "body" typeface. I'll cheerfully do a lot if not all of the necessary tag additions, if others are OK with it happening.

var (or synonym |) is also useful markup for things like μ in Ψ

@hartig
Copy link
Contributor Author

hartig commented Jun 16, 2023

I have created two issues related to the two markup suggestions, see #101 and #102.

@hartig
Copy link
Contributor Author

hartig commented Jun 18, 2023

I have pushed a new commit to this PR (see d808fb8). This commit addresses Andy's concerns about my changes to the definition of Distinct by implementing his suggestion to separate the definitions; that is, with this new commit, the definition of Distinct is now back to how it always was and there is a new function called Dedup that is used in the definition of the Aggregation operator. Additionally, this commit addresses the following comments:

Comment by @kasei

This wording seems confusing to me. If there are duplicated elements in Ψ, I would not think of the distinct sequence as "containing every element in Ψ" (since some of them will be omitted because they are duplicates).

Addressed by changing "Every element" to "Every unique element" as suggested by @TallTed .

Comment by @afs

s/≠/is not the same term as/

I kept the ≠ in the condition for the indexes i and j but integrated "is not the same term as" into the text of the corresponding bullet point.

Comment by @afs

s/every/any/ because the text names the chosen as e1 and e2.

Done.

spec/index.html Outdated Show resolved Hide resolved
spec/index.html Outdated Show resolved Hide resolved
Copy link
Contributor

@Tpt Tpt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I allowed myself some questions I had while reviewing this MR but they are not directly related with it.

spec/index.html Show resolved Hide resolved
spec/index.html Show resolved Hide resolved
spec/index.html Show resolved Hide resolved
spec/index.html Show resolved Hide resolved
spec/index.html Outdated Show resolved Hide resolved
@hartig
Copy link
Contributor Author

hartig commented Jun 27, 2023

@afs @rubensworks are you still planning to review this PR?
Otherwise, I would merge it so that we can make progress on a number of related issues (#94 #99 #105 #106 #107, and eventually also #101 #102 #103).

@afs
Copy link
Contributor

afs commented Jun 27, 2023

Yes, I am hoping to review it.

Sorry for the delay.

Reviewing changes in RDF docs has also taken up time.

@hartig
Copy link
Contributor Author

hartig commented Jun 27, 2023

No worries. Take your time! I just wanted to quickly check.

Copy link
Member

@rubensworks rubensworks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay, I lost track of this one.

Copy link
Contributor

@afs afs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. Solution sequences are easier to think about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:substantive Change in the spec affecting its normative content (class 3) –see also spec:bug, spec:new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants