Join() support

[vinesworth]

Hi,

it's been a while and now I'm finally stuck into absense of joins, and going to implement it. After having analysed revision history, it looks like ��it would be sufficient to:

• add JoinOperation, likely based on SelectManyOperation or GroupByOperation;
• update OperationFactory's methods FromExpression() and FromQueryableExpression();
• add tests accordingly.

Is it all?

[wasabii]

Believe so! Thanks!
On May 8, 2014 6:16 AM, "vinesworth" [email protected] wrote:

Hi,

it's been a while and now I'm finally stuck into absense of joins, and
going to implement it. After having analysed revision history, it looks
like it would be sufficient to:

• add JoinOperation, likely based on SelectManyOperation or
  GroupByOperation;
• update OperationFactory's methods FromExpression() and
  FromQueryableExpression();
• add tests accordingly.

Is it all?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/7
.

[vinesworth]

Hi again :)

I'm currently in the phase of making the unit-tests pass. Occasionally it requires me to fix minor issues in the code other than mine. So, here are some questions regarding that, one at a time.

1. Notifications of GroupByOperation are likely flawed. Here, a new empty group is created and reported as added:

group = groups[key] = new Grouping<TKey, TElement>(key);
NotifyCollectionChangedUtil.RaiseAddEvent<TElement>(RaiseCollectionChanged, group);

But, the way how the event is raised makes it effectively useless.

raise(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, newItems.ToList()));

Obviously, after .ToList() one can neither cast it back to Grouping<> nor even access the key. What's the purpose of having .ToList() there?

[wasabii]

NotifyCollectionChangedEventArgs (from System.Collection.Specialized) only
supports accepting an IList. You have to pass it as a IList.

It might make sense to implement IList on Grouping. That would allow you to
avoid this. The default IGrouping interface doesn't require it, however.

On Tue, Aug 19, 2014 at 8:58 AM, vinesworth [email protected]
wrote:

Hi again :)

I'm currently in the phase of making the unit-tests pass. Occasionally it
requires me to fix minor issues in the code other than mine. So, here are
some questions regarding that, one at a time.

1. Notifications of GroupByOperation are likely flawed. Here
   https://github.com/wasabii/OLinq/blob/master/OLinq/GroupByOperation.cs#L122-L123,
   a new empty group is created and reported as added:

group = groups[key] = new Grouping<TKey, TElement>(key);NotifyCollectionChangedUtil.RaiseAddEvent(RaiseCollectionChanged, group);

But, the way how the event is raised
https://github.com/wasabii/OLinq/blob/master/OLinq/NotifyCollectionChangedUtil.cs#L15
makes it effectively useless.

raise(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, newItems.ToList()));

Obviously, after .ToList() one can neither cast it back to Grouping<> nor
even access the key. What's the purpose of having .ToList() there?

—
Reply to this email directly or view it on GitHub
#7 (comment).

[wasabii]

After a bit of thought, I'd totally support implementing IList on Grouping.
Patches welcome!

[vinesworth]

Hmm. Isn't the grouping supposed to be the new item itself?

class GroupByOperation<TElement, TKey>
    : EnumerableSourceWithLambdaOperation<TElement, TKey, IEnumerable<IGrouping<TKey, TElement>>>,
      IEnumerable<IGrouping<TKey, TElement>>,
      INotifyCollectionChanged

I'd rather expect NotifyCollectionChangedEventArgs.NewItems to contain what the IEnumerable<> does, which is IGrouping<>s themselves, not single TElements... And to watch for the element arrivals I'd subscribe to the groupings. Am I wrong somewhere?

[wasabii]

There are two sets of collections involved: the List and each
individual Grouping.

When a new Grouping is added to the output, subscribers need to know that.

When a new item is added to any particular Grouping, subscribers need to
know that, as well.

[vinesworth]

Right. But there's only one CollectionChanged event, and it doesn't provide a clean way to differentiate what has actually been added: a group or a value. One might try to look at the actual type of NotifyCollectionChangedEventArgs.NewItems, but I personally feel there might be some unobvious trouble (e.g. null values)... And even if one would know that he's got an item, how could he tell what grouping it belongs to?

[wasabii]

Groups are the only thing that are added when you're in the
GroupByOperation. The Group itself takes care of notifying about it's
contents.

On Wed, Aug 20, 2014 at 5:42 PM, vinesworth [email protected]
wrote:

Right. But there's only one CollectionChanged event, and it doesn't
provide a clean way to differentiate what has actually been added: a group
or a value. One might try to look at the actual type of
NotifyCollectionChangedEventArgs.NewItems, but I personally feel there
might be some unobvious trouble (e.g. null values)... And even if one
would know that he's got an item, how could he tell what grouping it
belongs to?

—
Reply to this email directly or view it on GitHub
#7 (comment).