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

Multi-valued hierarchical facets #3182

Merged

Conversation

@wolfgangmm
Copy link
Member

wolfgangmm commented Jan 4, 2020

Extend hierarchical facets to accept multiple values passed in via an array.

Defining a hierarchical facet on a node, it is possible that a node is a member of multiple hierarchies at the same time. For example, a text could fall into several hierarchical subject categories like science/math or humanities/history. Currently this is not supported: we only allow a single hierarchical facet per dimension.

Multiple hierarchies can be easily described as an XQuery array of sequences like [('science', 'math'), ('humanities', 'history')]. This PR extends the facet indexer to check if the result of the XQuery expression to obtain the facet value is an array. If it is, it is treated as a multi-valued hierarchical facet.

Type of tests:

See extended xqsuite test.

Documentation

eXist-db/documentation@148bf8a

@wolfgangmm wolfgangmm requested review from joewiz and eXist-db/core Jan 4, 2020
Copy link
Contributor

duncdrum left a comment

Nice one! I already have a use case for this

final List<String> paths = new ArrayList<>(seq.getItemCount());
for (SequenceIterator i = seq.unorderedIterator(); i.hasNext(); ) {
final String value = i.nextItem().getStringValue();
if (value.length() > 0) {

This comment has been minimized.

Copy link
@dizzzz

dizzzz Jan 4, 2020

Member

functional identical to !String#isEmpty() ; the current is faster for sure.
Is there a NPE risk?

This comment has been minimized.

Copy link
@wolfgangmm

wolfgangmm Jan 7, 2020

Author Member

In line with XQuery specs, getStringValue() on an item should not return null.

This comment has been minimized.

Copy link
@adamretter

adamretter Jan 21, 2020

Member

I think @dizzzz is right here.

@dizzzz
dizzzz approved these changes Jan 4, 2020
Copy link
Member

adamretter left a comment

Just the arraylist -> array thing

tuurma added a commit to eXist-db/documentation that referenced this pull request Jan 6, 2020
@joewiz
joewiz approved these changes Jan 6, 2020
Copy link
Member

joewiz left a comment

This is a very useful extension to the facets facility.

@wolfgangmm

This comment has been minimized.

Copy link
Member Author

wolfgangmm commented Jan 7, 2020

As @tuurma just pointed out to me, this PR needs to be slightly extended to be really useful, so please do not merge yet. I'll update it today or tomorrow.

@wolfgangmm

This comment has been minimized.

Copy link
Member Author

wolfgangmm commented Jan 7, 2020

Ok, I added one more commit to also cover the query side of things and am done now. Documentation will be coming...

@tuurma

This comment has been minimized.

Copy link
Contributor

tuurma commented Jan 10, 2020

accompanying documentation PR now in, ready to merge

@tuurma
tuurma approved these changes Jan 10, 2020
@dizzzz

This comment has been minimized.

Copy link
Member

dizzzz commented Jan 21, 2020

image

Copy link
Member

dizzzz left a comment

please could you resolve the merge conflict?

@joewiz joewiz merged commit 15663d8 into eXist-db:develop Jan 23, 2020
3 checks passed
3 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.