Make projected parquet collection schema forward compatible with the given file schema #1921

mickjermsurawong-stripe · 2019-09-27T22:10:28Z

Problem: projection schema is created by ThriftSchemaConvertVisitor which suffix fields with _tuple

scalding/scalding-parquet-scrooge/src/main/java/com/twitter/scalding/parquet/scrooge/ScroogeReadSupport.java

Lines 98 to 101 in 59d9327

    
           if (thriftClass == null) { 
        
             thriftClass = getThriftClassFromMultipleFiles(context.getKeyValueMetadata(), configuration); 
        
           } 
        
           requestedProjection = getProjectedSchema(projectionFilter);

When reading parquet file with standard format 3-level or even legacy ones 2-level array, projection fails because it has name _tuple which is incompatible. For example, given file schemas to represent List<String> country_codes

message compliant_schema {
  required group country_codes (LIST) {
    repeated binary array (UTF8);
  }
  required int32 to_be_removed;
}

will fail with the generated projection

message thrift_generated {
  optional group country_codes (LIST) {
    repeated binary country_codes_tuple (UTF8);
  }
}

This PR introduces ParquetListFormatForwardCompatibility to convert thrift-generated schema to compliant ones. The difficulties here are:
- we can not simply just traverse the two graphs and swap node one-for-one. The node has to be from the projection, because it has subset of fields and different optional/required info.
- there are at least 4 variations of compliant parquet types, and we want to convert from non-compliant to compliant, and among the compliant.
The strategy here is to implement different rules for each format. The rule tell us how to decompose the list schema, and to reconstruct it again.
- For example, we match projected schema with Rule 1, and match file schema with Rule 2.
- Rule 1 will decompose the projected schema, and Rule 2 will take that information to construct in light of the file schema.

CLAassistant · 2019-09-27T22:10:33Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ mickjermsurawong-stripe
❌ joshrosen-stripe
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

...rc/main/java/com/twitter/scalding/parquet/scrooge/ParquetListFormatForwardCompatibility.java

...et-scrooge/src/test/scala/com/twitter/scalding/parquet/scrooge/ScroogeReadSupportTests.scala

...rc/main/java/com/twitter/scalding/parquet/scrooge/ParquetListFormatForwardCompatibility.java

mickjermsurawong-stripe · 2019-09-27T22:19:25Z

cc @moulimukherjee

mickjermsurawong-stripe · 2019-09-27T23:23:24Z

Hi @johnynek,
We ran into a problem where Scalding job fails reading parequet files produced by another Spark job. The problem we discovered is due to projected schema which is generated from Thrift class has different list format than our Spark job.

Just would like to ask for a quick look whether this is the right strategy, and if this is something that we would like to push it to the public one as well.

johnynek

Thanks for the PR.

Have we run this internally to do some smoke tests that it can read old data that was previously written?

I added some general comments. I really don't know the latest with parquet. Maybe @isnotinvain or @julienledem can comment?

cc @ttim

...rc/main/java/com/twitter/scalding/parquet/scrooge/ParquetListFormatForwardCompatibility.java

johnynek · 2019-09-28T00:05:45Z

Also, you need to bump the .travis.yml jdk setting to openjdk8 since oraclejdk8 I think no longer works on travis.

mickjermsurawong-stripe · 2019-09-28T00:13:25Z

Thanks @johnynek. Ah I haven't done any tests internally on actual data. Just purely unit tests now. Will follow up with that.

I can port all these to Scala, and will make the resolver stateless; yup the object instantiation isn't neccessary.

mickjermsurawong-stripe · 2019-09-30T18:39:04Z

hi @isnotinvain or @julienledem, I've addressed the first round of general feedback, and added support for map schema compat as well. Would appreciate your review here.

joshrosen-stripe

I left some initial review comments, primarily focused on understanding (and potentially simplifying) two of the larger and more complex pattern matches at the heart of the converter.

I also left a couple of minor style nits, but those can be addressed later (in a final polish pass) once we're confident in the rest of the design.

I haven't finished looking at this yet, though: I still need to go through ParquetListFormatRule and also need to take a closer look at the test cases.

...scala/com/twitter/scalding/parquet/scrooge/ParquetCollectionFormatForwardCompatibility.scala

joshrosen-stripe · 2019-09-30T18:25:46Z

...scala/com/twitter/scalding/parquet/scrooge/ParquetCollectionFormatForwardCompatibility.scala

