Skip to content

WIP: V3 Sync Unions#65

Closed
jeswr wants to merge 5 commits into
mainfrom
v3-union
Closed

WIP: V3 Sync Unions#65
jeswr wants to merge 5 commits into
mainfrom
v3-union

Conversation

@jeswr

@jeswr jeswr commented Apr 10, 2022

Copy link
Copy Markdown
Collaborator

Same idea as #54 but targeted for v3.x.

#63 and #59 should be merged first

Now also resolves #67

Comment thread linkedlist.ts Outdated
Comment thread linkedlist.ts
Comment thread linkedlist.ts
Comment thread asynciterator.ts Outdated
Comment thread asynciterator.ts Outdated
Comment thread asynciterator.ts Outdated
Comment thread asynciterator.ts Outdated
@jeswr

jeswr commented Apr 15, 2022

Copy link
Copy Markdown
Collaborator Author

Note: the failing test is because .map is buffering. This will be resolved once we rebase to #59

@jeswr jeswr requested a review from jacoscaz April 17, 2022 14:44

@jacoscaz jacoscaz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nicely done, I like how the maxBufferSize is used to limit the number of source the iterator reads from. These are just minor nitpicks.

Comment thread asynciterator.ts
prepend(items: T[] | AsyncIterator<T>): AsyncIterator<T> {
return this.transform({ prepend: items });
return new UnionIterator(
[Array.isArray(items) ? new ArrayIterator(items, { autoStart: false }) : items, this],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

After #63 is merged, this could become simply wrap(items, { letIteratorThrough: true }) (thus supporting a wider range of types for items).

Comment thread asynciterator.ts
append(items: T[] | AsyncIterator<T>): AsyncIterator<T> {
return this.transform({ append: items });
return new UnionIterator(
[this, Array.isArray(items) ? new ArrayIterator(items, { autoStart: false }) : items],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same as above

Comment thread asynciterator.ts
const sources = this._pending!.sources!;
delete this._pending!.sources;
// @ts-ignore
this._sources = (Array.isArray(sources) ? fromArray(sources) : wrap(sources)).map<AsyncIterator<T>>((it: any) => isPromise(it) ? wrap(it as any) : (it as AsyncIterator<T>));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this could also be simplified after #63

Comment thread asynciterator.ts
private _currentSource = -1;
export class UnionIterator<T> extends AsyncIterator<T> {
private _sources : AsyncIterator<AsyncIterator<T>>;
private buffer = new LinkedList<AsyncIterator<T>>();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this needs a _ prefix.

Comment thread linkedlist.ts
mutateFilter(filter: (item: V) => boolean) {
let last: LinkedNode<V> | null;
let next: LinkedNode<V> | null;
while (this._head !== null && !filter(this._head.value)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suspect this could be simplified into one single loop if we were to have the list itself implement the LinkedNode interface, referencing the first node in the chain as next instead of _head. It might not be worth the effort, though.

@jeswr

jeswr commented Apr 20, 2022

Copy link
Copy Markdown
Collaborator Author

Thanks for the review - I'll wait for #63 to be merged and the rebase and clean this up :)

Comment thread asynciterator.ts
private _sources : InternalSource<T>[] = [];
private _pending? : { sources?: AsyncIterator<MaybePromise<AsyncIterator<T>>> };
private _currentSource = -1;
export class UnionIterator<T> extends AsyncIterator<T> {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Document whether or not it is in-order; crucial to prepend and append.

@jeswr

jeswr commented Aug 1, 2022

Copy link
Copy Markdown
Collaborator Author

Did work getting this up to date on the train. Will push changes later today

jeswr referenced this pull request in comunica/comunica Aug 1, 2022
jeswr added a commit that referenced this pull request Aug 1, 2022
@jeswr jeswr mentioned this pull request Aug 1, 2022
@jeswr jeswr closed this Aug 1, 2022
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 this pull request may close these issues.

Performance: Improve performance of append and prepend

3 participants