New Annotated_text field type #30364

markharwood · 2018-05-03T15:03:07Z

Largely a copy of text field type but adds ability to include markdown-like syntax in the text.
The “AnnotatedText” class parses text+markup and converts into plain text and AnnotationTokens.
The annotation token values are injected unchanged alongside the regular text tokens to provide a
form of additional indexed overlay useful in positional searches and highlighting.

See example use in https://gist.github.com/markharwood/13dfd386d1c3ae76e116dd3d42413f58

Closes #29467

elasticmachine · 2018-05-03T15:03:09Z

Pinging @elastic/es-search-aggs

markharwood · 2018-05-08T08:07:32Z

run gradle build tests

jimczi

I like the approach, the format for the annotation is simple and can be extended later. I am not totally convinced by the removal of the annotations for highlighting though. There are valuable informations in the annotations that can be used for highlighting that are lost if we just remove them. Would it be possible to keep the annotations in the text and mark the entire annotations when it needs to be highlighted:

So <b>[Madonna](type=person&value=Madonna)<b>

?

jimczi · 2018-05-09T07:27:01Z

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

+            return fielddata;
+        }
+
+        public void setFielddata(boolean fielddata) {


Should it be allowed to use fielddata ? This field can be used in the significant text aggregation and the annotated text should be indexed in a doc values field so maybe we can disallow fielddata entirely ?

maybe we can disallow fielddata entirely ?

I wouldn't be opposed to that - we're looking to phase it out so this makes sense.

jimczi · 2018-05-09T07:37:19Z

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

+        private static String readToString(Reader reader) throws IOException{
+            //TODO FIXME I think this loses any end-of-text markers that are used
+            // internally in Lucene to turn arrays of strings into single strings.
+            // I think the positionIncrementGap logic looks for these special markers


The logic for the position increment gap is not based on markers, multi-valued fields are indexed in multiple fields that share the same field name, the index writer registers the last position for each field and applies the posInc gap to the last position seen for the field being processed. Is this comment related to highlighting ? We don't apply the posInc gap during highlighting because positions are not important (highlighting is based only on offsets).

I'm currently working on an issue with highlighting on fields that have arrays of text rather than a single value. There's all sorts of "magic" going on in unified highighter to assemble and disassemble strings to and from arrays using special marker characters.

The unified highlighter uses U+0000 to separate the values but it retrieves the original text first so this should not clash with the annotations? What is the issue with multi-values ? Can you explain the problem you're seeing ?

Generally invalid offsets. I need to give the highlighter both marked-up and non-marked-up versions of the text. Marked-up text is required for any re-analysis to get the annotations but plain text is used for BreakIterator and PassageFormatter. I think the confusion comes when content.length() is used to denote end of a passage and the highlighter is using the marked-up version so is too long. I had code to reset the last passage's endOffset properly but that doesn't work properly in a multi-value because there's multiple passages with the wrong offsets. Maybe the better approach is to smuggle the marked-up version to the choice of analyzer and keep everything else in the highlighter (PassageFormatters, BreakIterators etc) working with just the plain-text.

jimczi · 2018-05-09T07:46:21Z

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

+ * This code is largely a copy of TextFieldMapper which is less than ideal - 
+ * my attempts to subclass TextFieldMapper faild but we can revisit this.
+ **/
+public class AnnotatedTextFieldMapper extends FieldMapper {


To simplify the usage of this field type I think we should automatically (or add an option to activate it) index the annotated text in a doc_values field. This field would be available for aggregations and the terms bucket can be used to generate queries for highlighting or matching. I know it's possible to just index the annotations in a different field but I think it would be beneficial to have to link these two fields internally.

There's a number of questions around that:

What if users want the source JSON to include a structured "person" array?

How do we define the "copy_to" fields in the annotation markup?

Are there any complexities if the annotated text field appears under a nested object?

Could we save that feature for a phase 2 and keep this initial PR simpler for the moment?

We have additional fields for other use cases (parent/child, prefix text, ...) so it can work here too. The name of the field should be derived from the main field name, it can be hardcoded or the suffix can be added as an option in the field.
We can add this feature to phase 2 but I thought that it would be easier to have a clear link between the two fields in the first iteration. Let's leave this for a follow up.

jimczi · 2018-05-09T07:48:27Z

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

+ * This code is largely a copy of TextFieldMapper which is less than ideal - 
+ * my attempts to subclass TextFieldMapper faild but we can revisit this.
+ **/
+public class AnnotatedTextFieldMapper extends FieldMapper {


Is it possible to extend TextFieldMapper ? If not can we change TextFieldMapper to allow it ?

I did try subclassing initially and tweaking base class visibility where required but still was hitting issues so opted to copy code while I was proving the concept. I can cycle back around this as an internal optimization once we're happy with the functionality

I suspect the option of subclassing just got harder now that I've removed the FieldData support in AnnotatedTextFieldMapper and the TextFieldMapper base class still has support.

markharwood · 2018-05-09T10:45:15Z

Thanks for the review, @jimczi

Would it be possible to keep the annotations in the text and mark the entire annotations when it needs to be highlighted:

The simple case with "Madonna" looks feasible but gets tricky if the query is for the phrase "paid stormy" and the annotated text is

he paid [Stormy Daniels](type=person&value=Stephanie+Clifford) $130,000

The highlight begin/end tags would then overlap with the annotation begin/end markers.

jimczi · 2018-05-09T11:06:42Z

The highlight begin/end tags would then overlap with the annotation begin/end markers.

Since we highlight terms individually in the unified highlighter, the tags would be around stormy alone so I guess that it can be handled in the client side ? Though I don't know how the sentence break iterator would behave with the annotations format (it could split passage in the middle of an annotation) so removing the annotations makes sense for the moment. We could also add the ability in the custom passage formatter to transform an annotation into an html highlighting that extracts the type and the value to present them nicely ? I think you had some ideas regarding this.

markharwood · 2018-05-16T09:44:52Z

run gradle build tests

markharwood · 2018-05-29T15:39:38Z

run gradle build tests

markharwood · 2018-06-08T16:00:57Z

TL;DR: I think it's going to be too messy/complex for the annotated_text field to support phrase and prefix sub-fields like the text field currently does.

I'd ideally reuse classes like the PhraseFieldType from TextFieldMapper but the way Analyzers are constructed means it is not possible to strip the annotations markup from text before feeding to the shingle filter used in the phrase indexing.
The reason for this is that the annotated_text fields rely on the special markup-stripping analyzer being the outermost component in the analysis pipeline. The standard Lucene approach for wrapping analysis components prevents this special markup-stripping logic from being invoked.
The Phrase-indexing logic in PhraseWrappedAnalyzer.wrapComponents() is where the annotated_text field's existing TokenStreamComponents is replaced with a new one, taking the Tokenizer and components from the old TSC and adding another TokenFilter to the TokenStream. This is unfortunate because annotated_text uses a custom TokenStreamComponents impl with a custom setReader() which is thrown away as part of this substitution. The Phrase indexer's replacement TokenStreamComponents has the default setReader() behaviour which calls tokenizer.setReader() which is final so there is really no opportunity for the annotation field to intercept setReader when wrapped in this way.

markharwood · 2018-06-11T10:38:12Z

There is a workaround for the above unwrappabale analyzer problem but it involves the use of ThreadLocal to pass annotations from Reader to the annotations TokenFilter rather than relying on a custom TokenStreamComponents.
(Note I opened a Lucene issue - should TokenStreamComponents be made final if any subclasses are removed by the typical component-wrapping process?)

romseygeek · 2018-06-11T12:41:31Z

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

+            annotationsInjector.setAnnotations(at);
+            super.setReader(new StringReader(at.textMinusMarkup));  
+        }
+


Can you do this instead in AnnotatedHighlighterAnalyzer.wrapReader()? That would solve your issues with further wrapping, I think.

I use wrapReader in the main AnnotationAnalyzerWrapper which is now a wrappable Analyzer (courtesy of some ThreadLocal ickyness).
The line you've highlighted is from a special Analyzer designed specifically for use with highlighting annotated fields. The challenge there is a different one - the UnifiedHighlighter calls an Analyzer with fragments of the original plain text (eg sentences) and the AnnotatedHighlighterAnalyzer is responsible for mapping the plain-text string sentence presented by the highlighter to it's marked-up counterpart so that it can produce the Annotation tokens.

markharwood · 2018-06-11T17:38:42Z

server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/UnifiedHighlighter.java

+            if (fieldType.typeName().equals (AnnotatedTextFieldMapper.CONTENT_TYPE)){
+                AnnotatedHighlighterAnalyzer annotatedHighlighter = new AnnotatedHighlighterAnalyzer(fieldValues, analyzer);
+                fieldValues = annotatedHighlighter.getPlainTextValuesForHighlighter();
+                // use the wrapped analyzer that holds marked-up text


@romseygeek Not sure what you've got cooking with new highlighters but I made need to avoid reusing TextFieldMapper's "phrase" field type and introduce a fork "annotations_phrase" because that's a similar special-case scenario like the plain "annotations" type. Before we feed the original JSON content into the highlighter's passage fragmentation logic we must strip the text of annotation markup using getPlainTextValuesForHighlighter

jpountz

I like it in general, that looks useful. Some general comments:

I see it as an expert feature so I'm wondering whether we want to have it in a plugin.
I think we should include the type in the term instead of (or in addition to) the type attribute since the latter is not indexed, eg. person=First Last as a term rather than just First Last? It should help avoid matching these named entities when trying to match regular text?
Should we try to detect whether the analyzer splits on punctuation? I suspect behavior could be surprising if you used this field eg. with a keyword analyzer?
I'd vote to remove indexing of prefixes and phrases to this new field so that it focuses on the indexing of named entities rather than querying of text?
I'm still debating with myself whether highlighting should retain the annotations or not.
Can we find another way to propagate annotations to the token stream that doesn't need threadlocals? We should be able to let the annotation filter know about annotations from the reader somehow via TokenStreamComponents.setReader?
The key part of this PR is the analysis of annotated text so I'd like to see dedicated tests for this that do not initialize a node or mappings and do call things like BaseTokenStreamTestCase#checkAnalysisConsistency to make sure it behaves well.

markharwood · 2018-06-12T15:47:06Z

I see it as an expert feature so I'm wondering whether we want to have it in a plugin.

That choice might have impact on the following "core" pieces of software:

Unified highlighter currently has some special-case code to use a custom Analyzer when highlighting annotated fields.
Significant_text agg - I'd like to give users options to choose between selecting annotations and/or the underlying text. Currently results on Wikipedia content have a mix of annotation tokens e.g. Donald Trump and regular text tokens like donald and trump. This can be a little repetitive/annoying.
Kibana will need to render text appropriately

Kibana can always build in support for annotated text even when the Java plugin is not installed but the core Java parts needing special-casing for annotations mean we'd have to drag some fork/subclass of that core functionality into the plugin.

I think we should include the type in the term

I have struggled with this decision. The trade off is readability vs strict matching. I opted for no types for readability - when it comes to strict matching, the majority of named entities are generally mixed case or multi-word so don't over-match. Apple the company doesn't match the fruit apple but I suppose a lower-case company like elastic would match elastic band. One other workaround is to embed type info in the annotation (I did this to all of Wikipedia content using dbpedia's type info) which means you end up with annotations like this:

[elastic](wikipedia_item=elastic&dbpedia_type=Company) is highly elastic

For "strict" matching you could then search for the token elastic and Company at the same position.

I'd vote to remove indexing of prefixes and phrases to this new field so that it focuses on the indexing of named entities rather than querying of text?

I got it working with a ThreadLocal and I do see general search for entities near free text as useful but it's questionable whether phrase/prefix are required too. Highlighting for phrase/prefix I suspect is a bit of a mess currently with mixes of fields but I think @romseygeek is working on something new there.

I'm still debating with myself whether highlighting should retain the annotations or not.

I have an impl working for basic highlighting and it's certainly useful - just need to check how it behaves with various highlighter options.

Can we find another way to propagate annotations to the token stream

If we don't support phrase/prefix etc sub-fields and the related "analyzer wrapping" we can use a subclass of TokenStreamComponents (but I have that Lucene issue on this being a generally bad approach).

jpountz · 2018-06-13T13:32:59Z

Unified highlighter currently has some special-case code to use a custom Analyzer when highlighting annotated fields.

We should avoid making highlighters aware of specificities of some field types. Of course there will always be exceptions, but this feature doesn't sound like a common need enough to trade maintenability.

Significant_text agg - I'd like to give users options to choose between selecting annotations and/or the underlying text. Currently results on Wikipedia content have a mix of annotation tokens e.g. Donald Trump and regular text tokens like donald and trump. This can be a little repetitive/annoying.

Can this be addressed with an include regex that only matches terms that start with person: or an exclude regex that matches everything that contains a :?

The trade off is readability vs strict matching

Strict matching feels important to me: if some terms are both common terms and annotations, how do you filter on the annotation specifically? Your idea to require tokens at the same position would work but I'm a bit unhappy about corner-cases such as the fact that foo[type=bar] and bar[type=foo] would still be hard to distinguish? Also, the fact that some entities may match full-text queries is problematic for relevance since the term frequency will be artificially bumped up.

If we don't support phrase/prefix etc sub-fields and the related "analyzer wrapping" we can use a subclass of TokenStreamComponents

+1 to this trade-off

markharwood · 2018-06-13T21:27:27Z

his feature doesn't sound like a common need enough to trade maintenability.

OK I'll look into bundling a highlighter subclass in a plugin

Can this be addressed with an include regex

I'm not sure that's enough. The annotations represent the bits of the text where we are certain about what is in there so would generally prefer to take the annotation, not the underlying text string but for the rest of the text we still want significant words. So the policy options I thought might be required for discovery were:

text only
text and annotations
annotations only
annotations and only text outside of the annotations

Strict matching feels important to me

I guess users who want that could also always choose their own choice of uniquifying prefix and put that in their values? That would provide a workaround for those that want strictness but there would be no workaround for users who prefer readability if we always use token type as a prefix for values.

markharwood · 2018-06-26T14:37:14Z

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

@@ -56,6 +57,7 @@
        }
        ALLOWED_QUERY_MAPPER_TYPES.add("scaled_float");
        ALLOWED_QUERY_MAPPER_TYPES.add(TextFieldMapper.CONTENT_TYPE);
+        ALLOWED_QUERY_MAPPER_TYPES.add(AnnotatedTextFieldMapper.CONTENT_TYPE);


@jpountz this is currently a bit of a blocker for moving annotated_text to a plugin.

I agree this one is annoying. I started a discussion at #31655.

markharwood · 2018-06-26T14:37:58Z

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

@@ -90,7 +91,7 @@
    public static final boolean DEFAULT_FAIL_ON_UNSUPPORTED_FIELDS = true;

    private static final Set<Class<? extends MappedFieldType>> SUPPORTED_FIELD_TYPES = new HashSet<>(
-            Arrays.asList(TextFieldType.class, KeywordFieldType.class));
+            Arrays.asList(TextFieldType.class, KeywordFieldType.class, AnnotatedTextFieldType.class));


Another example of core hard-coding text types

I agree supporting more-like-this would be nice ultimately, but let's just skip it for now?

I was trying to workaround the lack of MLT using the TermVectorsService but encountered a similar hard-coded road-block:

elasticsearch/server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java

Line 165 in 00b0e10

if (fieldType instanceof KeywordFieldMapper.KeywordFieldType == false

markharwood · 2018-09-12T12:56:11Z

I've removed type support now and build is back to green.

Good to go @jpountz @jimczi ?

jpountz

LGTM. I like the guidance that you provided in the docs around avoiding clashes with regular tokens.

Largely a copy of `text` field type but adds ability to include markdown-like syntax in the text. The “AnnotatedText” class parses text+markup and converts into plain text and AnnotationTokens. The annotation token values are injected unchanged alongside the regular text tokens to provide a form of additional indexed overlay useful in positional searches and highlighting. Annotated_text fields do not support fielddata as we want to phase this out. Also includes a new "annotated" highlighter type that retains annotations and merges in search hits as additional annotation markup. Closes elastic#29467

…ng (originally required for index_phrase/prefix which we no longer support)

…ed fields, remove `typed` annotations from doc examples

…sticsearchParseException if a document uses these

romseygeek

LGTM. A couple of formatting nits, and one question that can be done in a follow-up if you prefer.

romseygeek · 2018-09-17T08:34:02Z

...ext/src/main/java/org/elasticsearch/index/mapper/annotatedtext/AnnotatedTextFieldMapper.java

+        public AnnotatedTextFieldType clone() {
+            return new AnnotatedTextFieldType(this);
+        }
+


Extra newline?

romseygeek · 2018-09-17T08:34:20Z

...ext/src/main/java/org/elasticsearch/index/mapper/annotatedtext/AnnotatedTextFieldMapper.java

+            return mpqb.build();
+        }        
+    }
+


romseygeek · 2018-09-17T08:35:31Z

...rc/main/java/org/elasticsearch/search/fetch/subphase/highlight/AnnotatedTextHighlighter.java

+import static org.apache.lucene.search.uhighlight.CustomUnifiedHighlighter.MULTIVAL_SEP_CHAR;
+
+public class AnnotatedTextHighlighter extends UnifiedHighlighter {
+


extra newline

romseygeek · 2018-09-17T08:42:08Z

...rc/main/java/org/elasticsearch/search/fetch/subphase/highlight/AnnotatedTextHighlighter.java

+        }
+        return null;
+    }
+


A lot of this is just a copy of the parent method; is it worth trying to break it up a bit more and use inheritance to avoid copying so much code? The only differences are in the Analyzer and PassageFormatter types, I think?

…otatedTextHighlighter duplicate code by opening up UnifiedHighlighter for necessary overrides of Analyzer, field loading and PassageFormatter

romseygeek · 2018-09-17T13:37:06Z

server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/UnifiedHighlighter.java

+    protected Analyzer wrap(Analyzer analyzer) {
+        return analyzer;
+    }
+


Can we merge wrap() into getAnalyzer()? It feels wrong to have both methods

getAnalyzer is static so can't be overriden?

Right, I mean change it so that it isn't static. Or have a different, non-static getAnalyzer() method that calls out to the static version in the base class.

Static method prob needs to stay as already called from PlainHighlighter and can't have instance method with same name so least disruptive change is a new getThisAnalyzer() (not crazy about that name) that calls existing static getAnalyzer.

How about moving the static method to HighlightUtils? That seems to me to be a better place for it anyway, if it's being called from multiple highlighters.

romseygeek · 2018-09-17T13:37:46Z

Thanks, that looks much better. I left one comment, but LGTM otherwise.

romseygeek · 2018-09-18T09:01:16Z

+1

markharwood added >feature :Search Relevance/Highlighting How a query matched a document :Search Foundations/Mapping Index mappings, including merging and defining field types labels May 3, 2018

markharwood requested review from jpountz and jimczi May 3, 2018 15:03

jbudz mentioned this pull request May 4, 2018

Render highlighting in new annotated_text type elastic/kibana#18690

Closed

colings86 removed the :Search Relevance/Highlighting How a query matched a document label May 8, 2018

jimczi reviewed May 9, 2018

View reviewed changes

markharwood force-pushed the fix/29467 branch from f46a782 to d14260f Compare May 14, 2018 12:43

markharwood force-pushed the fix/29467 branch from 145442b to e545f1b Compare May 16, 2018 18:29

markharwood force-pushed the fix/29467 branch from 4d7ba36 to 2e08a48 Compare June 8, 2018 09:28

markharwood force-pushed the fix/29467 branch from 4bce8d1 to c23b38d Compare June 11, 2018 11:08

romseygeek reviewed Jun 11, 2018

View reviewed changes

markharwood commented Jun 11, 2018

View reviewed changes

jpountz reviewed Jun 12, 2018

View reviewed changes

markharwood commented Jun 26, 2018

View reviewed changes

markharwood force-pushed the fix/29467 branch 2 times, most recently from 04f4287 to 5ac2935 Compare July 5, 2018 10:43

markharwood force-pushed the fix/29467 branch 2 times, most recently from 1c75bf5 to e8acd14 Compare September 1, 2018 10:03

markharwood force-pushed the fix/29467 branch from e8acd14 to f93ae0f Compare September 12, 2018 09:13

jpountz approved these changes Sep 12, 2018

View reviewed changes

markharwood added 13 commits September 17, 2018 09:37

Added analyze api example

f469116

Addressed review comments. Removed dependency on ThreadLocal in parsi…

9afa0f2

…ng (originally required for index_phrase/prefix which we no longer support)

Bring in support for MultiPhraseQuery (copied from TextFieldType)

f9fbf36

Fix for checkstyle violation

75ced30

Remove eagerGlobalOrdinals setting, change docs example type “_doc”

75cbcc9

Addressing review comments - add UncheckedIOException, reject unindex…

12fe50d

…ed fields, remove `typed` annotations from doc examples

Unused import

e1256da

Added “testNotIndexedField” test and removed irrelevant test.

a8cb0e9

Remove support for type=value syntax in annotations. We now throw Ela…

43720ee

…sticsearchParseException if a document uses these

Remove types from test

b3b09ef

Remove types from Tests

ce7d879

Fix for changes to TotalHits

6dd2085

markharwood force-pushed the fix/29467 branch from 5bfc69f to 6dd2085 Compare September 17, 2018 08:46

romseygeek approved these changes Sep 17, 2018

View reviewed changes

Address @romseygeek review comments. Removed newlines and removed Ann…

c3e8b9f

…otatedTextHighlighter duplicate code by opening up UnifiedHighlighter for necessary overrides of Analyzer, field loading and PassageFormatter

romseygeek reviewed Sep 17, 2018

View reviewed changes

Moved static getAnalyzer to HighlightUtils

0a5422c

markharwood merged commit 2fa09f0 into elastic:master Sep 18, 2018

markharwood added v7.0.0 v6.5.0 labels Sep 18, 2018

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

		import static org.apache.lucene.search.uhighlight.CustomUnifiedHighlighter.MULTIVAL_SEP_CHAR;

		public class AnnotatedTextHighlighter extends UnifiedHighlighter {

New Annotated_text field type #30364

New Annotated_text field type #30364

Uh oh!

Conversation

markharwood commented May 3, 2018

Uh oh!

elasticmachine commented May 3, 2018

Uh oh!

markharwood commented May 8, 2018

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markharwood commented May 9, 2018

Uh oh!

jimczi commented May 9, 2018

Uh oh!

markharwood commented May 16, 2018

Uh oh!

markharwood commented May 29, 2018

Uh oh!

markharwood commented Jun 8, 2018

Uh oh!

markharwood commented Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markharwood Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

markharwood commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpountz commented Jun 13, 2018

Uh oh!

markharwood commented Jun 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markharwood commented Jun 11, 2018 •

edited

Loading

markharwood Jun 11, 2018 •

edited

Loading

markharwood commented Jun 12, 2018 •

edited

Loading