gh-282 ToIterable #286
gh-282 ToIterable #286
Conversation
GCHQDev404
force-pushed
the
gh-282-ToIterable
branch
from
c846c58
to
781866d
Compare
Codecov Report
@@ Coverage Diff @@
## develop #286 +/- ##
=============================================
+ Coverage 82.08% 82.13% +0.04%
- Complexity 1616 1621 +5
=============================================
Files 194 195 +1
Lines 4316 4328 +12
Branches 435 437 +2
=============================================
+ Hits 3543 3555 +12
Misses 592 592
Partials 181 181
Continue to review full report at Codecov.
|
|
@GCHQDev404 @t92549 Should this PR be pointing at a 2.0 branch rather than develop? |
Koryphe is on version 2.5.0 now so develop is tracking version 2 |
core/src/main/java/uk/gov/gchq/koryphe/impl/function/ToIterable.java
Outdated
Show resolved
Hide resolved
core/src/test/java/uk/gov/gchq/koryphe/impl/function/ToIterableTest.java
Show resolved
Hide resolved
core/src/test/java/uk/gov/gchq/koryphe/impl/function/ToIterableTest.java
Show resolved
Hide resolved
| } else if (value.getClass().isArray()) { | ||
| try { | ||
| return Arrays.asList((Object[]) value); | ||
| } catch (final ClassCastException e) { | ||
| throw new UnsupportedOperationException("The given array is not of type Object[]", e); | ||
| } | ||
| } else { | ||
| rtn = Collections.singletonList(value); | ||
| } | ||
| return rtn; |
There was a problem hiding this comment.
This isn't consistent with the other Koryphe functions - see ToList.
The test should be value instanceof Object[] to see if it's an array of Object. There shouldn't be an exception because the cast won't fail. If value is an Array of primitives then it should be wrapped in the same way as happens for ToList. This is potentially unexpected behaviour, but if that is to be improved/fixed (e.g. so that primitives are supported), it should be done for all of Koryphe and in a separate issue.
There was a problem hiding this comment.
Is this function not just a duplication of ToList? As it is making new Lists anyway
There was a problem hiding this comment.
It returns Iterable<Object>, whereas ToList returns List<?>. I think you might be onto something here though. Instead of creating ToIterable, it might be possible to just cast or otherwise convert the result of ToList into an Iterable<Object>. If that can be done easily enough, then this class is redundant and shouldn't be merged.
There was a problem hiding this comment.
See the comment in Gaffer PR #2769
|
not required |
|
This likely this is better with lazy Iterable, that ToList. 🔥 Phoenix |
| rtn = Collections.emptyList(); | ||
| } else if (value instanceof Iterable) { | ||
| //noinspection unchecked | ||
| rtn = (Iterable<Object>) value; |
There was a problem hiding this comment.
This line is key, if the input is a lazy iterable the same iterable it returned, whereas in ToList the whole iterable has to be read to make a list
There was a problem hiding this comment.
Wouldn't it be possible to change ToList to have a flag to do this instead of a duplicating class?
I know there was a reason Koryphe is being used somewhere like this, but if we know we have an Iterable, it doesn't make sense to me to be using Koryphe to turn something which is already an Iterable into itself. So while ToList is not a good solution, I'm not sure of the reason for using Koryphe at all.
It makes sense for Koryphe to accept a list and return the same thing, but I don't see why anyone would do this unless they didn't know they already had a list.
There was a problem hiding this comment.
You don't know what is being returned from the individual graphs, until its returned.
3 graphs could return:
3 nulls
3 iterates of elements
3 schemas
3 strings
3 lists of strings.
|
Closed again as not needed, superseded by gchq/Gaffer#2831 |
Related Issue