+                                    repeatedListType: Option[Type] = None,
+                                    repeatedMapType: Option[Type] = None)
+
+  private def unwrapGroup(typ: Type, wrappers: Seq[GroupType] = Seq()): GroupUnwrapped = {


Perhaps a naive question, but what does "wrapped" or "wrapper" refer to in this case? Could you add a comment explaining this? That might make the following code easier to review.

I now simplified to just extract for only the repeated type, and let the recursive call in main method formatForwardCompatibleType handle multiple layers instead.
This now becomes a sealed trait for the group type itself, and a helper method to get its repeated type

private[scrooge] sealed trait CollectionGroup { def groupType: GroupType def repeatedType: Type }

joshrosen-stripe · 2019-09-30T18:30:29Z

...quet-scrooge/src/main/scala/com/twitter/scalding/parquet/scrooge/ParquetListFormatRule.scala

+    }
+  }
+
+  def isGroupList(projection: Type): Boolean = {


To confirm my understanding:

This is matching on things like

required group my_list (LIST) { repeated group list { optional binary element (UTF8); } }

where groupProjection here refers to the top-most type, and the groupProjection.getFields.get(0).isRepetition(Type.Repetition.REPEATED) check would apply to the second-level element (repeated group list)?

yup that's correct on groupProjection, and the repeated is the second level.

The part after repeated could be different depending on formats eg.

required group my_list (LIST) { repeated repeated int32 [element|array]; }

joshrosen-stripe · 2019-09-30T18:36:02Z

...scala/com/twitter/scalding/parquet/scrooge/ParquetCollectionFormatForwardCompatibility.scala

+        repeatedListType=None,
+        repeatedMapType=None
+      )
+    } else if (typ.asGroupType.getFieldCount != 1) {


In this else if branch and the else branch I noticed we have typ.asGroupType calls everywhere. To avoid that and to make it clear that !typ.isPrimitive implies that typ is a group type, could we instead do something like

if (typ.isPrimitive) { // ... } else { val groupTyp = typ.asGroupTyp if (groupTyp.getFieldCount != 1) { //... } else { //... } }

We might be able to further improve clarity by changing the sign of the second condition, doing

if (typ.isPrimitive) { // ... } else { val groupTyp = typ.asGroupTyp if (groupTyp.getFieldCount == 1) { // <--- inverted condition //... } else { //... } }

which then simplifies or clarifies the

// Note the field count is strictly 1 here

comment present in the current code.

We could then add a comment in the else branch saying something like

// The group contains more than one field, so ... <whatever that implies>

This is maybe nitpicky, but I'm trying to walk through each case here to make sure that I understand what all of the branches correspond to, so I'm considering whether a different ordering or presentation of the cases can make it easier to understand and verify.

Simplified this to two methods to extract group list and maplist instead.

...scala/com/twitter/scalding/parquet/scrooge/ParquetCollectionFormatForwardCompatibility.scala

+   * this is because the subset fields of source node and its optional/required must
+   * be maintained in the formatted result.
+   */
+  private def formatForwardCompatibleType(sourceType: Type, targetType: Type): Type = {


...scala/com/twitter/scalding/parquet/scrooge/ParquetCollectionFormatForwardCompatibility.scala

...quet-scrooge/src/main/scala/com/twitter/scalding/parquet/scrooge/ParquetListFormatRule.scala

joshrosen-stripe · 2019-09-30T19:00:10Z

...quet-scrooge/src/main/scala/com/twitter/scalding/parquet/scrooge/ParquetListFormatRule.scala

+
+  private[scrooge] def isElementRequired(repeatedType: Type): Boolean
+
+  private[scrooge] def check(typ: Type): Boolean


From the name, it's a bit non-obvious what this method does. It this actually something like appliesToType(typ: Type) which returns true if the type is a list of this format and false otherwise?

+1 on naming this something more descriptive

Updated as suggested

thomaschow · 2019-09-30T21:46:30Z

...quet-scrooge/src/main/scala/com/twitter/scalding/parquet/scrooge/ParquetListFormatRule.scala

+  def name: String
+}
+
+private[scrooge] object Source extends SourceOrTarget {


what's the advantage of distinguishing between Source and Target? is it so we can define supported directionalities in terms of parquet spec conversion? I'm wondering if we're being too specific here and wind up seeing a direction that we didn't engineer for, and we fail

I guess since scalding is going to be stuck on the older parquet spec, this would not happen.

~~ack. Will see how I can do end-to-end testing in this repo.~~
Ah the only thing i don't support here is the target type of _tuple format which then would be compatible with the thrift-generated projection source

thomaschow · 2019-09-30T21:59:50Z

...quet-scrooge/src/main/scala/com/twitter/scalding/parquet/scrooge/ParquetListFormatRule.scala

+  override def check(repeatedType: Type): Boolean = repeatedType.getName.endsWith("_tuple")
+
+  override def elementName(repeatedType: Type): String = {
+    repeatedType.getName.substring(0, repeatedType.getName.length - 6)


nit: would like docs or variable/method renaming to explain this substring indexing a little more clearly

I now made _tuple as a variable where length is calculated from it more explicitly.

...quet-scrooge/src/main/scala/com/twitter/scalding/parquet/scrooge/ParquetListFormatRule.scala

thomaschow · 2019-09-30T22:25:29Z

.../com/twitter/scalding/parquet/scrooge/ParquetCollectionFormatForwardCompatibilityTests.scala

+import org.apache.parquet.thrift.struct.ThriftType.{ListType, MapType, StructType}
+import org.scalatest.{Matchers, WordSpec}
+
+class ParquetCollectionFormatForwardCompatibilityTests extends WordSpec with Matchers {


ooc how hard would it be to add some actual thrift fixtures based tests in here?

ack. Will see how I can do end-to-end testing in this repo.

mickjermsurawong-stripe · 2019-10-01T20:31:05Z

@joshrosen-stripe, @tchow-stripe
Thanks for the really helpful review. I've simplified many parts of the code, and addressed the PR feedback.
Pending issues are figuring out to do end-to-end testing here.
@isnotinvain this is ready for another round of review.

xton

This review is incomplete, I'll finish later this evening. I've got some rename suggestions and some possible bugs so far.

xton · 2019-10-02T23:21:36Z

...scala/com/twitter/scalding/parquet/scrooge/ParquetCollectionFormatForwardCompatibility.scala

+    sourceGroup.groupType.withNewFields(formattedRepeated)
+  }
+
+  private def findCollectionGroup(typ: Type): Option[CollectionGroup] = {


Naming nit:

Suggested change

private def findCollectionGroup(typ: Type): Option[CollectionGroup] = {

private def extractCollectionGroup(typ: Type): Option[CollectionGroup] = {

"find" implies that the return values will be a subset of the input, while "extract" (which the underlying functions use) suggests that the whole input will be considered.

xton · 2019-10-02T23:25:20Z

...scala/com/twitter/scalding/parquet/scrooge/ParquetCollectionFormatForwardCompatibility.scala

+        formatForwardCompatibleCollectionGroup[ListGroup](sourceGroup, targetGroup)
+      case (Some(sourceGroup: MapGroup), Some(targetGroup: MapGroup)) =>
+        formatForwardCompatibleCollectionGroup[MapGroup](sourceGroup, targetGroup)
+      case _ if sourceType.isPrimitive || targetType.isPrimitive => // Base case


I would explicitly check and throw if one is a primitive but not the other. As written this will silently pass through certain schema mismatches.

xton · 2019-10-02T23:32:13Z

...scala/com/twitter/scalding/parquet/scrooge/ParquetCollectionFormatForwardCompatibility.scala

+   * this is because the subset fields of source node and its optional/required must
+   * be maintained in the formatted result.
+   */
+  private def formatForwardCompatibleType(sourceType: Type, targetType: Type): Type = {


Suggested change

private def formatForwardCompatibleType(sourceType: Type, targetType: Type): Type = {

private def formatForwardCompatibleType(fileType: Type, projectionType: Type): Type = {

"source" and "target" are very generic names and this function is really only used for one thing. It might make sense to rename them accordingly to make reasoning about the correctness here easier.

From conversation thread below:

By that i mean we want to convert projection source schema to be the in the format of the target file schema.

xton · 2019-10-02T23:35:51Z

...scala/com/twitter/scalding/parquet/scrooge/ParquetCollectionFormatForwardCompatibility.scala

+        val targetGroup = targetType.asGroupType
+        val resultFields = sourceGroup.getFields.asScala.map { sourceField =>
+          if (!targetGroup.containsField(sourceField.getName)) {
+            if (!sourceField.isRepetition(Repetition.OPTIONAL)) {


This check is incorrect, I think. "source" == the file's schema, yes (see above renaming suggestion)? If so, then the optionality of a field which is not included in the projection is not important. We should be iterating over the fields in the projected (target) schema and checking the optionality of the projected fields if they're missing from the file.

Ah no, source here is actually the projection, and target is the file's scheme.
By that i mean we want to convert projection source schema to be the in the format of the target file schema.
Given this confusion, i think makes sense that I should rename this.

on this point, I think naming convention along the lines of: fileSchema, readSchema, projectedFileSchema, projectedReadSchema all help clarify which is which

…schema

xton

One potential simplification and some rename/docs nits. Looking good!

...scala/com/twitter/scalding/parquet/scrooge/ParquetCollectionFormatForwardCompatibility.scala

...rquet-scrooge/src/main/scala/com/twitter/scalding/parquet/scrooge/ParquetListFormatter.scala

xton · 2019-10-03T16:46:50Z

...scala/com/twitter/scalding/parquet/scrooge/ParquetCollectionFormatForwardCompatibility.scala

+      fileGroup.repeatedType,
+      projectFileType(_, _))
+    // Respect optional/required from the projected read group.
+    projectedReadGroup.groupType.withNewFields(projectedFileRepeatedType)


Could this end up letting through cases where the file's field is optional and the projection's field is required? If so we should explicitly detect and throw for that (probably in projectFileType).

Added the assertion at the "struct projection" which will have access of field-level optional/required

...scala/com/twitter/scalding/parquet/scrooge/ParquetCollectionFormatForwardCompatibility.scala

xton · 2019-10-03T16:58:48Z

...rquet-scrooge/src/main/scala/com/twitter/scalding/parquet/scrooge/ParquetListFormatter.scala

+  override val rules: Seq[ParquetListFormatRule] = Seq(
+    PrimitiveElementRule, PrimitiveArrayRule,
+    GroupElementRule, GroupArrayRule,
+    TupleRule, StandardRule


Soooo many cases... That said, I like this approach. Makes it painfully clear the variants that we're supporting.

...arquet-scrooge/src/main/scala/com/twitter/scalding/parquet/scrooge/ParquetMapFormatter.scala

xton · 2019-10-03T17:12:43Z

...scala/com/twitter/scalding/parquet/scrooge/ParquetCollectionFormatForwardCompatibility.scala

+  private def projectFileGroup[T <: CollectionGroup](projectedReadGroup: T,
+                                                     fileGroup: T,
+                                                     fieldContext: FieldContext)(implicit t: ClassTag[T]): GroupType = {
+
+    val (formatter, updatedFieldContext) = t.runtimeClass.asInstanceOf[Class[T]] match {
+      case c if c == classOf[MapGroup] =>
+        (ParquetMapFormatter, fieldContext.copy(nestedListLevel = fieldContext.nestedListLevel + 1))
+      case c if c == classOf[ListGroup] =>
+        (ParquetListFormatter, fieldContext.copy(nestedListLevel = fieldContext.nestedListLevel + 1))
+    }


Let's refactor this to avoid reflection and implicit magic. E.g.:

Suggested change

private def projectFileGroup[T <: CollectionGroup](projectedReadGroup: T,

fileGroup: T,

fieldContext: FieldContext)(implicit t: ClassTag[T]): GroupType = {

val (formatter, updatedFieldContext) = t.runtimeClass.asInstanceOf[Class[T]] match {

case c if c == classOf[MapGroup] =>

(ParquetMapFormatter, fieldContext.copy(nestedListLevel = fieldContext.nestedListLevel + 1))

case c if c == classOf[ListGroup] =>

(ParquetListFormatter, fieldContext.copy(nestedListLevel = fieldContext.nestedListLevel + 1))

}

private def projectFileGroup(projectedReadGroup: T, fileGroup: T, fieldContext: FieldContext, formatter: ParquetCollectionFormatter): GroupType = {

Updated to

def projectFileGroup(fileGroup: CollectionGroup, projectedReadGroup: CollectionGroup, fieldContext: FieldContext, formatter: ParquetCollectionFormatter)

…file schema

mickjermsurawong-stripe · 2019-10-04T18:50:55Z

Thanks @xton, addressed the feedback.
@isnotinvain could you help to take a look on this please?

joshrosen-stripe · 2019-10-04T21:35:00Z

...c/main/scala/com/twitter/scalding/parquet/scrooge/ParquetCollectionFormatCompatibility.scala

+  private def projectFileType(fileType: Type, projectedReadType: Type, fieldContext: FieldContext): Type = {
+    (extractCollectionGroup(projectedReadType), extractCollectionGroup(fileType)) match {
+      case _ if projectedReadType.isPrimitive && fileType.isPrimitive =>
+        projectedReadType


Do we need to check that both types are the same primitive type? Or is that handled elsewhere? I imagine that certain types of primitive-type-changing projections might be supported (e.g. widening an int to a long) but others might not be (e.g. converting a string to an int).

That type compatibility is handled in a separate place here, so by the time this is called we should already have type compatibility.

scalding/scalding-parquet-scrooge/src/main/java/com/twitter/scalding/parquet/scrooge/ScroogeReadSupport.java

Lines 133 to 136 in 73ce124

public static MessageType getSchemaForRead(MessageType fileMessageType, MessageType projectedMessageType) {

assertGroupsAreCompatible(fileMessageType, projectedMessageType);

return projectedMessageType;

}

joshrosen-stripe · 2019-10-04T22:53:40Z

...c/main/scala/com/twitter/scalding/parquet/scrooge/ParquetCollectionFormatCompatibility.scala

+  def formatCompatibleRepeatedType(fileRepeatedType: Type,
+                                   readRepeatedType: Type,
+                                   fieldContext: FieldContext,
+                                   recursiveSolver: (Type, Type, FieldContext) => Type): Type


In practice, I think this is always invoked with the projectFileType method defined above?

Yup it is. My thought is to make this recursive call explicit from the caller side so it's clear from the reading within this format compatbility layer instead of jumping into the respective formatter which then cyclically refers back to the caller's method.

* Add more comments; add explicit else conditions to some if cases; lift base case out of pattern match * Formatting changes * More minor formatting.

mickjermsurawong-stripe · 2019-10-08T15:54:24Z

hi @isnotinvain, would like to follow-up on this if you have more feedback here please.

mickjermsurawong-stripe · 2019-10-16T19:01:11Z

Hi @isnotinvain, another bump on this please.

Updates from our side: we actually have implemented this in our internally (via subclassing of ParquetScroogeScheme), and it does cover production use cases of read Spark job schema.

list having 3 level, and 2-level in legacy mode, compared to _tuple suffixed via generated schema from thrift class.
map having repeated group named key_value instead of map
This will also cover more conversion types as outlined in ParquetMapFormatter and ParquetListFormatter

Without this patch, scalding job reading Spark job output can either fail hard or falsely read empty collection.

forward list compat

e373ea2

mickjermsurawong-stripe changed the title ~~Make projected parquet schema forward compatible with the given file schema~~ [WIP] Make projected parquet schema forward compatible with the given file schema Sep 27, 2019

mickjermsurawong-stripe commented Sep 27, 2019

View reviewed changes

...rc/main/java/com/twitter/scalding/parquet/scrooge/ParquetListFormatForwardCompatibility.java Outdated Show resolved Hide resolved

mickjermsurawong-stripe commented Sep 27, 2019

View reviewed changes

...et-scrooge/src/test/scala/com/twitter/scalding/parquet/scrooge/ScroogeReadSupportTests.scala Outdated Show resolved Hide resolved

mickjermsurawong-stripe commented Sep 27, 2019

View reviewed changes

...et-scrooge/src/test/scala/com/twitter/scalding/parquet/scrooge/ScroogeReadSupportTests.scala Outdated Show resolved Hide resolved

mickjermsurawong-stripe commented Sep 27, 2019

View reviewed changes

...rc/main/java/com/twitter/scalding/parquet/scrooge/ParquetListFormatForwardCompatibility.java Outdated Show resolved Hide resolved

johnynek reviewed Sep 28, 2019

View reviewed changes

mickjermsurawong-stripe added 10 commits September 27, 2019 17:14

bump travis to openjdk8

721a490

fail when projecting on list of struct

0adbfae

recurse only on elements

84c119a

check for non-optional extra fields for projection

6bf7652

move to scala

018f267

migrate test to different class

ae89c06

handle map legacy format

ca67991

format import

b7363b8

improve docs

2996317

fix style warning

e02d0a8

mickjermsurawong-stripe changed the title ~~[WIP] Make projected parquet schema forward compatible with the given file schema~~ Make projected parquet collection schema forward compatible with the given file schema Sep 30, 2019

joshrosen-stripe reviewed Sep 30, 2019

View reviewed changes

thomaschow reviewed Sep 30, 2019

View reviewed changes

...quet-scrooge/src/main/scala/com/twitter/scalding/parquet/scrooge/ParquetListFormatRule.scala Outdated Show resolved Hide resolved

thomaschow reviewed Sep 30, 2019

View reviewed changes

mickjermsurawong-stripe added 4 commits October 1, 2019 00:07

fix warning descenents of sealed traits

436def8

remove duplicate test

7ba2305

improve test names and remove one duplicate

d2f2fb5

add docs

9c96d23

xton reviewed Oct 2, 2019

View reviewed changes

mickjermsurawong-stripe added 6 commits October 2, 2019 17:15

Check on schema type mismatch

09bd4dd

explicit rename from source/target to projected read schema and file …

83b6898

…schema

support creating _tuple format and generalize compat in all directions

d0ed5d6

support legacy spark list of nullable elements

5de8b64

improve docs

db937c3

add tests for all supported list compat conversions

0c38af8

xton suggested changes Oct 3, 2019

View reviewed changes

mickjermsurawong-stripe added 8 commits October 4, 2019 05:51

improve docs

3700a85

remove classtag inference

3b7255b

rename schema name and use consistent method

3a9be2e

file rename to drop "forward" compat

5b5cf2b

test rename and make variables consistent

e29c7e9

make names file/read oriented

cd7e69c

improve test make sure formatted type is still compatible with given …

b05f13c

…file schema

check for field optional/required

7bb0382

joshrosen-stripe reviewed Oct 4, 2019

View reviewed changes

joshrosen-stripe mentioned this pull request Oct 4, 2019

Review suggestions for https://github.com/twitter/scalding/pull/1921 mickjermsurawong-stripe/scalding#1

Merged

joshrosen-stripe reviewed Oct 4, 2019

View reviewed changes

joshrosen-stripe and others added 3 commits October 5, 2019 15:03

Review suggestions for twitter#1921 (#1)

6365db8

* Add more comments; add explicit else conditions to some if cases; lift base case out of pattern match * Formatting changes * More minor formatting.

improve code coverage and remove dead code after restructuring

b6c8a92

auto-format from running sbt test

579905c

	if (thriftClass == null) {
	thriftClass = getThriftClassFromMultipleFiles(context.getKeyValueMetadata(), configuration);
	}
	requestedProjection = getProjectedSchema(projectionFilter);


		private[scrooge] def isElementRequired(repeatedType: Type): Boolean

		private[scrooge] def check(typ: Type): Boolean

	private def findCollectionGroup(typ: Type): Option[CollectionGroup] = {
	private def extractCollectionGroup(typ: Type): Option[CollectionGroup] = {

	private def formatForwardCompatibleType(sourceType: Type, targetType: Type): Type = {
	private def formatForwardCompatibleType(fileType: Type, projectionType: Type): Type = {

	public static MessageType getSchemaForRead(MessageType fileMessageType, MessageType projectedMessageType) {
	assertGroupsAreCompatible(fileMessageType, projectedMessageType);
	return projectedMessageType;
	}

Make projected parquet collection schema forward compatible with the given file schema #1921

Are you sure you want to change the base?

Make projected parquet collection schema forward compatible with the given file schema #1921

Conversation

mickjermsurawong-stripe commented Sep 27, 2019 • edited

CLAassistant commented Sep 27, 2019 • edited

mickjermsurawong-stripe commented Sep 27, 2019

mickjermsurawong-stripe commented Sep 27, 2019

johnynek left a comment

Choose a reason for hiding this comment

johnynek commented Sep 28, 2019

mickjermsurawong-stripe commented Sep 28, 2019

mickjermsurawong-stripe commented Sep 30, 2019

joshrosen-stripe 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

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaschow Sep 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickjermsurawong-stripe Oct 1, 2019 • edited

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

mickjermsurawong-stripe commented Oct 1, 2019

xton 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

mickjermsurawong-stripe Oct 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xton 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

mickjermsurawong-stripe commented Oct 4, 2019 • edited

joshrosen-stripe Oct 4, 2019 • edited

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

mickjermsurawong-stripe commented Oct 8, 2019

mickjermsurawong-stripe commented Oct 16, 2019

mickjermsurawong-stripe commented Sep 27, 2019 •

edited

CLAassistant commented Sep 27, 2019 •

edited

thomaschow Sep 30, 2019 •

edited

mickjermsurawong-stripe Oct 1, 2019 •

edited

mickjermsurawong-stripe Oct 2, 2019 •

edited

mickjermsurawong-stripe commented Oct 4, 2019 •

edited

joshrosen-stripe Oct 4, 2019 •

edited