[GitHub] [spark] rdblue commented on a change in pull request #29890: [SPARK-33014][SQL] Support multiple bucket columns in DataSourceV2 table

[GitBox <gi...@apache.org>]

rdblue commented on a change in pull request #29890:
URL: https://github.com/apache/spark/pull/29890#discussion_r529079681



##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/connector/expressions/TransformExtractorSuite.scala
##########
@@ -130,23 +130,32 @@ class TransformExtractorSuite extends SparkFunSuite {
   }
 
   test("Bucket extractor") {
-    val col = ref("a", "b")
-    val bucketTransform = new Transform {
-      override def name: String = "bucket"
-      override def references: Array[NamedReference] = Array(col)
-      override def arguments: Array[Expression] = Array(lit(16), col)
-      override def describe: String = s"bucket(16, ${col.describe})"
+    def checkBucketTransform(cols: Seq[NamedReference], expectedBucketCols: Seq[String]): Unit = {
+      new Transform {
+        override def name: String = "bucket"
+        override def references: Array[NamedReference] = cols.toArray
+        override def arguments: Array[Expression] = Array(lit(16)) ++ cols
+        override def describe: String = s"bucket(${arguments.map(_.describe).mkString(", ")})"
+      } match {
+        case BucketTransform(numBuckets, fieldReferences) =>
+          val bucketColumns = fieldReferences.collect {
+            case FieldReference(parts) => parts.mkString(".")
+          }
+          assert(numBuckets === 16)
+          assert(bucketColumns === expectedBucketCols)
+        case _ =>
+          fail("Did not match BucketTransform extractor")
+      }
     }
 
-    bucketTransform match {
-      case BucketTransform(numBuckets, FieldReference(seq)) =>
-        assert(numBuckets === 16)
-        assert(seq === Seq("a", "b"))
-      case _ =>
-        fail("Did not match BucketTransform extractor")
-    }
+    // CLUSTERED BY (a, b)
+    checkBucketTransform(Seq(ref("a"), ref("b")), Seq("a", "b"))
+    // CLUSTERED BY (a.b)
+    checkBucketTransform(Seq(ref("a", "b")), Seq("a.b"))
+    checkBucketTransform(Seq(ref("a.b")), Seq("a.b"))
+    checkBucketTransform(Seq(ref("`a.b`")), Seq("`a.b`"))
 
-    transform("unknown", ref("a", "b")) match {
+    transform("unknown", ref("c")) match {

Review comment:
       Why did this test case change?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org