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

ConcatenateListMergeFunction should return lazy iterables #2829

Closed
t92549 opened this issue Nov 23, 2022 · 3 comments · Fixed by #2831
Closed

ConcatenateListMergeFunction should return lazy iterables #2829

t92549 opened this issue Nov 23, 2022 · 3 comments · Fixed by #2831
Assignees
Labels
enhancement Improvement to existing functionality/feature federated-store Specific to/touches the federated-store module

Comments

@t92549
Copy link
Contributor

t92549 commented Nov 23, 2022

Describe the new feature you'd like
The federated store's ConcatenateListMergeFunction should return results in a lazy iterable.

Why do you want this feature?
Pre v2.0 the results were in a lazy iterable so this functionality should be retained.

Additional context
Currently, the koryphe function ToList is used to turn all results into iterables. However, this stops the results from being returned lazily as they are read into a list:

final Iterable<Object> updateSafe = isNull(update) ? Collections.emptyList() : (Iterable<Object>) new ToList().apply(update);

As well as this, Lists.newArrayList has the same issue:
Koryphe functions do not really need to be used at all here in fact, and IterableConcat can be replaced by directly using a ChainedIterable, which is what used to happen and what is happening indirectly anyway.

If the proposed changes were implemented, then gchq/koryphe#286 can be closed.

As well as these functionality changes, there should be proper javadoc and tests added to this class to ensure it works as expected and the expected behaviour is documented.

Additionally, I would argue the class should be renamed to ConcatenateMergeFunction and ConcatenateSetMergeFunction should be deleted.
This is because results are concatenated whether they are in a list or not, and returned in an Iterable. The results are not necessarily returned in a list. If a user specifically wanted a list or set return type, they could use ToList or ToSet in their operation chain.

@t92549 t92549 added enhancement Improvement to existing functionality/feature federated-store Specific to/touches the federated-store module labels Nov 23, 2022
@t92549 t92549 added this to the v2.0.0-alpha-0.4 milestone Nov 23, 2022
@GCHQDev404
Copy link
Contributor

GCHQDev404 commented Nov 24, 2022

ChainedIterable requires an Array of Iterables to be given to the constructor.

You don't know what is being returned from the individual graphs, until it's returned and they may not be Iterables.

3 graphs could return:
3 nulls
3 iterates of elements
3 schemas
3 strings
3 lists of strings.

Also you need to have all the iterates available as the same time which doesn't work the same way as an BiFunction being looped and updated.


edit

are you talking about change this line new IterableConcat<>().apply(Lists.newArrayList(updateSafe, state)); to ChainedIterable ?

@t92549
Copy link
Contributor Author

t92549 commented Nov 24, 2022

I understand you need to be flexible, but the way it used to be done retained lazy iterables, whereas now it does not:

t92549 added a commit that referenced this issue Jan 3, 2023
…2831)

* Refactor ConcatenateListMergeFunction to have lazy iterables
Remove creation of list/array/collection except for when object is not already an iterable

* Rename merge function and remove Set function

* Added more renames

* Added javadoc and comments

* Added tests

* Added iterable null test and arrays test

* Change ConcatenateMergeFunction to convert Arrays to Iterables

* Add further tests for Arrays and test for Integer

* Fix test name and add extra test

* Remove redundant instanceOf asserts

Co-authored-by: t92549 <80890692+t92549@users.noreply.github.com>
@t92549
Copy link
Contributor Author

t92549 commented Jan 3, 2023

Closed by #2831

@t92549 t92549 closed this as completed Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to existing functionality/feature federated-store Specific to/touches the federated-store module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants