[ADJvoENI] apoc.schema.properties.distinct(Count) can hang #257
Conversation
vga91
force-pushed
the
apoc.schema.properties-can-hang
branch
2 times, most recently
from
1df2697
to
e78fda6
Compare
There was a problem hiding this comment.
I think it would be ok to add support for fulltext index (we have e.g. added support for new types of constraints recently), but the way you have implemented it will not work. I would say in case you manage to write a good filtering step, we can add the support, otherwise we can document that it is not supported and skip it rather than hang the procs.
Also had some ideas for tests and documentation, where the docs things can be added to the already existing Trello card
| WITH * | ||
| CALL apoc.schema.properties.distinct("LabelNotExistent", "prop") | ||
| YIELD value RETURN *""", | ||
| r -> assertEquals(Collections.emptyList(), r.get("value")) |
There was a problem hiding this comment.
Normally you can see your own writes in Cypher, so if possible I think this should return 2. If that is not feasible, I think we should at least document that properties created in the same transaction will not be shown. You could add it to this card perhaps: https://trello.com/c/MBiu8YIz/1827-improve-documentation-about-apocschemapropertiesdistinctcount
| try { | ||
| indexSession = read.indexReadSession( indexDescriptor ); | ||
| }catch (Exception e) { | ||
| // we skip indexScan if it's still populating |
There was a problem hiding this comment.
Maybe it would be good to add something about this in the documentation. Should in that case be part of this card https://trello.com/c/MBiu8YIz/1827-improve-documentation-about-apocschemapropertiesdistinctcount which is about adding that you need an index for these procedures
There was a problem hiding this comment.
Sorry, I missed this point. I added it as a checklist's item
|
Yes, I've started making the various changes, I'll finish this pr asap |
vga91
force-pushed
the
apoc.schema.properties-can-hang
branch
2 times, most recently
from
d2f2493
to
c0adb5a
Compare
There was a problem hiding this comment.
I think the behaviour is OK now, but there is still some edge cases users might be surprised over. Take a look at my new comments and let me know what you think. I can collect all docs improvements into a checklist on the already existing docs card if you like
vga91
force-pushed
the
apoc.schema.properties-can-hang
branch
from
3bd7141
to
11e1c00
Compare
vga91
force-pushed
the
apoc.schema.properties-can-hang
branch
from
11e1c00
to
2766d1e
Compare
…c#257) * [ADJvoENI] apoc.schema.properties.distinct(Count) can hang * [ADJvoENI] changes review * [ADJvoENI] moved getProperty method - added distinct method in order to remove idxs duplicates
…c#257) (#3404) * [ADJvoENI] apoc.schema.properties.distinct(Count) can hang (neo4j/apoc#257) * [ADJvoENI] apoc.schema.properties.distinct(Count) can hang * [ADJvoENI] changes review * [ADJvoENI] moved getProperty method - added distinct method in order to remove idxs duplicates * [ADJvoENI] fixed compilation errs
The test cases about 2nd scenario (ie fulltext indexes) is to be completed,
because it is to understand if the solution is good or not,
as it is in limbo between a bugfix and a feature.
That is, currently fulltext indexes are just not supported, hence the infinite hang.
To solve I created several
if(isFulltexIndexes) { fullTextImplementation } else { currentImplementation}.Alternatively we can do something like
if(isFulltexIndexes) { skipCurrentFulltextIndex }, and document this.But I think this is not a correct behavior, because as a user I would expect
apoc.schema.propertiesto support any type of index, so maybe this is a bugfix and not a feature.Wdyt?