add rowRestriction for bigDiffy bq inputs #489

jrmcglynn · 2021-10-08T21:02:50Z

This PR adds a rowRestriction argument to bigDiffy for BigQuery inputs. This allows for comparing partitions of natively partitioned BigQuery tables, but could be used in other ways as well. Moving BigQuery reads to bigQueryStorage was a necessity to implement this efficiently, but should be (faster and cheaper)[https://spotify.github.io/scio/io/BigQuery.html#bigquerytype-fromstorage] in addition.

rowRestriction was not implemented for avro inputs so a check was made to ensure that users do not pass in that combination of arguments.

codecov · 2021-10-08T21:06:39Z

Codecov Report

Merging #489 (09796d6) into master (274ec55) will increase coverage by 0.13%.
The diff coverage is 69.23%.

@@            Coverage Diff             @@
##           master     #489      +/-   ##
==========================================
+ Coverage   69.21%   69.35%   +0.13%     
==========================================
  Files          35       35              
  Lines        1517     1527      +10     
  Branches      129      133       +4     
==========================================
+ Hits         1050     1059       +9     
- Misses        467      468       +1

Flag	Coverage Δ
ratatoolCli	`2.99% <0.00%> (-0.03%)`	⬇️
ratatoolCommon	`∅ <ø> (∅)`
ratatoolDiffy	`32.14% <69.23%> (+0.45%)`	⬆️
ratatoolExamples	`17.91% <0.00%> (-0.13%)`	⬇️
ratatoolSampling	`60.89% <ø> (ø)`
ratatoolScalacheck	`80.48% <ø> (ø)`
ratatoolShapeless	`5.06% <0.00%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...in/scala/com/spotify/ratatool/diffy/BigDiffy.scala	`53.84% <69.23%> (+1.71%)`	⬆️

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 274ec55...09796d6. Read the comment docs.

catherinejelder · 2021-10-14T19:49:52Z

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

    keyFn: TableRow => MultiKey,
    diffy: TableRowDiffy,
    ignoreNan: Boolean = false
-  ): BigDiffy[TableRow] =
+  ): BigDiffy[TableRow] = {
+    val restrictionCleaned = rowRestriction.map(_.replace('"', ' '))


forgive my ignorance re: the storage api, but why is this replace needed?

the rowRestriction argument will often be quoted in the CLI, e.g, --rowRestriction="date = DATE '2021-10-01'". regrettably the quotation marks themselves also get passed into the JVM. this replace remove those quotation marks.

could the quotation marks be stripped from the beginning and end of the string instead? a quick ghe codesearch shows a lot of variety in how quotation marks are used in this arg, including stuff like "lifecycle_state = \"DELETE_REQUESTED\""

We should probably just strip both types of quote to avoid this

are you thinking something like this? handles ", ', or `; and makes sure that the character being replaced is not in the middle of the line

val restrictionCleaned = rowRestriction.map( _.replaceAll("(^[\"'`])|([\"'`]$)", s"") )

see my update here -- checks if the first and last characters are quote chars and are identical to each other, and removes if so

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

catherinejelder · 2021-10-14T19:53:21Z

ratatool-diffy/src/test/scala/com/spotify/ratatool/diffy/BigDiffyTest.scala

@@ -317,4 +317,23 @@ class BigDiffyTest extends PipelineSpec {

    exc.getMessage shouldBe "Output mode is GCS, but output abc is not a valid GCS location"
  }
+


would it be possible to add a test for bq as well?

this would require mocking the BQ client so that it returns schemas, data, etc. I haven't done this in Scala before, so I don't have an opinion on whether that is worth doing here. what do you think?

it's a thin enough wrapper around the scio api that it might be ok to not test it, but it might be worth asking flatmap if they'd recommend it and if there's a straightforward way to do it

I think this might be harder because of our BQ hacks in this repo, IIRC that's why we didn't do this before. But we should look at adding some integration tests or something

I'm on vacation for the next week, but I can give this a shot when I get back

catherinejelder · 2021-10-21T18:24:46Z

some of our tests in the sampling module are non-deterministic and should pass if you re-run once or twice - sorry for the inconvenience there

idreeskhan · 2021-10-21T19:06:53Z

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

    keyFn: TableRow => MultiKey,
    diffy: TableRowDiffy,
    ignoreNan: Boolean = false
-  ): BigDiffy[TableRow] =
+  ): BigDiffy[TableRow] = {
+    val restrictionCleaned = rowRestriction.map(_.replace('"', ' '))


We should probably just strip both types of quote to avoid this

idreeskhan · 2021-10-21T19:08:49Z

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

@@ -700,6 +707,10 @@ object BigDiffy extends Command with Serializable {
    // validity checks passed, ok to run the diff
    val result = inputMode match {
      case "avro" =>
+        if(rowRestriction.isDefined) {
+          throw new NotImplementedError(s"rowRestriction is not implemented for avro inputs")


Can we have a clearer error message here? It's not that it's not implemented, it's that it is only supported by BigQuery (meaning it won't be implemented in the future)

good call! lmk what you think of the updated exception

idreeskhan · 2021-10-21T19:10:11Z

ratatool-diffy/src/test/scala/com/spotify/ratatool/diffy/BigDiffyTest.scala

@@ -317,4 +317,23 @@ class BigDiffyTest extends PipelineSpec {

    exc.getMessage shouldBe "Output mode is GCS, but output abc is not a valid GCS location"
  }
+


I think this might be harder because of our BQ hacks in this repo, IIRC that's why we didn't do this before. But we should look at adding some integration tests or something

…gdiffy-bqRowRestriction

jrmcglynn · 2022-01-07T16:48:46Z

I've successfully tested the following scenarios by triggering from the CLI locally:

rowRestriction arg with wrapped quotes, --rowRestriction="date=DATE '2021-10-01'"
rowRestriction arg with no quotes, --rowRestriction=date=DATE'2021-10-01'
rowRestriction arg with a compound filter, --rowRestriction="licensor='Universal Music Group' AND artist_name=‘Avicii'"
No rowRestriction

For each rowRestriction scenario I've confirmed that the filter was successfully applied on the input. The only time the filter was not successfully applied was when I did --rowRestriction="date='2021-10-01'"; apparently BigQuery's storage API is picky about needing the date string to be explicitly cast into a date type.

Let me know any further thoughts here!

idreeskhan

Looks good, can I merge?

idreeskhan · 2022-01-10T15:41:50Z

Oh wait sorry one more thing, can you document the potential inputs for rowRestriction somewhere? Those are like 3 different types that you tested but only one is in the CLI. Maybe just somewhere in one of the READMEs is good

jrmcglynn · 2022-01-10T21:04:27Z

@idreeskhan I added a section in the BigDiffy README, is that along the lines of what you were thinking?

I also modified the CLI docs to make it clear that it's an optional param and match Google's doc's for this parameter

John McGlynn added 2 commits October 8, 2021 16:48

add rowRestriction for bigDiffy bq inputs

6842330

bigDiffy rowRestriction doc update

60ed583

jrmcglynn mentioned this pull request Oct 8, 2021

bigDiffy comparison between natively partitioned BigQuery tables #483

Open

catherinejelder reviewed Oct 14, 2021

View reviewed changes

add rowRestriction to bigDiffy README

3872285

idreeskhan reviewed Oct 21, 2021

View reviewed changes

John McGlynn added 6 commits November 24, 2021 17:27

update exception for rowRestriction with avro input

25a4e38

update expected rowRestriction exception in test

c0d030e

update cleaning method for rowRestriction argument in BigDiffy

e0075b1

modify docs for rowRestriction arg

3c451c9

add example rowRestriction arg to docs

36623e1

Merge branch 'master' of github.com:spotify/ratatool into jmcglynn/bi…

21e1cbc

…gdiffy-bqRowRestriction

idreeskhan approved these changes Jan 10, 2022

View reviewed changes

udpate rowRestriction docs for BigDiffy

09796d6

idreeskhan merged commit 5129b0c into spotify:master Jan 14, 2022

jrmcglynn mentioned this pull request Jan 21, 2022

add support for time-partitioned BQ partitions in bigDiffy [WIP] #485

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add rowRestriction for bigDiffy bq inputs #489

add rowRestriction for bigDiffy bq inputs #489

jrmcglynn commented Oct 8, 2021

codecov bot commented Oct 8, 2021 •

edited

Loading

catherinejelder Oct 14, 2021

jrmcglynn Oct 14, 2021

catherinejelder Oct 21, 2021 •

edited

Loading

idreeskhan Oct 21, 2021

jrmcglynn Nov 24, 2021

jrmcglynn Jan 7, 2022

catherinejelder Oct 14, 2021

jrmcglynn Oct 14, 2021

catherinejelder Oct 21, 2021

idreeskhan Oct 21, 2021

jrmcglynn Nov 29, 2021

catherinejelder commented Oct 21, 2021

idreeskhan Oct 21, 2021

idreeskhan Oct 21, 2021

jrmcglynn Nov 24, 2021

idreeskhan Oct 21, 2021

jrmcglynn commented Jan 7, 2022

idreeskhan left a comment

idreeskhan commented Jan 10, 2022 •

edited

Loading

jrmcglynn commented Jan 10, 2022

		@@ -317,4 +317,23 @@ class BigDiffyTest extends PipelineSpec {

		exc.getMessage shouldBe "Output mode is GCS, but output abc is not a valid GCS location"
		}

add rowRestriction for bigDiffy bq inputs #489

add rowRestriction for bigDiffy bq inputs #489

Conversation

jrmcglynn commented Oct 8, 2021

codecov bot commented Oct 8, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catherinejelder Oct 21, 2021 • 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

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

catherinejelder commented Oct 21, 2021

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

jrmcglynn commented Jan 7, 2022

idreeskhan left a comment

Choose a reason for hiding this comment

idreeskhan commented Jan 10, 2022 • edited Loading

jrmcglynn commented Jan 10, 2022

codecov bot commented Oct 8, 2021 •

edited

Loading

catherinejelder Oct 21, 2021 •

edited

Loading

idreeskhan commented Jan 10, 2022 •

edited

Loading