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

[dITjfMpu] apoc.coll.indexOf unexpectedly treats collections differently than the same hardcoded list #422

Merged
merged 3 commits into from
May 29, 2023

Conversation

vga91
Copy link
Collaborator

@vga91 vga91 commented May 23, 2023

Potentially this behaviour can affect a lot of procedures/functions, both apoc.coll and elsewhere.
I changed some of them, just to discuss about a possible solution that can be good for each procedure/fun that may have the problem.
For the other cases I created a follow-up card (id: v8imq7eo)

I think the possible solution are:

  • Util.toAnyValues(..), as done in apoc.coll.toSet, which transform every object in AnyValue
    • pros: less code to change, just wrap the lists with that method
    • cons: tranform Object in AnyValue, so e.g an hardcoded [1,2,3] is converted from ArrayList to long[], so it could be considered a breaking change
  • Create Util method that leverage ValueUtils.of(), as done in apoc.coll.indexOf and apoc.coll.containsAll
    • pros: no need to loop through lists to transform to AnyValue
    • cons: e.g with an apoc.coll.toSet(<NodePropValue>, ... <HardcodedValue>), if the two ..Value are equal, the function will return the nodeProp or the hardcoded based on which one finds first.

@vga91 vga91 marked this pull request as draft May 23, 2023 12:22
@vga91 vga91 force-pushed the issue-coll-indexOf-and-others branch from 14bb406 to e38bab4 Compare May 24, 2023 10:29
@vga91 vga91 changed the title [dITjfMpu] [WIP] apoc.coll.indexOf unexpectedly treats collections differently than the same hardcoded list [dITjfMpu] apoc.coll.indexOf unexpectedly treats collections differently than the same hardcoded list May 24, 2023
@vga91 vga91 force-pushed the issue-coll-indexOf-and-others branch from e38bab4 to 633dd90 Compare May 24, 2023 23:08
@vga91 vga91 force-pushed the issue-coll-indexOf-and-others branch from 633dd90 to f73f95c Compare May 24, 2023 23:23
@vga91 vga91 marked this pull request as ready for review May 24, 2023 23:24
// query that procedures a list,
// with both entity types, via collect(..), and hardcoded one
private static final String QUERY_WITH_MIXED_TYPES = "MATCH (n:Test) " +
"WITH n ORDER BY n.a " +
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use the new COLLECT instead to make it more readable :) Also maybe multi line instead of having addition as that is also a little hard to read:

WITH COLLECT {
MATCH (n:Test)
RETURN {something: n.something}
ORDER BY n.a} + { something: []
} + {something: 'alpha'} + {something: [1,2,3]} AS collection
RETURN apoc.coll.indexOf(collection, { something: [] }) AS index

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed

r -> {
long index = (long) r.get("index");
assertTrue("Actual index is " + index,
index == 1L || index == 2L);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are doing an ORDER BY (and change to using COLLECT subquery over function), then it should always be 1?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also did a test on another apoc.coll proc:

WITH COLLECT { 
    MATCH (n:Test) 
    RETURN n.something
    ORDER BY n.a}  + [[]] + [[1,2,3]] AS collection
CALL apoc.coll.elements(collection) 
YIELD _1,  _1l,
      _2,  _2l,
      _3, _3l,
      _4,  _4l,
      _5,  _5l,
      _6, _6l
RETURN _1,  _1l,
       _2,   _2l,
       _3,   _3l,
      _4,  _4l,
      _5,  _5l,
      _6, _6l

And it also doesn't work (lists are returned as null if they are from the collect).

There are a lot of coll funcs and procs, but I guess it would make sense for all to be fixed for this issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added the apoc.coll.elements array handling.
I think it's only a problem of that procedure, at least regarding the apoc.coll ones,
since the method I changed is only called by that function.
But potentially other pieces of code could have that problem,
so I'd create another follow-up card to investigate about that. Wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or maybe, we can fix the apoc.coll.elements directly in the new follow-up card,
together with the research/fix about other analogous problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, a new card is fine :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created the follow-up card (id: gtjPFvMD)

WITH COLLECT {
MATCH (n:Test)
RETURN {something: n.something}
ORDER BY n.a} + { something: [] } + {something: 'alpha'} + {something: [1,2,3]} AS collection\s
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the \s here for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was just a whitespace which intellij had automatically added.
Removed :)

@vga91 vga91 force-pushed the issue-coll-indexOf-and-others branch from 7667301 to f460d67 Compare May 29, 2023 12:42
Copy link
Contributor

@gem-neo4j gem-neo4j left a comment

Choose a reason for hiding this comment

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

LGTM :)

@vga91 vga91 merged commit 2c1f37c into dev May 29, 2023
@vga91 vga91 deleted the issue-coll-indexOf-and-others branch May 29, 2023 13:25
vga91 added a commit to neo4j-contrib/neo4j-apoc-procedures that referenced this pull request May 30, 2023
…tly than the same hardcoded list (neo4j/apoc#422)

* [dITjfMpu] apoc.coll.indexOf unexpectedly treats collections differently than the same hardcoded list

* [dITjfMpu] various changes

* [dITjfMpu] small review changes
vga91 added a commit to neo4j-contrib/neo4j-apoc-procedures that referenced this pull request May 30, 2023
…tly than the same hardcoded list (neo4j/apoc#422)

* [dITjfMpu] apoc.coll.indexOf unexpectedly treats collections differently than the same hardcoded list

* [dITjfMpu] various changes

* [dITjfMpu] small review changes
vga91 added a commit to neo4j-contrib/neo4j-apoc-procedures that referenced this pull request May 31, 2023
…tly than the same hardcoded list (neo4j/apoc#422) (#3600)

* [dITjfMpu] apoc.coll.indexOf unexpectedly treats collections differently than the same hardcoded list

* [dITjfMpu] various changes

* [dITjfMpu] small review changes
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.

2 participants