Skip to content

Conversation

@long-tran1
Copy link
Contributor

Sorting on TEXTSTRING mapped fields threw the following ES error, because JG applied sorting to the wrong ES field.

Fielddata is disabled on text fields by default. Set fielddata=true on [#field] in order to load fielddata in memory by uninverting the inverted index. Note that this can however use significant memory. Alternatively use a keyword field instead.

Thank you for contributing to JanusGraph!

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there an issue associated with this PR? Is it referenced in the commit message?
  • Does your PR body contain #xyz where xyz is the issue number you are trying to resolve?
  • Has your PR been rebased against the latest commit within the target branch (typically master)?
  • Is your initial contribution a single, squashed commit?

For code changes:

  • Have you written and/or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository?
  • If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 6, 2021

CLA Signed

The committers are authorized under a signed CLA.

@janusgraph-bot janusgraph-bot added the cla: external Externally-managed CLA label Apr 6, 2021
@li-boxuan
Copy link
Member

Thanks for your contribution! Depending on your firm's policy, you either need to sign the individual CLA or have your firm signed up so that you can join the CCLA. See https://github.com/JanusGraph/janusgraph/blob/master/CONTRIBUTING.md

@li-boxuan li-boxuan requested a review from porunov April 7, 2021 07:13
Copy link
Member

@li-boxuan li-boxuan left a comment

Choose a reason for hiding this comment

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

Thank you, this fix looks good to me!

If possible, could you also add some tests in JanusGraphIndexTest? That is the place where we write integration tests involving mixed index. This will help us make sure the TEXTSTRING sorting functionality works for both ES and Lucene. You may take a look at testDualMapping and either amend it or add a new test method.

@long-tran1 long-tran1 force-pushed the fix-textstring-sort branch from 3968347 to d5f3519 Compare April 8, 2021 14:34
Signed-off-by: Long Tran <long.tran1@fireeye.com>
@long-tran1 long-tran1 force-pushed the fix-textstring-sort branch from d5f3519 to 232b511 Compare April 8, 2021 14:35
@long-tran1
Copy link
Contributor Author

If possible, could you also add some tests in JanusGraphIndexTest? That is the place where we write integration tests involving mixed index. This will help us make sure the TEXTSTRING sorting functionality works for both ES and Lucene. You may take a look at testDualMapping and either amend it or add a new test method.

Added some assertion to the existing testDualMapping test.

Copy link
Member

@li-boxuan li-boxuan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@porunov
Copy link
Member

porunov commented Apr 14, 2021

Thank you @long-tran1 !
Will you be able to sign EasyCLA so that we could merge your contributions?
https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/1543822/77385607/2562/#/?version=1

@long-tran1
Copy link
Contributor Author

Thank you @long-tran1 !
Will you be able to sign EasyCLA so that we could merge your contributions?
https://api.easycla.lfx.linuxfoundation.org/v2/repository-provider/github/sign/1543822/77385607/2562/#/?version=1

Yup! Currently working on it. I'll let you know asap.

@long-tran1
Copy link
Contributor Author

I'm now authorized under a signed CLA.

Copy link
Member

@porunov porunov left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @long-tran1 !

While the failing HBase tests are not related to your PR, I would suggest to wait for #2576 and rebase from master branch as soon as #2576 is merged.

@porunov porunov added this to the Release v0.6.0 milestone Apr 23, 2021
@porunov
Copy link
Member

porunov commented Apr 23, 2021

@li-boxuan , @farodin91 - looks like HBase tests are now fixed. Merging this PR

@porunov porunov merged commit 72df7f5 into JanusGraph:master Apr 23, 2021
@farodin91
Copy link
Contributor

@li-boxuan , @farodin91 - looks like HBase tests are now fixed. Merging this PR

I think bug sounds more like an issue with GitHub Actions. CQL bug did apear round the same time. #2578

@long-tran1 long-tran1 deleted the fix-textstring-sort branch July 7, 2021 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: external Externally-managed CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants