Upload diff results from BigDiffy to BigQuery #113

anne-decusatis · 2018-07-09T20:37:09Z

fixes #106

idreeskhan

Great work, would like you to take a look at the comments and address them in code or in responses here.

idreeskhan · 2018-07-09T20:42:03Z

ratatool-diffy/src/main/scala/com/spotify/ratatool/diffy/BigDiffy.scala

-      bigDiffy.keyStats.saveAsTextFile(s"$output/keys")
-      bigDiffy.fieldStats.saveAsTextFile(s"$output/fields")
-      bigDiffy.globalStats.saveAsTextFile(s"$output/global")
+  @BigQueryType.toTable


If paradise import fixes the annotation compilation then we can probably reduce these down to a single class again

I was also seeing other errors around the subtypes not being mappable to BigQuery types - in particular, I'm saving enums and Any types as Strings in BigQuery with these new case classes. I would prefer to leave them separate.

idreeskhan · 2018-07-09T20:44:30Z

ratatool-diffy/src/main/scala/com/spotify/ratatool/diffy/BigDiffy.scala

      } catch {
        case e: Throwable =>
          usage()
          throw e
      }
    }

-    val result = mode match {
+    val saveToBq: Boolean = outputMode match {


I think I prefer to leave this as a str for 2 reasons. 1 to match the current avro/bq impl for input. 2 so that it's easier to see the effects later on the code. Can use a case match instead of if later to split it out

I can pass it in as a string and parse it in saveStats -- this has the downside that it will still run the analysis & only fail at the very end when saving. The code I have now throws an exception before doing any work if the output mode is invalid, since this branch is early in the main function. What do you think is better? Could also do a validation before running and still pass around the string but I find that a bit harder to follow, personally.

Enum value?

idreeskhan · 2018-07-09T20:46:24Z

ratatool-diffy/src/main/scala/com/spotify/ratatool/diffy/BigDiffy.scala

-      bigDiffy.fieldStats.saveAsTextFile(s"$output/fields")
-      bigDiffy.globalStats.saveAsTextFile(s"$output/global")
+  @BigQueryType.toTable
+  case class KeyStatsBigQuery(key: String, diffType: String, delta: Option[DeltaBigQuery])


Outside the scope of this PR but we can potentially add @description annotations here now that it can write out to BQ to make it clearer for users what fields are.

should I create a follow up issue for this?

codecov-io · 2018-07-09T21:05:10Z

Codecov Report

Merging #113 into master will decrease coverage by 1.23%.
The diff coverage is 18.42%.

@@            Coverage Diff             @@
##           master     #113      +/-   ##
==========================================
- Coverage   72.56%   71.33%   -1.24%     
==========================================
  Files          29       29              
  Lines        1367     1392      +25     
  Branches      165      163       -2     
==========================================
+ Hits          992      993       +1     
- Misses        375      399      +24

Flag	Coverage Δ
#ratatoolCli	`4.45% <0%> (-0.12%)`	⬇️
#ratatoolCommon	`100% <ø> (?)`
#ratatoolDiffy	`27.04% <0%> (-0.59%)`	⬇️
#ratatoolExamples	`19.38% <18.42%> (-0.15%)`	⬇️
#ratatoolSampling	`61.86% <ø> (-0.24%)`	⬇️
#ratatoolScalacheck	`82.11% <ø> (ø)`	⬆️
#ratatoolShapeless	`5.38% <0%> (-0.14%)`	⬇️

Impacted Files	Coverage Δ
...in/scala/com/spotify/ratatool/diffy/BigDiffy.scala	`45.33% <18.42%> (-6.67%)`	⬇️
...la/com/spotify/ratatool/samplers/AvroSampler.scala	`96.87% <0%> (-3.13%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b1b586...088e884. Read the comment docs.

tkhduracell · 2018-07-16T14:10:12Z

ratatool-diffy/src/main/scala/com/spotify/ratatool/diffy/BigDiffy.scala

+    val keyStatsPath = s"$output/keys"
+    val fieldStatsPath = s"$output/fields"
+    val globalStatsPath = s"$output/global"
+    if (!saveToBq) {


Unnecessary negation?

tkhduracell · 2018-07-16T14:17:29Z

ratatool-diffy/src/main/scala/com/spotify/ratatool/diffy/BigDiffy.scala

+      if (withHeader) {
+        bigDiffy.keyStats.map(_.toString).saveAsTextFileWithHeader(keyStatsPath, "key\tdifftype")
+        bigDiffy.fieldStats.map(_.toString).saveAsTextFileWithHeader(fieldStatsPath,
+          "field\tcount\tfraction\tdeltaType\tmin" +


Could maybe we nice with "\t".join("field", "count", ...)? Or using the case class to generate this header?

tkhduracell · 2018-07-16T14:23:46Z

ratatool-diffy/src/main/scala/com/spotify/ratatool/diffy/BigDiffy.scala

+        ))
+        .saveAsTypedBigQuery(s"${output}_keys")
+      bigDiffy.fieldStats.map(stat =>
+        FieldStatsBigQuery(stat.field, stat.count, stat.fraction, stat.deltaStats.map(ds =>


Maybe this transformation could live in a companion to FieldStatsBigquery, or similar approach?
bigDiffy.fieldStats.map(FieldStatsBigquery.fromValue)

tkhduracell · 2018-07-16T14:24:29Z

ratatool-diffy/src/main/scala/com/spotify/ratatool/diffy/BigDiffy.scala

+        bigDiffy.globalStats.map(_.toString).saveAsTextFileWithHeader(globalStatsPath,
+          "numTotal\tnumSame\tnumDiff\tnumMissingLhs\tnumMissingRhs")
+      }
+      else {


Should newline be here?

jbigred1 · 2018-08-14T18:11:31Z

ratatool-diffy/src/main/scala/com/spotify/ratatool/diffy/BigDiffy.scala

+  /** saves stats to either GCS as text, or BigQuery */
+  def saveStats[T](bigDiffy: BigDiffy[T], output: String, withHeader: Boolean = false,
+                   outputMode: OutputMode = GCS): Unit = {
+    val keyStatsPath = s"$output/keys"


can we move these to exist inside the GCS match statement

jbigred1 · 2018-08-14T18:17:12Z

ratatool-diffy/src/main/scala/com/spotify/ratatool/diffy/BigDiffy.scala

@@ -329,7 +383,8 @@ object BigDiffy extends Command {
      s"""BigDiffy - pair-wise field-level statistical diff
        |Usage: ratatool $command [dataflow_options] [options]
        |
-        |  --mode=[avro|bigquery]


do we want to support mode as well for backwards compatibility

I think it's fine to break it, will add a more helpful log error if mode is passed in

jbigred1 · 2018-08-14T18:18:48Z

could we add tests for this

it should be easy to test this with some input just to confirm the transformations are correct

jbigred1

Looks good once the comments are addressed

catherinejelder · 2018-08-14T20:34:35Z

ratatool-diffy/src/main/scala/com/spotify/ratatool/diffy/BigDiffy.scala

-        |  --unordered=<keys>     ',' separated field list to treat as unordered
-        |  [--with-header]        Output all TSVs with header rows
+        |  --input-mode=(avro|bigquery)     Diff-ing Avro or BQ records
+        |  [--output-mode=(gcs|bigquery)]   Saves to a text file in GCS or a BigQuery dataset


what do the braces connote - optional args?

Yea, common convention in bash help/usage guides

But I was making this optional and then reverted the change, so usage needs to be updated

anne-decusatis added 3 commits July 9, 2018 14:14

first pass at saving results to BigQuery tables

4997873

update flags to better names for input and output

ddabcd9

save delta as nested field not as String

2bf63f2

idreeskhan reviewed Jul 9, 2018

View reviewed changes

tkhduracell reviewed Jul 16, 2018

View reviewed changes

idreeskhan added 2 commits August 13, 2018 16:57

Output Enum, clean up tab separation

e61620d

Merge branch 'master' into bigquery_upload_bigdiffy_results

5786779

idreeskhan mentioned this pull request Aug 13, 2018

Add @description annotations for BigDiffy output to BQ #119

Open

idreeskhan requested review from catherinejelder and jbigred1 August 13, 2018 21:19

jbigred1 reviewed Aug 14, 2018

View reviewed changes

jbigred1 approved these changes Aug 14, 2018

View reviewed changes

Adjust README and usage docs

088e884

catherinejelder approved these changes Aug 14, 2018

View reviewed changes

Update docs, fix default to GCS output

3bc5251

idreeskhan merged commit 98f7fb3 into spotify:master Aug 16, 2018

idreeskhan deleted the bigquery_upload_bigdiffy_results branch August 16, 2018 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upload diff results from BigDiffy to BigQuery #113

Upload diff results from BigDiffy to BigQuery #113

anne-decusatis commented Jul 9, 2018

idreeskhan left a comment

idreeskhan Jul 9, 2018

anne-decusatis Jul 10, 2018

idreeskhan Jul 9, 2018

anne-decusatis Jul 10, 2018

tkhduracell Jul 16, 2018

idreeskhan Jul 9, 2018

anne-decusatis Jul 10, 2018

idreeskhan Aug 13, 2018

codecov-io commented Jul 9, 2018 •

edited

Loading

tkhduracell Jul 16, 2018

tkhduracell Jul 16, 2018 •

edited

Loading

tkhduracell Jul 16, 2018 •

edited

Loading

tkhduracell Jul 16, 2018

jbigred1 Aug 14, 2018

jbigred1 Aug 14, 2018

idreeskhan Aug 14, 2018

jbigred1 commented Aug 14, 2018

jbigred1 left a comment

catherinejelder Aug 14, 2018

idreeskhan Aug 16, 2018 •

edited

Loading

idreeskhan Aug 16, 2018

Upload diff results from BigDiffy to BigQuery #113

Upload diff results from BigDiffy to BigQuery #113

Conversation

anne-decusatis commented Jul 9, 2018

idreeskhan 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

codecov-io commented Jul 9, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

tkhduracell Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

tkhduracell Jul 16, 2018 • edited Loading

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

jbigred1 commented Aug 14, 2018

jbigred1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idreeskhan Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 9, 2018 •

edited

Loading

tkhduracell Jul 16, 2018 •

edited

Loading

tkhduracell Jul 16, 2018 •

edited

Loading

idreeskhan Aug 16, 2018 •

edited

Loading