Index phrases #30450

romseygeek · 2018-05-08T08:49:50Z

This commit adds an index_phrases option to text fields, which if enabled creates a subsidiary field containing two-term shingles from the main field. Phrase match queries are then redirected to this subsidiary field.

The index prefix field is normally indexed as docs-only, given that it cannot be used in phrases. However, in the case that the parent field has been indexed with offsets, or has term-vector offsets, we should also store this in the index prefix field for highlighting. Note that this commit does not implement highlighting on prefix fields, but rather ensures that future work can implement this without a backwards-break in index data. Closes #28994

Specifying `index_phrases: true` on a text field mapping will add a subsidiary [field]._index_phrase field, indexing two-term shingles from the parent field. The parent analysis chain is re-used, wrapped with a FixedShingleFilter. At query time, if a phrase match query is executed, the mapping will redirect it to run against the subsidiary field. This should trade faster phrase querying for a larger index and longer indexing times.

elasticmachine · 2018-05-08T08:50:56Z

Pinging @elastic/es-search-aggs

jimczi

I left some comments. I think we should have a simple function in MappedFieldType that can build phrase queries from a text+analyzer. This would simplify the usage and keep the focus on phrase queries building.

jimczi · 2018-05-09T07:56:57Z

server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java

+
+        PhraseFieldType(String name) {
+            setTokenized(true);
+            setIndexOptions(IndexOptions.DOCS_AND_FREQS_AND_POSITIONS);


We should index offsets if the parent field activated them.

jimczi · 2018-05-09T07:57:33Z

server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java

+                protected Query analyzePhrase(String field, TokenStream stream, int slop) throws IOException {
+                    assert slop == 0;
+                    Query q = super.analyzePhrase(field + FAST_PHRASE_SUFFIX, new FixedShingleFilter(stream, 2), slop);
+                    logger.info("Phrase query: " + q);


debug, trace ?

what happens if you search for a single term in a phrase query ? Shouldn't we fallback to the parent field since it won't match the shingles ?

I'm also wondering what happens in case the token stream has gaps because of stop words since we can't use the shingle field in that case?

It should work since FixedShingleFilter adds gap in the shingle in the form of a filler token (defaults to _).

jimczi · 2018-05-09T08:08:53Z

plugins/analysis-icu/src/main/java/org/elasticsearch/index/analysis/IcuTokenizerFactory.java

@@ -80,7 +80,7 @@ private ICUTokenizerConfig getIcuConfig(Environment env, Settings settings) {
            if (tailored.isEmpty()) {
                return null;
            } else {
-                final BreakIterator breakers[] = new BreakIterator[UScript.CODE_LIMIT];
+                final RuleBasedBreakIterator breakers[] = new RuleBasedBreakIterator[UScript.CODE_LIMIT];


This change is already in master

jimczi · 2018-05-09T08:08:59Z

plugins/analysis-icu/src/main/java/org/elasticsearch/index/analysis/IcuTokenizerFactory.java

@@ -105,7 +105,7 @@ public RuleBasedBreakIterator getBreakIterator(int script) {
    }

    //parse a single RBBi rule file
-    private BreakIterator parseRules(String filename, Environment env) throws IOException {
+    private RuleBasedBreakIterator parseRules(String filename, Environment env) throws IOException {


jimczi · 2018-05-09T08:14:07Z

server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java

@@ -360,6 +361,10 @@ public Query nullValueQuery() {

    public abstract Query existsQuery(QueryShardContext context);

+    public MatchQuery matchQuery(QueryShardContext context, String analyzer, int slop) {


Can this be changed to analyzePhrase(QueryShardContext context, String text, Analyzer analyzer, int slop) ?
It seems to much to allow this override so I'd prefer something more focus on the real purpose.

or even further: phraseQuery(BytesRef[] terms, int[] positions), ie. only a factory for phrase queries and still leave analysis up to the query parser

+1, I think the FixedShingleFilter should be able to produce the shingles from the terms and positions even if there are gaps in the phrase query (produced by stop filter).

@jimczi I would be cautious about that. This optimization should return the same hits as a phrase query on the original field. On the regular field, a query for "the fox" would be parsed as a term query on fox, so it feels wrong to me that we run it as a term query on _ fox on the shingle field, as it would have different matches. I'd rather like to document that this option can't optimize queries with stop words.

Thanks for clarifying. I agree this is trappy so +1 to disable this optim when gaps are detected and to add documentation about this limitation.

I've changed the signature to analyzePhrase(String, TokenStream, int), and copied the logic from QueryBuilder#analyzePhrase, which simplifies things a lot. Stop words should be handled automatically, because QueryBuilder itself only calls analyzePhrase if the tokenstream has length > 1. So "the fox" would be analyzed to just fox and parsed as a term query, while "Reynard the fox" would get converted to a phrase query for "reynard _" "_ fox"

We might not want the latter, since the phrase query on the regular field would match everything that has fox 2 positions after Reynard, while a phrase query for "reynard _" "_ fox" adds the additional constraint that there is no token between them. For instance "Reynard cunning fox" would match on the regular field but not on the shingle field.

Even though this might better fit the original intention of the user, I'd like this feature to be as transparent as possible and not change the set of matches for a given query?

jimczi · 2018-05-09T08:18:46Z

server/src/main/java/org/elasticsearch/index/query/MatchPhraseQueryBuilder.java

-        }
-        matchQuery.setPhraseSlop(slop);
+        MappedFieldType fieldType = context.fieldMapper(fieldName);
+        MatchQuery matchQuery = fieldType.matchQuery(context, analyzer, slop);


Will the same work for multi_match, query_string and simple_query_string ? I don't think you need to override match_query, calling MappedFieldType#analyzePhrase should be enough ? There's also an issue for query time synonyms where we use span queries instead of phrases but this can be done in a follow up.

jpountz

I left comments but I'm very excited about this feature! I'm curious whether it would integrate directly with the match phrase prefix query or whether some work needs to be done there too to make it perform efficient phrases when shingles are indexe?

jpountz · 2018-05-09T13:18:24Z

docs/CHANGELOG.asciidoc

@@ -150,6 +150,9 @@ option. ({pull}30140[#29658])
 A new analysis plugin called `analysis_nori` that exposes the Lucene Korean
 analysis module.  ({pull}30397[#30397])

+Text fields now have an `index_phrases` parameter that allows faster phrase queries


as a non native speaker it feels weird that index_prefix is singular while index_phrases is plural?

jpountz · 2018-05-09T13:21:49Z

server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java

@@ -360,6 +361,10 @@ public Query nullValueQuery() {

    public abstract Query existsQuery(QueryShardContext context);

+    public MatchQuery matchQuery(QueryShardContext context, String analyzer, int slop) {


or even further: phraseQuery(BytesRef[] terms, int[] positions), ie. only a factory for phrase queries and still leave analysis up to the query parser

jpountz · 2018-05-09T13:32:56Z

docs/reference/mapping/types/text.asciidoc

+
+    If enabled, two-term word combinations ('shingles') are indexed into a separate
+    field.  This allows phrase queries to run more efficiently, at the expense
+    of a larger index.  Accepts `true` or `false` (default).


Maybe we should document that this feature works better when stop words are not removed?

jpountz · 2018-05-09T13:35:50Z

server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java

+                protected Query analyzePhrase(String field, TokenStream stream, int slop) throws IOException {
+                    assert slop == 0;
+                    Query q = super.analyzePhrase(field + FAST_PHRASE_SUFFIX, new FixedShingleFilter(stream, 2), slop);
+                    logger.info("Phrase query: " + q);


I'm also wondering what happens in case the token stream has gaps because of stop words since we can't use the shingle field in that case?

jimczi

Looks good @romseygeek . I left some comments regarding synonyms and some query option that we need to handle.

jimczi · 2018-05-22T08:21:24Z

server/src/main/java/org/elasticsearch/index/query/MatchPhraseQueryBuilder.java

-        if (analyzer != null) {
-            matchQuery.setAnalyzer(analyzer);
-        }
+        matchQuery.setAnalyzer(analyzer);


Why is this check not needed ? setAnalyzer will throw an exception if analyzer is null.

jimczi · 2018-05-22T08:26:07Z

server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java

+
+            stream.reset();
+            while (stream.incrementToken()) {
+                position += posIncrAtt.getPositionIncrement();


There is an option in query_string to disallow position increment (enable_position_increments) so I guess that we should apply it here too.

jimczi · 2018-05-22T08:28:43Z

docs/reference/mapping/types/text.asciidoc

+
+    If enabled, two-term word combinations ('shingles') are indexed into a separate
+    field.  This allows phrase queries to run more efficiently, at the expense
+    of a larger index.  Accepts `true` or `false` (default).


jimczi · 2018-05-22T08:32:03Z

server/src/test/java/org/elasticsearch/index/mapper/TextFieldMapperTests.java

+
+        Query q3 = new MatchPhraseQueryBuilder("field", "two words").slop(1).toQuery(queryShardContext);
+        assertThat(q3, is(new PhraseQuery(1, "field", "two", "word")));
+


Maybe add a test with a single term ?

jimczi · 2018-05-22T08:44:20Z

server/src/main/java/org/elasticsearch/index/search/MatchQuery.java

@@ -360,7 +360,7 @@ protected Query analyzePhrase(String field, TokenStream stream, int slop) throws
                    throw exc;
                }
            }
-            Query query = super.analyzePhrase(field, stream, slop);
+            Query query = mapper.analyzePhrase(field, stream, slop);


analyzePhrase is called when there are no synonyms in the query so I think we should also override analyzeMultiPhrase to handle single term synonyms. Since the fixed size shingle filter doesn't handle multi-terms synonyms there is no need to override analyzeGraphPhrase which should continue to use the main field.

jpountz · 2018-05-22T16:21:33Z

server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java

            return new TextFieldMapper(
-                    name, fieldType, defaultFieldType, positionIncrementGap, prefixMapper,
+                    name, fieldType, defaultFieldType, positionIncrementGap, prefixMapper, indexPhrases,


we do not need to pass it to TextFieldMapper if it is already in the FieldType?

jpountz · 2018-05-22T16:24:16Z

server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java

@@ -420,6 +513,10 @@ void setPrefixFieldType(PrefixFieldType prefixFieldType) {
            this.prefixFieldType = prefixFieldType;
        }

+        void indexPhrases(boolean indexPhrases) {
+            this.indexPhrases = indexPhrases;
+        }


you should update TextFieldTypeTests to add a new modifier for this property

I'd missed TextFieldTypeTest - I also need to add in checks for index_prefix here. I agree with you that it's odd to have index_prefix and index_phrases, so I think we should change the former to be plural as well. I'll open a separate PR for that change, and to add the TextFieldTypeTest modifications for index_prefix

jpountz · 2018-05-22T16:28:57Z

server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java

@@ -455,6 +552,28 @@ public Query nullValueQuery() {
            return termQuery(nullValue(), null);
        }

+        @Override
+        public Query analyzePhrase(String field, TokenStream stream, int slop) throws IOException {
+            if (indexPhrases && slop == 0) {


this should also check that there are no gaps in the stream

jpountz

I left some comments/questions but I like it a lot and think it's very close now. I didn't see any validation around the index options but I think we should disallow enabling this option if the index options do not index positions?

jpountz · 2018-05-25T07:35:37Z

server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java

@@ -360,6 +362,10 @@ public Query nullValueQuery() {

    public abstract Query existsQuery(QueryShardContext context);

+    public Query analyzePhrase(String field, TokenStream stream, int slop) throws IOException {


Maybe phraseQuery would be more consistent with other methods here (existsQuery, termQuery, rangeQuery)?

jpountz · 2018-05-25T07:36:06Z

server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java

@@ -360,6 +362,10 @@ public Query nullValueQuery() {

    public abstract Query existsQuery(QueryShardContext context);

+    public Query analyzePhrase(String field, TokenStream stream, int slop) throws IOException {
+        throw new IllegalArgumentException("Can only use phrase queries on keyword and text fields - not on [" + name + "] which is of type [" + typeName() + "]");


Maybe only mention text fields in the error message, even if we don't fail phrase queries on keywords, I'm not sure this is very useful?

jpountz · 2018-05-25T07:39:24Z

server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java

@@ -106,6 +119,11 @@ public Builder fielddata(boolean fielddata) {
            return builder;
        }

+        public Builder indexPhrases(boolean indexPhrases) {
+            this.indexPhrases = indexPhrases;


should we directly set on the field type?

jpountz · 2018-05-25T07:43:03Z

server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java

+                setIndexOptions(IndexOptions.DOCS_AND_FREQS_AND_POSITIONS_AND_OFFSETS);
+            }
+            if (parent.storeTermVectorOffsets()) {
+                setStoreTermVectorOffsets(true);


do we need to copy other storeTermVectorXXX settings?

jpountz · 2018-05-25T07:47:52Z

server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java

@@ -420,6 +520,11 @@ void setPrefixFieldType(PrefixFieldType prefixFieldType) {
            this.prefixFieldType = prefixFieldType;
        }

+        void indexPhrases(boolean indexPhrases) {


Prefix with set to be consistent with most other setters on this class?

jpountz · 2018-05-25T08:07:08Z

server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java

+            PositionIncrementAttribute posIncAtt = stream.getAttribute(PositionIncrementAttribute.class);
+            stream.reset();
+            while (stream.incrementToken()) {
+                if (posIncAtt.getPositionIncrement() > 1)


let's put brackets to be consistent with the rest of the code base?

jpountz · 2018-05-25T08:07:37Z

server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java

+            if (indexPhrases && slop == 0 && hasGaps(stream) == false) {
+                stream = new FixedShingleFilter(stream, 2);
+                field = field + FAST_PHRASE_SUFFIX;
+            }


I'd like this to work on all token streams and not only cached token streams, so maybe we should do something like:

if (indexPhrases && slop == 0) { CachingTokenFilter cachedStream; if (stream instanceof CachingTokenFilter) { cachedStream = (CachingTokenFilter) stream; } else { stream = cachedStream = new CachedTokenStrem(stream); } if (hasGaps(cachedStream) == false) { stream = new FixedShingleFilter(stream, 2); field = field + FAST_PHRASE_SUFFIX; } }

and change the signature of hasGaps to take a CachingTokenFilter?

jpountz · 2018-05-25T08:09:21Z

server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java

@@ -540,6 +691,10 @@ else if (this.prefixFieldMapper != null || mw.prefixFieldMapper != null) {
            throw new IllegalArgumentException("mapper [" + name() + "] has different index_prefix settings, current ["
                + this.prefixFieldMapper + "], merged [" + mw.prefixFieldMapper + "]");
        }
+        else if (this.fieldType().indexPhrases != mw.fieldType().indexPhrases) {
+            throw new IllegalArgumentException("mapper [" + name() + "] has different index_phrase settings, current ["


s/index_phrase/index_phrases/ ?

jpountz

It looks good to me overall, we just need one more check that positions are indexed on the field when index_phrases is set to true?

jpountz

LGTM. Thanks Alan!

romseygeek · 2018-05-29T19:53:14Z

@jimczi are you good with this too?

jimczi

LGTM, thanks @romseygeek !

Specifying `index_phrases: true` on a text field mapping will add a subsidiary [field]._index_phrase field, indexing two-term shingles from the parent field. The parent analysis chain is re-used, wrapped with a FixedShingleFilter. At query time, if a phrase match query is executed, the mapping will redirect it to run against the subsidiary field. This should trade faster phrase querying for a larger index and longer indexing times. Relates to #27049

Relates #30450

* master: Remove usage of explicit type in docs (elastic#29667) Share common readFrom/writeTo code in AcknowledgeResponse (elastic#30983) Adapt bwc versions after backporting elastic#31045 to 6.x Mute MatchPhrase*QueryBuilderTests [Docs] Fix typo in watcher conditions documentation (elastic#30989) Remove wrong link in index phrases doc Move pipeline APIs to ingest namespace (elastic#31027) [DOCS] Fixes accounting setting names (elastic#30863) [DOCS] Rewords _field_names documentation (elastic#31029) Index phrases (elastic#30450)

* master: Match phrase queries against non-indexed fields should throw an exception (#31060) In the internal highlighter APIs, use the field type as opposed to the mapper. (#31039) [DOCS] Removes duplicated authentication pages Enable customizing REST tests blacklist (#31074) Make sure KeywordFieldMapper#clone preserves split_queries_on_whitespace. (#31049) [DOCS] Moves machine learning overview to stack-docs [ML] Add secondary sort to ML events (#31063) [Rollup] Specialize validation exception for easier management (#30339) Adapt bwc versions after backporting #31045 to 6.3 Remove usage of explicit type in docs (#29667) Share common readFrom/writeTo code in AcknowledgeResponse (#30983) Adapt bwc versions after backporting #31045 to 6.x Mute MatchPhrase*QueryBuilderTests [Docs] Fix typo in watcher conditions documentation (#30989) Remove wrong link in index phrases doc Move pipeline APIs to ingest namespace (#31027) [DOCS] Fixes accounting setting names (#30863) [DOCS] Rewords _field_names documentation (#31029) Index phrases (#30450) Remove leftover debugging from PTCMDT Fix PTCMDT#testMinVersionSerialization Make Persistent Tasks implementations version and feature aware (#31045)

* 6.x: Add TRACE, CONNECT, and PATCH http methods (#31079) Change ObjectParser exception (#31030) Make sure KeywordFieldMapper#clone preserves split_queries_on_whitespace. (#31049) [DOCS] Removes duplicated authentication pages [Rollup] Specialize validation exception for easier management (#30339) Enable customizing REST tests blacklist (#31074) [DOCS] Moves machine learning overview to stack-docs [ML] Add secondary sort to ML events (#31063) QA: Check rollup job creation safety (#31036) Adapt bwc versions after backporting #31045 to 6.3 Remove usage of explicit type in docs (#29667) Move pipeline APIs to ingest namespace (#31027) Adapt bwc versions after backporting #31045 to 6.x Make Persistent Tasks implementations version and feature aware (#31045) Mute MatchPhrase*QueryBuilderTests [Docs] Fix typo in watcher conditions documentation (#30989) Remove wrong link in index phrases doc Reuse expiration date of trial licenses (#31033) Index phrases (#30450) [DOCS] Fixes accounting setting names (#30863) [DOCS] Rewords _field_names documentation (#31029)

The test cannot run when no type is registered. Relates #30450

romseygeek added 8 commits March 14, 2018 16:05

Merge branch 'master' into index-phrases

e931107

Merge branch 'master' into index-phrases

3719e30

Upgrade to lucene 7.4.0 snapshot

4cea300

Merge branch 'master' into index-phrases

efd612a

docs

aca2b7e

Merge branch 'master' into index-phrases

910f0c1

romseygeek self-assigned this May 8, 2018

romseygeek requested review from jimczi, colings86 and jpountz May 8, 2018 08:50

romseygeek added the :Search/Search Search-related issues that do not fall into other categories label May 8, 2018

romseygeek added >feature v6.4.0 labels May 8, 2018

romseygeek added 2 commits May 8, 2018 13:15

Placate checkstyle

e0fe29d

changelog

c156cbd

jimczi requested changes May 9, 2018

View reviewed changes

jpountz reviewed May 9, 2018

View reviewed changes

romseygeek added 3 commits May 18, 2018 14:03

Merge branch 'master' into index-phrases

007ee3d

MappedFieldType.matchQuery -> .analyzePhrase

66b1e48

Merge remote-tracking branch 'origin/master' into index-phrases

4d6cb66

jimczi reviewed May 22, 2018

View reviewed changes

jpountz reviewed May 22, 2018

View reviewed changes

romseygeek added 3 commits May 24, 2018 11:46

iter

9eb8a6d

Merge remote-tracking branch 'origin/master' into index-phrases

69cf210

iter

0deebb6

jpountz requested changes May 25, 2018

View reviewed changes

iter

6cfa4b1

jpountz requested changes May 29, 2018

View reviewed changes

romseygeek added 2 commits May 29, 2018 16:42

Merge branch 'master' into index-phrases

6208c31

Check for positions

1d7852e

jpountz approved these changes May 29, 2018

View reviewed changes

jimczi approved these changes May 29, 2018

View reviewed changes

romseygeek added 5 commits May 30, 2018 09:29

Merge branch 'master' into index-phrases

289186e

Merge branch 'master' into index-phrases

518e280

index_phrases isn't an updateable setting

8dd5cd5

Merge remote-tracking branch 'origin/master' into index-phrases

dab97ad

Merge remote-tracking branch 'origin/master' into index-phrases

b2e732a

romseygeek merged commit 0427339 into elastic:master Jun 4, 2018

romseygeek deleted the index-phrases branch June 4, 2018 07:50

jpountz mentioned this pull request Jun 4, 2018

Make it easier to optimize search with better analysis #27049

Closed

jimczi added a commit that referenced this pull request Jun 4, 2018

Remove wrong link in index phrases doc

fa6b726

Relates #30450

jimczi added a commit that referenced this pull request Jun 4, 2018

Remove wrong link in index phrases doc

6681dc0

Relates #30450

jimczi mentioned this pull request Jun 4, 2018

Match phrase queries against non-indexed fields should throw an exception #31060

Merged

jimczi added a commit that referenced this pull request Jun 5, 2018

Fix MatchPhrasePrefixQueryBuilderTests#testPhraseOnFieldWithNoTerms

990442f

The test cannot run when no type is registered. Relates #30450

romseygeek added the v7.0.0 label Jun 7, 2018

jpountz mentioned this pull request Jun 19, 2018

Re-think integration of the common-grams filter #31427

Open

Mpdreamz mentioned this pull request Sep 21, 2018

[meta] 6.4.0 release elastic/elasticsearch-net#3397

Closed

89 tasks

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

		@@ -360,6 +361,10 @@ public Query nullValueQuery() {

		public abstract Query existsQuery(QueryShardContext context);

		public MatchQuery matchQuery(QueryShardContext context, String analyzer, int slop) {


		Query q3 = new MatchPhraseQueryBuilder("field", "two words").slop(1).toQuery(queryShardContext);
		assertThat(q3, is(new PhraseQuery(1, "field", "two", "word")));

		@@ -360,6 +362,10 @@ public Query nullValueQuery() {

		public abstract Query existsQuery(QueryShardContext context);

		public Query analyzePhrase(String field, TokenStream stream, int slop) throws IOException {

Index phrases #30450

Index phrases #30450

Conversation

romseygeek commented May 8, 2018

elasticmachine commented May 8, 2018

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpountz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpountz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpountz left a comment

Choose a reason for hiding this comment

jpountz left a comment

Choose a reason for hiding this comment

romseygeek commented May 29, 2018

jimczi left a comment

Choose a reason for hiding this comment