fix: check fields by name when we can

In some cases we do not care about the order of the fields as long as the name and type matches.
nrkno · Aug 23, 2023 · 7a33cb4 · 7a33cb4
1 parent 29f2543
commit 7a33cb4
Show file tree

Hide file tree

Showing 4 changed files with 106 additions and 9 deletions.
diff --git a/core/src/main/scala/no/nrk/bigquery/conforms.scala b/core/src/main/scala/no/nrk/bigquery/conforms.scala
@@ -34,7 +34,8 @@ object conforms {
 
     apply(
       actualSchema.copy(fields = actualSchema.fields.map(anonymize)),
-      givenSchema
+      givenSchema,
+      checkIndex = true
     )
   }
 
@@ -60,17 +61,18 @@ object conforms {
       case other => BQSchema.of(other)
     }
 
-    apply(actualSchema, givenSchema)
+    apply(actualSchema, givenSchema, checkIndex = false)
   }
 
-  /* this is a stronger comparison than `onlyTypes`, because it takes into column names as well */
+  /** this is a stronger comparison than `onlyTypes`, because it takes into column names as well */
   def apply(
       actualSchema: BQSchema,
-      givenSchema: BQSchema
+      givenSchema: BQSchema,
+      checkIndex: Boolean
   ): Option[List[String]] = {
     val reasonsBuilder = List.newBuilder[String]
 
-    def go(
+    def goByIndex(
         path: List[BQField],
         actualFields: Seq[BQField],
         givenFields: Seq[BQField]
@@ -90,15 +92,42 @@ object conforms {
           case Some(givenField) if (givenField.mode == Mode.REPEATED) != (actualField.mode == Mode.REPEATED) =>
             reasonsBuilder += s"Expected ${render(actualField)} to have mode ${actualField.mode}, got ${givenField.mode}"
           case Some(givenField) if givenField.subFields.nonEmpty =>
-            go(givenField :: path, actualField.subFields, givenField.subFields)
+            goByIndex(givenField :: path, actualField.subFields, givenField.subFields)
           case Some(ok @ _) =>
             ()
           case None =>
             reasonsBuilder += s"Expected ${render(actualField)} at 0-based index $idx, but it given table/struct was shorter"
         }
       }
 
-    go(Nil, actualSchema.fields, givenSchema.fields)
+    def goByName(
+        path: List[BQField],
+        actualFields: Seq[BQField],
+        givenFields: Seq[BQField]
+    ): Unit =
+      actualFields.foreach { actualField =>
+        val givenFieldOpt = givenFields.find(_.name == actualField.name)
+
+        // if we're inside structs, render the full path
+        def render(f: BQField) =
+          s"field `${(f :: path).reverse.map(_.name).mkString(".")}`"
+
+        givenFieldOpt match {
+          case Some(givenField) if givenField.tpe != actualField.tpe =>
+            reasonsBuilder += s"Expected ${render(actualField)} to have type ${actualField.tpe}, got ${givenField.tpe}"
+          case Some(givenField) if (givenField.mode == Mode.REPEATED) != (actualField.mode == Mode.REPEATED) =>
+            reasonsBuilder += s"Expected ${render(actualField)} to have mode ${actualField.mode}, got ${givenField.mode}"
+          case Some(givenField) if givenField.subFields.nonEmpty =>
+            goByName(givenField :: path, actualField.subFields, givenField.subFields)
+          case Some(ok @ _) =>
+            ()
+          case None =>
+            reasonsBuilder += s"Expected ${render(actualField)} to part of [${givenFields.map(_.name).mkString(", ")}]"
+        }
+      }
+
+    if (checkIndex) goByIndex(Nil, actualSchema.fields, givenSchema.fields)
+    else goByName(Nil, actualSchema.fields, givenSchema.fields)
 
     reasonsBuilder.result() match {
       case Nil => None

diff --git a/core/src/main/scala/no/nrk/bigquery/internal/TableUpdateOperation.scala b/core/src/main/scala/no/nrk/bigquery/internal/TableUpdateOperation.scala
@@ -41,7 +41,8 @@ object TableUpdateOperation {
                 val illegalSchemaExtension: Option[UpdateOperation.IllegalSchemaExtension] =
                   conforms(
                     actualSchema = remoteAsTableDef.schema,
-                    givenSchema = localTableDef.schema
+                    givenSchema = localTableDef.schema,
+                    checkIndex = true
                   ).map { reasons =>
                     UpdateOperation.IllegalSchemaExtension(
                       TableDefOperationMeta(existingRemoteTable, tableDef),

diff --git a/core/src/test/scala/no/nrk/bigquery/conformsTest.scala b/core/src/test/scala/no/nrk/bigquery/conformsTest.scala
@@ -0,0 +1,67 @@
+package no.nrk.bigquery
+
+import com.google.cloud.bigquery.{Field, StandardSQLTypeName}
+import munit.FunSuite
+
+class conformsTest extends FunSuite {
+  test("failing check by index") {
+    val res = conforms(
+      BQSchema.of(
+        BQField("one", StandardSQLTypeName.STRING, Field.Mode.NULLABLE),
+        BQField("two", StandardSQLTypeName.INT64, Field.Mode.NULLABLE)
+      ),
+      BQSchema.of(
+        BQField("one", StandardSQLTypeName.INT64, Field.Mode.NULLABLE),
+        BQField("two", StandardSQLTypeName.STRING, Field.Mode.NULLABLE)
+      ),
+      checkIndex = true
+    )
+    assertEquals(res.map(_.length), Some(2), clue(res.map(_.mkString(", "))))
+  }
+
+  test("passing check by index") {
+    val res = conforms(
+      BQSchema.of(
+        BQField("one", StandardSQLTypeName.STRING, Field.Mode.NULLABLE),
+        BQField("two", StandardSQLTypeName.INT64, Field.Mode.NULLABLE)
+      ),
+      BQSchema.of(
+        BQField("one", StandardSQLTypeName.STRING, Field.Mode.NULLABLE),
+        BQField("two", StandardSQLTypeName.INT64, Field.Mode.NULLABLE)
+      ),
+      checkIndex = true
+    )
+    assertEquals(res.map(_.length), None, clue(res.map(_.mkString(", "))))
+  }
+
+  test("passing check by name") {
+    val res = conforms(
+      BQSchema.of(
+        BQField("one", StandardSQLTypeName.STRING, Field.Mode.NULLABLE),
+        BQField("two", StandardSQLTypeName.INT64, Field.Mode.NULLABLE)
+      ),
+      BQSchema.of(
+        BQField("two", StandardSQLTypeName.INT64, Field.Mode.NULLABLE),
+        BQField("one", StandardSQLTypeName.STRING, Field.Mode.NULLABLE)
+      ),
+      checkIndex = false
+    )
+    assertEquals(res.map(_.length), None, clue(res.map(_.mkString(", "))))
+  }
+
+  test("failing check by name") {
+    val res = conforms(
+      BQSchema.of(
+        BQField("one", StandardSQLTypeName.STRING, Field.Mode.NULLABLE),
+        BQField("two", StandardSQLTypeName.INT64, Field.Mode.NULLABLE)
+      ),
+      BQSchema.of(
+        BQField("two", StandardSQLTypeName.STRING, Field.Mode.NULLABLE),
+        BQField("one", StandardSQLTypeName.INT64, Field.Mode.NULLABLE)
+      ),
+      checkIndex = false
+    )
+    assertEquals(res.map(_.length), Some(2), clue(res.map(_.mkString(", "))))
+  }
+
+}
diff --git a/testing/src/main/scala/no/nrk/bigquery/testing/BQSmokeTest.scala b/testing/src/main/scala/no/nrk/bigquery/testing/BQSmokeTest.scala
@@ -291,7 +291,7 @@ object BQSmokeTest {
     def checkSchema(actualSchema: BQSchema): Unit =
       this match {
         case CheckType.Schema(expectedSchema) =>
-          conforms(actualSchema, expectedSchema) match {
+          conforms(actualSchema, expectedSchema, checkIndex = true) match {
             case Some(reasons) =>
               fail(s"Failed because ${reasons.mkString(", ")}", TypeClue(expectedSchema, actualSchema))
             case None => assert(true)