BigQuery JSON column: encode as Jackson JsonNode #5523

turb · 2024-11-14T14:45:40Z

On write, BigQuery JSON columns must not be provided as String, as it will be escaped. Apache Beam warns:

Make sure the TableRow value is a parsed JSON to ensure the read as a JSON type. Otherwise it will read as a raw (escaped) string.

Reverse-eng. shows that is means "as a JsonNode from Jackson".

codecov · 2024-11-14T17:41:59Z

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 61.44%. Comparing base (c45685a) to head (d966487).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...cala/com/spotify/scio/bigquery/types/package.scala	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5523      +/-   ##
==========================================
+ Coverage   61.43%   61.44%   +0.01%     
==========================================
  Files         312      312              
  Lines       11103    11105       +2     
  Branches      762      744      -18     
==========================================
+ Hits         6821     6824       +3     
+ Misses       4282     4281       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

…escape

turb · 2024-11-14T19:55:24Z

@RustedBones what remains here is binary compatibility: newly compiled Json fails if executed with previous lib.

RustedBones

We need an integration test for that to make sure both avro and json formats are ok with those types

RustedBones · 2024-11-15T10:20:28Z

...google-cloud-platform/src/main/scala/com/spotify/scio/bigquery/types/ConverterProvider.scala

+        case t if t =:= typeOf[Float]                                   => q"$tree.toDouble"
+        case t if t =:= typeOf[Double]                                  => tree
+        case t if t =:= typeOf[String]                                  => tree
+        case t if t =:= typeOf[com.fasterxml.jackson.databind.JsonNode] => tree


I don't think we should accept JsonNode here

I don't actually need it, but I supposed one may want to provide an already parsed (hence valid) JSON. I will remove it.

RustedBones · 2024-11-15T10:23:24Z

...google-cloud-platform/src/main/scala/com/spotify/scio/bigquery/types/ConverterProvider.scala

@@ -198,7 +199,7 @@ private[types] object ConverterProvider {
        case t if t =:= typeOf[Geography] =>
          q"$tree.wkt"
        case t if t =:= typeOf[Json] =>
-          q"$tree.wkt"
+          q"$tree.asJackson"


According to the doc here, loading json in avro expects a string type, annotated with the JSON sqlType property

There is the same comment for BigQuery, but after a reverse-eng. they meant JsonNode. If opened a PR just to change the warning: apache/beam#33121

However I can keep asJackson on line 417, and .wkt on line 202.

RustedBones · 2024-11-15T10:24:52Z

...google-cloud-platform/src/main/scala/com/spotify/scio/bigquery/types/ConverterProvider.scala

+        case t if t =:= typeOf[Float]                                   => q"$s.toFloat"
+        case t if t =:= typeOf[Double]                                  => q"$s.toDouble"
+        case t if t =:= typeOf[String]                                  => q"$s"
+        case t if t =:= typeOf[com.fasterxml.jackson.databind.JsonNode] => q"$s"


I don't think we should accept JsonNode here

RustedBones · 2024-11-15T10:25:09Z

...google-cloud-platform/src/main/scala/com/spotify/scio/bigquery/types/ConverterProvider.scala

@@ -412,7 +414,7 @@ private[types] object ConverterProvider {
        case t if t =:= typeOf[Geography] =>
          q"$tree.wkt"
        case t if t =:= typeOf[Json] =>
-          q"$tree.wkt"
+          q"$tree.asJackson"


RustedBones · 2024-11-15T10:26:17Z

scio-google-cloud-platform/src/main/scala/com/spotify/scio/bigquery/types/package.scala

+  case class Json(wkt: String) {
+    def asJackson: JsonNode = Json.mapper.readTree(wkt)
+  }
+  object Json {
+    private val mapper = new ObjectMapper()
+  }


I'd prefer to have this in the conversion part and leave the data as dumb as possible

Where would be the appropriate place to store the ObjectMapper instance, in order to not instantiate it for each row? Directly as a private val in ConverterProvider?

RustedBones · 2024-11-15T10:27:43Z

...le-cloud-platform/src/test/scala/com/spotify/scio/bigquery/types/ConverterProviderSpec.scala

+    for {
+      key <- Gen.alphaStr
+      value <- Gen.alphaStr
+    } yield Json("{\"" + key + "\":\"" + value + "\"}")


Nit: triple quote is nicer when writing json

Suggested change

} yield Json("{\"" + key + "\":\"" + value + "\"}")

} yield Json(s"""{"$key":"$value"}""")

TIL. I always supposed triple quote was only for multi-line.

…revent escape

RustedBones · 2024-11-15T17:16:37Z

Wrote some integration tests on top of your PR. Revealed that BigNumeric also has a bug!
Needs some polishing, will add that on top of this work if it's fine.

Thanks again for the contrib, and nice catch!

turb force-pushed the bq-jackson branch from 839a9d1 to 4aa9177 Compare November 14, 2024 17:19

turb force-pushed the bq-jackson branch 2 times, most recently from 6583e46 to 986438b Compare November 14, 2024 18:23

BigQuery JSON column: encode as Jackson JsonNode on write to prevent …

441cb89

…escape

turb force-pushed the bq-jackson branch from 986438b to 441cb89 Compare November 14, 2024 18:47

RustedBones reviewed Nov 15, 2024

View reviewed changes

fixup! BigQuery JSON column: encode as Jackson JsonNode on write to p…

c0303bb

…revent escape

turb requested a review from RustedBones November 15, 2024 15:57

RustedBones added 5 commits November 18, 2024 10:17

Move conversion helpers to companion object for consistency

965b045

Add test for BigNumeric

34ab53e

Add integration tests

6ffa85e

format

5f46375

Ack mima change

d966487

RustedBones merged commit 49f43b0 into spotify:main Nov 18, 2024
11 checks passed

RustedBones mentioned this pull request Nov 19, 2024

Update beam to 2.61 #5525

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BigQuery JSON column: encode as Jackson JsonNode #5523

BigQuery JSON column: encode as Jackson JsonNode #5523

turb commented Nov 14, 2024 •

edited

Loading

codecov bot commented Nov 14, 2024 •

edited

Loading

turb commented Nov 14, 2024

RustedBones left a comment

RustedBones Nov 15, 2024

turb Nov 15, 2024 •

edited

Loading

turb Nov 15, 2024

RustedBones Nov 15, 2024

turb Nov 15, 2024

turb Nov 15, 2024

RustedBones Nov 15, 2024

turb Nov 15, 2024

RustedBones Nov 15, 2024

turb Nov 15, 2024

RustedBones Nov 15, 2024

turb Nov 15, 2024

turb Nov 15, 2024

RustedBones Nov 15, 2024

turb Nov 15, 2024

turb Nov 15, 2024

RustedBones commented Nov 15, 2024

	} yield Json("{\"" + key + "\":\"" + value + "\"}")
	} yield Json(s"""{"$key":"$value"}""")

BigQuery JSON column: encode as Jackson JsonNode #5523

BigQuery JSON column: encode as Jackson JsonNode #5523

Conversation

turb commented Nov 14, 2024 • edited Loading

codecov bot commented Nov 14, 2024 • edited Loading

Codecov Report

turb commented Nov 14, 2024

RustedBones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turb Nov 15, 2024 • 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

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

RustedBones commented Nov 15, 2024

turb commented Nov 14, 2024 •

edited

Loading

codecov bot commented Nov 14, 2024 •

edited

Loading

turb Nov 15, 2024 •

edited

Loading