You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/12/15 23:52:59 UTC
[GitHub] [spark] huaxingao opened a new pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
huaxingao opened a new pull request #34914:
URL: https://github.com/apache/spark/pull/34914
### What changes were proposed in this pull request?
Add tests to make sure
`bucket(numBuckets: Int, references: Array[NamedReference])`
and
`bucket(
numBuckets: Int,
references: Array[NamedReference],
sortedCols: Array[NamedReference])` work
### Why are the changes needed?
test coverage
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
New tests
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] cloud-fan commented on pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Separate SortedBucketTransform from BucketTransform
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #34914:
URL: https://github.com/apache/spark/pull/34914#issuecomment-1012673782
thanks, merging to master!
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] AmplabJenkins commented on pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #34914:
URL: https://github.com/apache/spark/pull/34914#issuecomment-995384030
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/146247/
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] huaxingao commented on pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Separate SortedBucketTransform from BucketTransform
Posted by GitBox <gi...@apache.org>.
huaxingao commented on pull request #34914:
URL: https://github.com/apache/spark/pull/34914#issuecomment-1008467123
> It's better to update the title and description accordingly before merging.
Updated. Thanks!
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] SparkQA commented on pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #34914:
URL: https://github.com/apache/spark/pull/34914#issuecomment-995372755
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50721/
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] cloud-fan commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r780009544
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -141,20 +135,47 @@ private[sql] object BucketTransform {
def unapply(transform: Transform): Option[(Int, NamedReference, NamedReference)] =
transform match {
- case NamedTransform("bucket", Seq(
- Lit(value: Int, IntegerType),
- Ref(partCols: Seq[String]),
- Ref(sortCols: Seq[String]))) =>
- Some((value, FieldReference(partCols), FieldReference(sortCols)))
- case NamedTransform("bucket", Seq(
- Lit(value: Int, IntegerType),
- Ref(partCols: Seq[String]))) =>
- Some((value, FieldReference(partCols), FieldReference(Seq.empty[String])))
+ case NamedTransform("sorted_bucket", arguments) =>
+ var index: Int = -1
+ var posOfLit: Int = -1
+ var numOfBucket: Int = -1
+ arguments.foreach {
Review comment:
nit: we can do `arguments.zipWithIndex.foreach`, so that it's much easier to get `posOfLit`.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] viirya commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r780047792
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -112,26 +111,21 @@ private[sql] final case class BucketTransform(
override def arguments: Array[Expression] = numBuckets +: columns.toArray
- override def toString: String =
- if (sortedColumns.nonEmpty) {
- s"bucket(${arguments.map(_.describe).mkString(", ")}," +
- s" ${sortedColumns.map(_.describe).mkString(", ")})"
- } else {
- s"bucket(${arguments.map(_.describe).mkString(", ")})"
- }
+ override def describe: String = s"bucket(${arguments.map(_.describe).mkString(", ")})"
+
+ override def toString: String = describe
override def withReferences(newReferences: Seq[NamedReference]): Transform = {
this.copy(columns = newReferences)
}
}
private[sql] object BucketTransform {
- def unapply(expr: Expression): Option[(Int, FieldReference, FieldReference)] =
- expr match {
+ def unapply(expr: Expression): Option[(Int, FieldReference, FieldReference)] = expr match {
Review comment:
BTW, why `def unapply(expr: Expression)` addresses only `BucketTransform` but `def unapply(transform: Transform)` addresses both `sorted_bucket` and `bucket`?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] cloud-fan commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r779588070
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -112,25 +111,65 @@ private[sql] final case class BucketTransform(
override def arguments: Array[Expression] = numBuckets +: columns.toArray
- override def toString: String =
- if (sortedColumns.nonEmpty) {
- s"bucket(${arguments.map(_.describe).mkString(", ")}," +
- s" ${sortedColumns.map(_.describe).mkString(", ")})"
- } else {
- s"bucket(${arguments.map(_.describe).mkString(", ")})"
- }
+ override def describe: String = s"bucket(${arguments.map(_.describe).mkString(", ")})"
+
+ override def toString: String = describe
override def withReferences(newReferences: Seq[NamedReference]): Transform = {
this.copy(columns = newReferences)
}
}
private[sql] object BucketTransform {
+ def unapply(expr: Expression): Option[(Int, FieldReference)] = expr match {
+ case transform: Transform =>
+ transform match {
+ case BucketTransform(n, FieldReference(parts)) =>
+ Some((n, FieldReference(parts)))
+ case _ =>
+ None
+ }
+ case _ =>
+ None
+ }
+
+ def unapply(transform: Transform): Option[(Int, NamedReference)] = transform match {
+ case NamedTransform("bucket", Seq(
+ Lit(value: Int, IntegerType),
+ Ref(seq: Seq[String]))) =>
+ Some((value, FieldReference(seq)))
+ case _ =>
+ None
+ }
+}
+
+private[sql] final case class SortedBucketTransform(
+ numBuckets: Literal[Int],
+ columns: Seq[NamedReference],
+ sortedColumns: Seq[NamedReference] = Seq.empty[NamedReference]) extends RewritableTransform {
+
+ override val name: String = "sortedBucket"
Review comment:
`sorted_bucket` is more SQL-ish.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] huaxingao commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
huaxingao commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r779185599
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -104,24 +104,29 @@ private[sql] final case class BucketTransform(
columns: Seq[NamedReference],
sortedColumns: Seq[NamedReference] = Seq.empty[NamedReference]) extends RewritableTransform {
- override val name: String = "bucket"
+ override val name: String = if (sortedColumns.nonEmpty) "sortedBucket" else "bucket"
override def references: Array[NamedReference] = {
arguments.collect { case named: NamedReference => named }
}
- override def arguments: Array[Expression] = numBuckets +: columns.toArray
-
- override def toString: String =
+ override def arguments: Array[Expression] = {
if (sortedColumns.nonEmpty) {
- s"bucket(${arguments.map(_.describe).mkString(", ")}," +
- s" ${sortedColumns.map(_.describe).mkString(", ")})"
+ (columns.toArray :+ numBuckets) ++ sortedColumns
} else {
- s"bucket(${arguments.map(_.describe).mkString(", ")})"
+ numBuckets +: columns.toArray
}
+ }
+
+ override def toString: String = s"$name(${arguments.map(_.describe).mkString(", ")})"
override def withReferences(newReferences: Seq[NamedReference]): Transform = {
- this.copy(columns = newReferences)
+ if (sortedColumns.isEmpty) {
+ this.copy(columns = newReferences)
+ } else {
+ val splits = newReferences.grouped(columns.length).toList
+ this.copy(columns = splits(0), sortedColumns = splits(1))
Review comment:
Changed. Thanks!
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -104,24 +104,29 @@ private[sql] final case class BucketTransform(
columns: Seq[NamedReference],
sortedColumns: Seq[NamedReference] = Seq.empty[NamedReference]) extends RewritableTransform {
- override val name: String = "bucket"
+ override val name: String = if (sortedColumns.nonEmpty) "sortedBucket" else "bucket"
Review comment:
Added a new class `SortedBucketTransform`. Thanks!
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala
##########
@@ -1566,18 +1566,22 @@ class DataSourceV2SQLSuite
test("create table using - with sorted bucket") {
val identifier = "testcat.table_name"
withTable(identifier) {
- sql(s"CREATE TABLE $identifier (a int, b string, c int) USING $v2Source PARTITIONED BY (c)" +
- s" CLUSTERED BY (b) SORTED by (a) INTO 4 BUCKETS")
Review comment:
Just want to make sure multiple columns/sortedColumns work ok.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] cloud-fan commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r778963636
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -104,24 +104,29 @@ private[sql] final case class BucketTransform(
columns: Seq[NamedReference],
sortedColumns: Seq[NamedReference] = Seq.empty[NamedReference]) extends RewritableTransform {
- override val name: String = "bucket"
+ override val name: String = if (sortedColumns.nonEmpty) "sortedBucket" else "bucket"
override def references: Array[NamedReference] = {
arguments.collect { case named: NamedReference => named }
}
- override def arguments: Array[Expression] = numBuckets +: columns.toArray
-
- override def toString: String =
+ override def arguments: Array[Expression] = {
if (sortedColumns.nonEmpty) {
- s"bucket(${arguments.map(_.describe).mkString(", ")}," +
- s" ${sortedColumns.map(_.describe).mkString(", ")})"
+ (columns.toArray :+ numBuckets) ++ sortedColumns
} else {
- s"bucket(${arguments.map(_.describe).mkString(", ")})"
+ numBuckets +: columns.toArray
}
+ }
+
+ override def toString: String = s"$name(${arguments.map(_.describe).mkString(", ")})"
override def withReferences(newReferences: Seq[NamedReference]): Transform = {
- this.copy(columns = newReferences)
+ if (sortedColumns.isEmpty) {
+ this.copy(columns = newReferences)
+ } else {
+ val splits = newReferences.grouped(columns.length).toList
+ this.copy(columns = splits(0), sortedColumns = splits(1))
Review comment:
is it: `columns = newReferences.take(columns.length), sortedColumns = newReferences.drop(columns.length)`
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] cloud-fan commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r778966695
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala
##########
@@ -1566,18 +1566,22 @@ class DataSourceV2SQLSuite
test("create table using - with sorted bucket") {
val identifier = "testcat.table_name"
withTable(identifier) {
- sql(s"CREATE TABLE $identifier (a int, b string, c int) USING $v2Source PARTITIONED BY (c)" +
- s" CLUSTERED BY (b) SORTED by (a) INTO 4 BUCKETS")
Review comment:
why changing this test?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] cloud-fan commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r779584584
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -45,11 +45,11 @@ private[sql] object LogicalExpressions {
def bucket(numBuckets: Int, references: Array[NamedReference]): BucketTransform =
BucketTransform(literal(numBuckets, IntegerType), references)
- def bucket(
+ def sortedBucket(
Review comment:
It's OK to keep the name `bucket`, to match the name of this SQL feature
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] cloud-fan commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r780010175
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -141,20 +135,47 @@ private[sql] object BucketTransform {
def unapply(transform: Transform): Option[(Int, NamedReference, NamedReference)] =
transform match {
- case NamedTransform("bucket", Seq(
- Lit(value: Int, IntegerType),
- Ref(partCols: Seq[String]),
- Ref(sortCols: Seq[String]))) =>
- Some((value, FieldReference(partCols), FieldReference(sortCols)))
- case NamedTransform("bucket", Seq(
- Lit(value: Int, IntegerType),
- Ref(partCols: Seq[String]))) =>
- Some((value, FieldReference(partCols), FieldReference(Seq.empty[String])))
+ case NamedTransform("sorted_bucket", arguments) =>
+ var index: Int = -1
+ var posOfLit: Int = -1
+ var numOfBucket: Int = -1
+ arguments.foreach {
+ case Lit(value: Int, IntegerType) =>
+ numOfBucket = value
+ index = index + 1
+ posOfLit = index
+ case _ => index = index + 1
+ }
+ Some(numOfBucket, FieldReference(arguments.take(posOfLit).map(_.describe)),
+ FieldReference(arguments.drop(posOfLit + 1).map(_.describe)))
+ case NamedTransform("bucket", Seq(Lit(value: Int, IntegerType), Ref(seq: Seq[String]))) =>
Review comment:
this doesn't seem to be right. It only matches `bucket` with a single bucket column.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] AmplabJenkins commented on pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #34914:
URL: https://github.com/apache/spark/pull/34914#issuecomment-995379812
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/50721/
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] SparkQA commented on pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #34914:
URL: https://github.com/apache/spark/pull/34914#issuecomment-995346660
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50721/
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] SparkQA commented on pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #34914:
URL: https://github.com/apache/spark/pull/34914#issuecomment-995383839
**[Test build #146247 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/146247/testReport)** for PR 34914 at commit [`7188482`](https://github.com/apache/spark/commit/7188482b824b879746a8f0cf203f0e1bae6cd883).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] SparkQA removed a comment on pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #34914:
URL: https://github.com/apache/spark/pull/34914#issuecomment-995323921
**[Test build #146247 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/146247/testReport)** for PR 34914 at commit [`7188482`](https://github.com/apache/spark/commit/7188482b824b879746a8f0cf203f0e1bae6cd883).
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] viirya commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r777747567
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -145,14 +150,21 @@ private[sql] object BucketTransform {
def unapply(transform: Transform): Option[(Int, NamedReference, NamedReference)] =
transform match {
- case NamedTransform("bucket", Seq(
- Lit(value: Int, IntegerType),
- Ref(partCols: Seq[String]),
- Ref(sortCols: Seq[String]))) =>
- Some((value, FieldReference(partCols), FieldReference(sortCols)))
- case NamedTransform("bucket", Seq(
- Lit(value: Int, IntegerType),
- Ref(partCols: Seq[String]))) =>
+ case NamedTransform("sortedBucket", s) =>
+ var index: Int = -1
+ var posOfLit: Int = -1
+ var numOfBucket: Int = -1
+ s.foreach {
+ case Lit(value: Int, IntegerType) =>
Review comment:
There is an extra space between `case` and `Lit`.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] cloud-fan commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r778964484
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -104,24 +104,29 @@ private[sql] final case class BucketTransform(
columns: Seq[NamedReference],
sortedColumns: Seq[NamedReference] = Seq.empty[NamedReference]) extends RewritableTransform {
- override val name: String = "bucket"
+ override val name: String = if (sortedColumns.nonEmpty) "sortedBucket" else "bucket"
Review comment:
Can we create a new class `SortedBucketTransform`?
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -104,24 +104,29 @@ private[sql] final case class BucketTransform(
columns: Seq[NamedReference],
sortedColumns: Seq[NamedReference] = Seq.empty[NamedReference]) extends RewritableTransform {
- override val name: String = "bucket"
+ override val name: String = if (sortedColumns.nonEmpty) "sortedBucket" else "bucket"
Review comment:
Can we create a new class `SortedBucketTransform` to be clearer?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] cloud-fan commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r770236588
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -112,15 +112,9 @@ private[sql] final case class BucketTransform(
arguments.collect { case named: NamedReference => named }
}
- override def arguments: Array[Expression] = numBuckets +: columns.toArray
+ override def arguments: Array[Expression] = (numBuckets +: columns.toArray) ++ sortedColumns
Review comment:
does this really work? I don't see a way for people to extract bucket and sort columns from arguments with the `Transform` API.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] huaxingao commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
huaxingao commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r777785755
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -106,26 +106,31 @@ private[sql] final case class BucketTransform(
columns: Seq[NamedReference],
sortedColumns: Seq[NamedReference] = Seq.empty[NamedReference]) extends RewritableTransform {
- override val name: String = "bucket"
+ override val name: String = if (sortedColumns.nonEmpty) "sortedBucket" else "bucket"
override def references: Array[NamedReference] = {
arguments.collect { case named: NamedReference => named }
}
- override def arguments: Array[Expression] = numBuckets +: columns.toArray
-
- override def describe: String =
+ override def arguments: Array[Expression] = {
if (sortedColumns.nonEmpty) {
- s"bucket(${arguments.map(_.describe).mkString(", ")}," +
- s" ${sortedColumns.map(_.describe).mkString(", ")})"
+ (columns.toArray :+ numBuckets) ++ sortedColumns
} else {
- s"bucket(${arguments.map(_.describe).mkString(", ")})"
+ numBuckets +: columns.toArray
Review comment:
If there are `sortedColumn`, we need `numBuckets` in between of `columns` and `sortedColumns`, because we need a way to figure out which elements in the array are for columns, and which elements are for sortedColumns.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] cloud-fan commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r779588827
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTable.scala
##########
@@ -161,7 +162,11 @@ class InMemoryTable(
case (v, t) =>
throw new IllegalArgumentException(s"Match: unsupported argument(s) type - ($v, $t)")
}
- case BucketTransform(numBuckets, ref, _) =>
+ case BucketTransform(numBuckets, ref) =>
Review comment:
I think we can have a single `BucketTransform.unapply`, to match both `BucketTransform` and `SortedBucketTransform`, so that we can have a single case here and avoid duplicated code.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] cloud-fan closed pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Separate SortedBucketTransform from BucketTransform
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #34914:
URL: https://github.com/apache/spark/pull/34914
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #34914:
URL: https://github.com/apache/spark/pull/34914#issuecomment-995384030
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/146247/
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #34914:
URL: https://github.com/apache/spark/pull/34914#issuecomment-995379812
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/50721/
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] viirya commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r777746740
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -106,26 +106,31 @@ private[sql] final case class BucketTransform(
columns: Seq[NamedReference],
sortedColumns: Seq[NamedReference] = Seq.empty[NamedReference]) extends RewritableTransform {
- override val name: String = "bucket"
+ override val name: String = if (sortedColumns.nonEmpty) "sortedBucket" else "bucket"
override def references: Array[NamedReference] = {
arguments.collect { case named: NamedReference => named }
}
- override def arguments: Array[Expression] = numBuckets +: columns.toArray
-
- override def describe: String =
+ override def arguments: Array[Expression] = {
if (sortedColumns.nonEmpty) {
- s"bucket(${arguments.map(_.describe).mkString(", ")}," +
- s" ${sortedColumns.map(_.describe).mkString(", ")})"
+ (columns.toArray :+ numBuckets) ++ sortedColumns
} else {
- s"bucket(${arguments.map(_.describe).mkString(", ")})"
+ numBuckets +: columns.toArray
Review comment:
Shall we keep consistent order of `columns` and `numBuckets` for two cases in `arguments`?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] viirya commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r777748013
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/connector/expressions/TransformExtractorSuite.scala
##########
@@ -39,6 +40,7 @@ class TransformExtractorSuite extends SparkFunSuite {
override def describe: String = names.mkString(".")
}
+
Review comment:
unnecessary 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] huaxingao commented on pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Separate SortedBucketTransform from BucketTransform
Posted by GitBox <gi...@apache.org>.
huaxingao commented on pull request #34914:
URL: https://github.com/apache/spark/pull/34914#issuecomment-1012799610
Thanks!
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] SparkQA commented on pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #34914:
URL: https://github.com/apache/spark/pull/34914#issuecomment-995323921
**[Test build #146247 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/146247/testReport)** for PR 34914 at commit [`7188482`](https://github.com/apache/spark/commit/7188482b824b879746a8f0cf203f0e1bae6cd883).
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] cloud-fan commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r780009974
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -141,20 +135,47 @@ private[sql] object BucketTransform {
def unapply(transform: Transform): Option[(Int, NamedReference, NamedReference)] =
transform match {
- case NamedTransform("bucket", Seq(
- Lit(value: Int, IntegerType),
- Ref(partCols: Seq[String]),
- Ref(sortCols: Seq[String]))) =>
- Some((value, FieldReference(partCols), FieldReference(sortCols)))
- case NamedTransform("bucket", Seq(
- Lit(value: Int, IntegerType),
- Ref(partCols: Seq[String]))) =>
- Some((value, FieldReference(partCols), FieldReference(Seq.empty[String])))
+ case NamedTransform("sorted_bucket", arguments) =>
+ var index: Int = -1
+ var posOfLit: Int = -1
+ var numOfBucket: Int = -1
+ arguments.foreach {
+ case Lit(value: Int, IntegerType) =>
+ numOfBucket = value
+ index = index + 1
+ posOfLit = index
+ case _ => index = index + 1
+ }
+ Some(numOfBucket, FieldReference(arguments.take(posOfLit).map(_.describe)),
Review comment:
we know that the arguments of `bucket`/`sorted_bucket`are all `NamedReference`, how about `arguments.take(posOfLit).map(_.asInstanceOf[NamedReference])`?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] cloud-fan commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r780009168
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -112,26 +111,21 @@ private[sql] final case class BucketTransform(
override def arguments: Array[Expression] = numBuckets +: columns.toArray
- override def toString: String =
- if (sortedColumns.nonEmpty) {
- s"bucket(${arguments.map(_.describe).mkString(", ")}," +
- s" ${sortedColumns.map(_.describe).mkString(", ")})"
- } else {
- s"bucket(${arguments.map(_.describe).mkString(", ")})"
- }
+ override def describe: String = s"bucket(${arguments.map(_.describe).mkString(", ")})"
+
+ override def toString: String = describe
override def withReferences(newReferences: Seq[NamedReference]): Transform = {
this.copy(columns = newReferences)
}
}
private[sql] object BucketTransform {
- def unapply(expr: Expression): Option[(Int, FieldReference, FieldReference)] =
- expr match {
+ def unapply(expr: Expression): Option[(Int, FieldReference, FieldReference)] = expr match {
Review comment:
where do we use this `unapply`?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] viirya commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r780047246
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -112,26 +111,21 @@ private[sql] final case class BucketTransform(
override def arguments: Array[Expression] = numBuckets +: columns.toArray
- override def toString: String =
- if (sortedColumns.nonEmpty) {
- s"bucket(${arguments.map(_.describe).mkString(", ")}," +
- s" ${sortedColumns.map(_.describe).mkString(", ")})"
- } else {
- s"bucket(${arguments.map(_.describe).mkString(", ")})"
- }
+ override def describe: String = s"bucket(${arguments.map(_.describe).mkString(", ")})"
+
+ override def toString: String = describe
override def withReferences(newReferences: Seq[NamedReference]): Transform = {
this.copy(columns = newReferences)
}
}
private[sql] object BucketTransform {
- def unapply(expr: Expression): Option[(Int, FieldReference, FieldReference)] =
- expr match {
+ def unapply(expr: Expression): Option[(Int, FieldReference, FieldReference)] = expr match {
Review comment:
Could you add some comments on `unapply` (if it is really used) about what it returns?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] huaxingao commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
huaxingao commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r780854363
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -141,20 +135,47 @@ private[sql] object BucketTransform {
def unapply(transform: Transform): Option[(Int, NamedReference, NamedReference)] =
transform match {
- case NamedTransform("bucket", Seq(
- Lit(value: Int, IntegerType),
- Ref(partCols: Seq[String]),
- Ref(sortCols: Seq[String]))) =>
- Some((value, FieldReference(partCols), FieldReference(sortCols)))
- case NamedTransform("bucket", Seq(
- Lit(value: Int, IntegerType),
- Ref(partCols: Seq[String]))) =>
- Some((value, FieldReference(partCols), FieldReference(Seq.empty[String])))
+ case NamedTransform("sorted_bucket", arguments) =>
+ var index: Int = -1
+ var posOfLit: Int = -1
+ var numOfBucket: Int = -1
+ arguments.foreach {
+ case Lit(value: Int, IntegerType) =>
+ numOfBucket = value
+ index = index + 1
+ posOfLit = index
+ case _ => index = index + 1
+ }
+ Some(numOfBucket, FieldReference(arguments.take(posOfLit).map(_.describe)),
+ FieldReference(arguments.drop(posOfLit + 1).map(_.describe)))
+ case NamedTransform("bucket", Seq(Lit(value: Int, IntegerType), Ref(seq: Seq[String]))) =>
Review comment:
Seems somehow only a single column is supported in `BucketTransform`. Will fix this.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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
[GitHub] [spark] huaxingao commented on a change in pull request #34914: [SPARK-37627][SQL][FOLLOWUP] Add tests for sorted BucketTransform
Posted by GitBox <gi...@apache.org>.
huaxingao commented on a change in pull request #34914:
URL: https://github.com/apache/spark/pull/34914#discussion_r780854317
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/connector/expressions/expressions.scala
##########
@@ -112,26 +111,21 @@ private[sql] final case class BucketTransform(
override def arguments: Array[Expression] = numBuckets +: columns.toArray
- override def toString: String =
- if (sortedColumns.nonEmpty) {
- s"bucket(${arguments.map(_.describe).mkString(", ")}," +
- s" ${sortedColumns.map(_.describe).mkString(", ")})"
- } else {
- s"bucket(${arguments.map(_.describe).mkString(", ")})"
- }
+ override def describe: String = s"bucket(${arguments.map(_.describe).mkString(", ")})"
+
+ override def toString: String = describe
override def withReferences(newReferences: Seq[NamedReference]): Transform = {
this.copy(columns = newReferences)
}
}
private[sql] object BucketTransform {
- def unapply(expr: Expression): Option[(Int, FieldReference, FieldReference)] =
- expr match {
+ def unapply(expr: Expression): Option[(Int, FieldReference, FieldReference)] = expr match {
Review comment:
This was introduced in https://github.com/apache/spark/pull/30706 but doesn't seem to be used. I will remove for now.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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