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 2022/10/16 21:42:08 UTC
[GitHub] [spark] amaliujia opened a new pull request, #38276: [SPARK-40812][CONNECT]Add Deduplicate to Connect proto and DSL.
amaliujia opened a new pull request, #38276:
URL: https://github.com/apache/spark/pull/38276
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This PR supports `Deduplicate` to Connect proto and DSL.
Note that `Deduplicate` can not be replaced by SQL's `SELECT DISTINCT col_list`. The difference is that `Deduplicate` allows to remove duplicated rows based on a set of columns but returns all the columns. SQL's `SELECT DISTINCT col_list`, instead, can only return the `col_list`.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
1. To improve proto API coverage.
2. `Deduplicate` blocks https://github.com/apache/spark/pull/38166 because we want support `Union(isAll=false)` but that will return `Union().Distinct()`. `Deduplicate` is needed to write test cases for `Union(isAll=false)`.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
UT
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r997479516
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectDeduplicateSuite.scala:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql.connect.planner
+
+import org.apache.spark.sql.{Dataset, Row, SparkSession}
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.sql.types.{IntegerType, StringType, StructField, StructType}
+
+/**
+ * [[SparkConnectPlanTestWithSparkSession]] contains a SparkSession for the connect planner.
+ *
+ * It is not recommended to use Catalyst DSL along with this trait because `SharedSparkSession`
+ * has also defined implicits over Catalyst LogicalPlan which will cause ambiguity with the
+ * implicits defined in Catalyst DSL.
+ */
+trait SparkConnectPlanTestWithSparkSession extends SharedSparkSession with SparkConnectPlanTest {
+ override def getSession(): SparkSession = spark
+}
+
+class SparkConnectDeduplicateSuite extends SparkConnectPlanTestWithSparkSession {
+ lazy val connectTestRelation = createLocalRelationProto(
+ Seq(AttributeReference("id", IntegerType)(),
+ AttributeReference("key", StringType)(),
+ AttributeReference("value", StringType)()))
+
+ lazy val sparkTestRelation = {
+ spark.createDataFrame(
+ java.util.List.of[Row](),
+ StructType(Seq(
+ StructField("id", IntegerType),
+ StructField("key", StringType),
+ StructField("value", StringType)
+ )))
+ }
+
+
+ test("Test basic deduplicate") {
+ val connectPlan = {
+ import org.apache.spark.sql.connect.dsl.plans._
+ Dataset.ofRows(spark, transform(connectTestRelation.distinct()))
+ }
+
+ val sparkPlan = sparkTestRelation.distinct()
+ comparePlans(connectPlan.queryExecution.analyzed, sparkPlan.queryExecution.analyzed, false)
+
+ val connectPlan2 = {
+ import org.apache.spark.sql.connect.dsl.plans._
+ Dataset.ofRows(spark, transform(connectTestRelation.deduplicate(Seq("key", "value"))))
+ }
Review Comment:
Yeah here is some context that people may not know:
Scala seems to not allow two implicit defined in the same scope even though there is no ambiguity. In this case, Scala choose to ignore one of the implementation. The workaround was to use sub-scope to limit the impact of an implicit.
See comment here for the context: https://github.com/apache/spark/blob/4201a5924b5bd12d2c89f31b6049272748c58728/connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectProtoSuite.scala#L42
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectDeduplicateSuite.scala:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql.connect.planner
+
+import org.apache.spark.sql.{Dataset, Row, SparkSession}
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.sql.types.{IntegerType, StringType, StructField, StructType}
+
+/**
+ * [[SparkConnectPlanTestWithSparkSession]] contains a SparkSession for the connect planner.
+ *
+ * It is not recommended to use Catalyst DSL along with this trait because `SharedSparkSession`
+ * has also defined implicits over Catalyst LogicalPlan which will cause ambiguity with the
+ * implicits defined in Catalyst DSL.
+ */
+trait SparkConnectPlanTestWithSparkSession extends SharedSparkSession with SparkConnectPlanTest {
+ override def getSession(): SparkSession = spark
+}
+
+class SparkConnectDeduplicateSuite extends SparkConnectPlanTestWithSparkSession {
+ lazy val connectTestRelation = createLocalRelationProto(
+ Seq(AttributeReference("id", IntegerType)(),
+ AttributeReference("key", StringType)(),
+ AttributeReference("value", StringType)()))
+
+ lazy val sparkTestRelation = {
+ spark.createDataFrame(
+ java.util.List.of[Row](),
+ StructType(Seq(
+ StructField("id", IntegerType),
+ StructField("key", StringType),
+ StructField("value", StringType)
+ )))
+ }
+
+
+ test("Test basic deduplicate") {
+ val connectPlan = {
+ import org.apache.spark.sql.connect.dsl.plans._
+ Dataset.ofRows(spark, transform(connectTestRelation.distinct()))
+ }
+
+ val sparkPlan = sparkTestRelation.distinct()
+ comparePlans(connectPlan.queryExecution.analyzed, sparkPlan.queryExecution.analyzed, false)
+
+ val connectPlan2 = {
+ import org.apache.spark.sql.connect.dsl.plans._
+ Dataset.ofRows(spark, transform(connectTestRelation.deduplicate(Seq("key", "value"))))
+ }
Review Comment:
Yeah here are some context that people may not know:
Scala seems to not allow two implicit defined in the same scope even though there is no ambiguity. In this case, Scala choose to ignore one of the implementation. The workaround was to use sub-scope to limit the impact of an implicit.
See comment here for the context: https://github.com/apache/spark/blob/4201a5924b5bd12d2c89f31b6049272748c58728/connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectProtoSuite.scala#L42
--
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 diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1001311757
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectDeduplicateSuite.scala:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql.connect.planner
+
+import org.apache.spark.sql.{Dataset, Row, SparkSession}
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.sql.types.{IntegerType, StringType, StructField, StructType}
+
+/**
+ * [[SparkConnectPlanTestWithSparkSession]] contains a SparkSession for the connect planner.
+ *
+ * It is not recommended to use Catalyst DSL along with this trait because `SharedSparkSession`
Review Comment:
We can do it in a followup.
--
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] grundprinzip commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
grundprinzip commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r997381115
##########
connector/connect/src/main/protobuf/spark/connect/relations.proto:
##########
@@ -163,6 +164,14 @@ message Sort {
}
}
+// Relation of type [[Deduplicate]] which have duplicate rows removed, could consider either only
+// the subset of columns or all the columns.
+message Deduplicate {
+ Relation input = 1;
+ repeated string deduplicate_columns = 2;
+ bool is_all = 3;
Review Comment:
So the question is if `is_all` is a nice convenience hack or strictly necessary, I'm ok with both :)
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectDeduplicateSuite.scala:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql.connect.planner
+
+import org.apache.spark.sql.{Dataset, Row, SparkSession}
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.sql.types.{IntegerType, StringType, StructField, StructType}
+
+/**
+ * [[SparkConnectPlanTestWithSparkSession]] contains a SparkSession for the connect planner.
+ *
+ * It is not recommended to use Catalyst DSL along with this trait because `SharedSparkSession`
+ * has also defined implicits over Catalyst LogicalPlan which will cause ambiguity with the
+ * implicits defined in Catalyst DSL.
+ */
+trait SparkConnectPlanTestWithSparkSession extends SharedSparkSession with SparkConnectPlanTest {
+ override def getSession(): SparkSession = spark
+}
+
+class SparkConnectDeduplicateSuite extends SparkConnectPlanTestWithSparkSession {
+ lazy val connectTestRelation = createLocalRelationProto(
+ Seq(AttributeReference("id", IntegerType)(),
+ AttributeReference("key", StringType)(),
+ AttributeReference("value", StringType)()))
+
+ lazy val sparkTestRelation = {
+ spark.createDataFrame(
+ java.util.List.of[Row](),
+ StructType(Seq(
+ StructField("id", IntegerType),
+ StructField("key", StringType),
+ StructField("value", StringType)
+ )))
+ }
+
+
+ test("Test basic deduplicate") {
+ val connectPlan = {
+ import org.apache.spark.sql.connect.dsl.plans._
+ Dataset.ofRows(spark, transform(connectTestRelation.distinct()))
+ }
+
+ val sparkPlan = sparkTestRelation.distinct()
+ comparePlans(connectPlan.queryExecution.analyzed, sparkPlan.queryExecution.analyzed, false)
+
+ val connectPlan2 = {
+ import org.apache.spark.sql.connect.dsl.plans._
+ Dataset.ofRows(spark, transform(connectTestRelation.deduplicate(Seq("key", "value"))))
+ }
Review Comment:
I think there was an issue here with the way that the two implicits of Spark and Spark Connect DSL are handled.
##########
connector/connect/src/main/protobuf/spark/connect/relations.proto:
##########
@@ -163,6 +164,14 @@ message Sort {
}
}
+// Relation of type [[Deduplicate]] which have duplicate rows removed, could consider either only
+// the subset of columns or all the columns.
+message Deduplicate {
+ Relation input = 1;
+ repeated string deduplicate_columns = 2;
+ bool is_all = 3;
Review Comment:
given the usage of `is_all` can't this be inferred by the list of columns? If they're identical is_all is true?
--
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] HyukjinKwon commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1002828551
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -91,6 +93,37 @@ class SparkConnectPlanner(plan: proto.Relation, session: SparkSession) {
transformRelation(rel.getInput))
}
+ private def transformDeduplicate(rel: proto.Deduplicate): LogicalPlan = {
+ if (!rel.hasInput) {
+ throw InvalidPlanInput("Deduplicate needs a plan input")
+ }
+ if (rel.getAllColumnsAsKeys && rel.getColumnNamesCount > 0) {
+ throw InvalidPlanInput("Cannot deduplicate on both all columns and a subset of columns")
+ }
+ if (!rel.getAllColumnsAsKeys && rel.getColumnNamesCount == 0) {
Review Comment:
Why do we need `getAllColumnsAsKeys`? Seems like we can just tell when the columns are not set.
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -91,6 +93,37 @@ class SparkConnectPlanner(plan: proto.Relation, session: SparkSession) {
transformRelation(rel.getInput))
}
+ private def transformDeduplicate(rel: proto.Deduplicate): LogicalPlan = {
+ if (!rel.hasInput) {
+ throw InvalidPlanInput("Deduplicate needs a plan input")
+ }
+ if (rel.getAllColumnsAsKeys && rel.getColumnNamesCount > 0) {
+ throw InvalidPlanInput("Cannot deduplicate on both all columns and a subset of columns")
+ }
+ if (!rel.getAllColumnsAsKeys && rel.getColumnNamesCount == 0) {
Review Comment:
and this is not matched with the logical plan.
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1002848070
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -91,6 +93,37 @@ class SparkConnectPlanner(plan: proto.Relation, session: SparkSession) {
transformRelation(rel.getInput))
}
+ private def transformDeduplicate(rel: proto.Deduplicate): LogicalPlan = {
+ if (!rel.hasInput) {
+ throw InvalidPlanInput("Deduplicate needs a plan input")
+ }
+ if (rel.getAllColumnsAsKeys && rel.getColumnNamesCount > 0) {
+ throw InvalidPlanInput("Cannot deduplicate on both all columns and a subset of columns")
+ }
+ if (!rel.getAllColumnsAsKeys && rel.getColumnNamesCount == 0) {
Review Comment:
> I mean, we don't need `rel.getAllColumnsAsKeys` condition because we can know that's the case when `rel.getColumnNamesCount == 0`.
I want to clarify this specifically:
This is one of the Connect proto API design principle: we need to differentiate if a field is not set explicitly, or put it in another way, every intention should be expressed explicitly. Ultimately, this is to avoid ambiguity on the API surface.
One example is Project. If we see a Project without anything in the project list, then how do we interpret that? Does the user want to indicate a `SELECT *`? Does the user actually generate an invalid plan. The problem now is there are two possibilities for a plan, and the worse part is, one possibility is a valid plan, another is not. This led us explicitly encode `SELECT *` into the proto https://github.com/apache/spark/pull/38023.
So one of the reasons that we have a bool flag here is to not use `rel.getColumnNamesCount == 0` to infer distinct on all columns which is to ambiguity.
This might not be great because a few more fields could bring another problem: what if the user set them all. In terms of ambiguity, this is not an issue: we know that is an invalid plan without second choice.
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r997483148
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectPlannerSuite.scala:
##########
@@ -31,8 +31,11 @@ import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
* test cases.
*/
trait SparkConnectPlanTest {
+
+ def getSession(): SparkSession = None.orNull
Review Comment:
This is relevant to the comment above: the `SharedSparkSession` and its base defines implicit which will cause ambiguity with Catalyst implicit. For current `SparkConnectProtoSuite`, we cannot let it inherit `SharedSparkSession`. Meanwhile for the testing purpose on this `Deduplicate` implementation, we need a session. This is why this PR does some refactoring on the testing suites to have a separation.
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r996574943
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectDeduplicateSuite.scala:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql.connect.planner
+
+import org.apache.spark.sql.{Dataset, Row, SparkSession}
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.sql.types.{IntegerType, StringType, StructField, StructType}
+
+/**
+ * [[SparkConnectPlanTestWithSparkSession]] contains a SparkSession for the connect planner.
+ *
+ * It's not recommended to use Catalyst DSL along with this trait.
Review Comment:
Sure, let me update in the comment for the reason and also avoid abbreviation.
--
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 diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1000455134
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectDeduplicateSuite.scala:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql.connect.planner
+
+import org.apache.spark.sql.{Dataset, Row, SparkSession}
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.sql.types.{IntegerType, StringType, StructField, StructType}
+
+/**
+ * [[SparkConnectPlanTestWithSparkSession]] contains a SparkSession for the connect planner.
+ *
+ * It is not recommended to use Catalyst DSL along with this trait because `SharedSparkSession`
Review Comment:
This makes me think that we probably shouldn't use catalyst DSL at all. The tests need Spark Connect planner, which needs `SparkSession`. Some tests happen to not invoke `SparkSession`, but it's a bit hacky to rely on this assumption. We should just compare plans produced by proto DSL and DataFrame APIs.
--
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 diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1000451568
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -91,6 +93,40 @@ class SparkConnectPlanner(plan: proto.Relation, session: SparkSession) {
transformRelation(rel.getInput))
}
+ private def transformDeduplicate(rel: proto.Deduplicate): LogicalPlan = {
+ if (!rel.hasInput) {
+ throw InvalidPlanInput("Deduplicate needs a plan input")
+ }
+ if (rel.getIsAll && rel.getColumnNamesCount > 0) {
+ throw InvalidPlanInput("Cannot deduplicate on both all columns and a subset of columns")
+ }
+ if (!rel.getIsAll && rel.getColumnNamesCount == 0) {
+ throw InvalidPlanInput(
+ "Deduplicate requires to either deduplicate on all columns or a subset of columns")
+ }
+ val queryExecution = new QueryExecution(session, transformRelation(rel.getInput))
+ val resolver = session.sessionState.analyzer.resolver
+ val allColumns = queryExecution.analyzed.output
+ if (rel.getIsAll) {
+ Deduplicate(allColumns, queryExecution.analyzed)
+ } else {
+ val toGroupColumnNames = rel.getColumnNamesList.asScala.toSeq
+ // TODO: Connect does not support streaming now. But once we do, SPARK-31990 will need
+ // `toSet.toSeq` for `toGroupColumnNames` because of the backward compatibility issue
+ // (the Streaming's state store depends on the `groupCols` order).
Review Comment:
I don't think spark connect will have backward compatibility issues. It's a new 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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1001022068
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectDeduplicateSuite.scala:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql.connect.planner
+
+import org.apache.spark.sql.{Dataset, Row, SparkSession}
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.sql.types.{IntegerType, StringType, StructField, StructType}
+
+/**
+ * [[SparkConnectPlanTestWithSparkSession]] contains a SparkSession for the connect planner.
+ *
+ * It is not recommended to use Catalyst DSL along with this trait because `SharedSparkSession`
Review Comment:
We decided to with Catalyst DSL in https://github.com/apache/spark/pull/37994. However that also caused the pain of does the small scope to avoid implicit conflicts.
Given that we need to need a session based test, migrate all such tests to the same place that uses session and also DataFrame API just makes sense. Also I believe in this approach we don't have implicit conflict anymore in the same scope.
How about after this PR, let me send a follow up PR for the testing refactoring?
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1001029678
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -91,6 +93,40 @@ class SparkConnectPlanner(plan: proto.Relation, session: SparkSession) {
transformRelation(rel.getInput))
}
+ private def transformDeduplicate(rel: proto.Deduplicate): LogicalPlan = {
+ if (!rel.hasInput) {
+ throw InvalidPlanInput("Deduplicate needs a plan input")
+ }
+ if (rel.getIsAll && rel.getColumnNamesCount > 0) {
+ throw InvalidPlanInput("Cannot deduplicate on both all columns and a subset of columns")
+ }
+ if (!rel.getIsAll && rel.getColumnNamesCount == 0) {
+ throw InvalidPlanInput(
+ "Deduplicate requires to either deduplicate on all columns or a subset of columns")
+ }
+ val queryExecution = new QueryExecution(session, transformRelation(rel.getInput))
+ val resolver = session.sessionState.analyzer.resolver
+ val allColumns = queryExecution.analyzed.output
+ if (rel.getIsAll) {
+ Deduplicate(allColumns, queryExecution.analyzed)
+ } else {
+ val toGroupColumnNames = rel.getColumnNamesList.asScala.toSeq
+ // TODO: Connect does not support streaming now. But once we do, SPARK-31990 will need
+ // `toSet.toSeq` for `toGroupColumnNames` because of the backward compatibility issue
+ // (the Streaming's state store depends on the `groupCols` order).
Review Comment:
I see. Removed this comment.
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/dsl/package.scala:
##########
@@ -215,6 +215,26 @@ package object dsl {
.build()
}
+ def deduplicate(colNames: Seq[String]): proto.Relation =
+ proto.Relation
+ .newBuilder()
+ .setDeduplicate(
+ proto.Deduplicate
+ .newBuilder()
+ .setInput(logicalPlan)
+ .addAllColumnNames(colNames.asJava))
+ .build()
+
+ def distinct(): proto.Relation =
+ proto.Relation
+ .newBuilder()
+ .setDeduplicate(
+ proto.Deduplicate
+ .newBuilder()
+ .setInput(logicalPlan)
+ .setIsAll(true))
Review Comment:
Updated
--
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] amaliujia commented on pull request #38276: [SPARK-40812][CONNECT]Add Deduplicate to Connect proto and DSL.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on PR #38276:
URL: https://github.com/apache/spark/pull/38276#issuecomment-1280063548
R: @cloud-fan
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r996636946
##########
connector/connect/src/main/protobuf/spark/connect/relations.proto:
##########
@@ -163,6 +164,14 @@ message Sort {
}
}
+// Relation of type [[Deduplicate]] which have duplicate rows removed, could consider either only
+// the subset of columns or all the columns.
+message Deduplicate {
+ Relation input = 1;
+ repeated string deduplicate_columns = 2;
+ bool isAll = 3;
Review Comment:
yes :) it's is_all
--
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] zhengruifeng commented on pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
zhengruifeng commented on PR #38276:
URL: https://github.com/apache/spark/pull/38276#issuecomment-1280701203
please rebase to enable the codegen check
--
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 diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r997170213
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -71,6 +73,42 @@ class SparkConnectPlanner(plan: proto.Relation, session: SparkSession) {
session.sessionState.sqlParser.parsePlan(sql.getQuery)
}
+ private def transformDeduplicate(rel: proto.Deduplicate): LogicalPlan = {
+ if (!rel.hasInput) {
+ throw InvalidPlanInput("Deduplicate needs a plan input")
+ }
+ if (rel.getIsAll && rel.getDeduplicateColumnsCount > 0) {
+ throw InvalidPlanInput("Cannot deduplicate on both all columns and a subset of columns")
+ }
+ if (!rel.getIsAll && rel.getDeduplicateColumnsCount == 0) {
+ throw InvalidPlanInput(
+ "Deduplicate requires to either deduplicate on all columns or a subset of columns")
+ }
+ val queryExecution = new QueryExecution(session, transformRelation(rel.getInput))
+ val resolver = session.sessionState.analyzer.resolver
+ val allColumns = queryExecution.analyzed.output
+ val toGroupColumnNames = {
+ if (rel.getIsAll) {
+ allColumns.map(t => t.name).toSeq
Review Comment:
We don't need to do name lookup. We can just create `Deduplicate(allColumns, queryExecution.analyzed)`
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r996619430
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectDeduplicateSuite.scala:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql.connect.planner
+
+import org.apache.spark.sql.{Dataset, Row, SparkSession}
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.sql.types.{IntegerType, StringType, StructField, StructType}
+
+/**
+ * [[SparkConnectPlanTestWithSparkSession]] contains a SparkSession for the connect planner.
+ *
+ * It's not recommended to use Catalyst DSL along with this trait.
Review Comment:
done!
--
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 #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #38276:
URL: https://github.com/apache/spark/pull/38276#issuecomment-1284779356
oh, conflicts again...
--
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 #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #38276:
URL: https://github.com/apache/spark/pull/38276#issuecomment-1283654428
can you fix the conflicts?
--
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 diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1002832296
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -91,6 +93,37 @@ class SparkConnectPlanner(plan: proto.Relation, session: SparkSession) {
transformRelation(rel.getInput))
}
+ private def transformDeduplicate(rel: proto.Deduplicate): LogicalPlan = {
+ if (!rel.hasInput) {
+ throw InvalidPlanInput("Deduplicate needs a plan input")
+ }
+ if (rel.getAllColumnsAsKeys && rel.getColumnNamesCount > 0) {
+ throw InvalidPlanInput("Cannot deduplicate on both all columns and a subset of columns")
+ }
+ if (!rel.getAllColumnsAsKeys && rel.getColumnNamesCount == 0) {
Review Comment:
The issue is, in Spark connect client, we only see column names, not expr IDs. If the DF has duplicated column names, then deduplicate by all columns can't work in Spark connect client.
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1001022068
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectDeduplicateSuite.scala:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql.connect.planner
+
+import org.apache.spark.sql.{Dataset, Row, SparkSession}
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.sql.types.{IntegerType, StringType, StructField, StructType}
+
+/**
+ * [[SparkConnectPlanTestWithSparkSession]] contains a SparkSession for the connect planner.
+ *
+ * It is not recommended to use Catalyst DSL along with this trait because `SharedSparkSession`
Review Comment:
We decided to with Catalyst DSL in https://github.com/apache/spark/pull/37994. However that also caused the pain of does the small scope to avoid implicit conflicts.
Given that we need a session based test, migrate all such tests to the same place that uses session and also DataFrame API just makes sense. Also I believe in this approach we don't have implicit conflict anymore in the same scope.
How about after this PR, let me send a follow up PR for the testing refactoring?
Or we can the refactoring happen (I prefer a separate PR) then I can rebase this one. Either way is fine to me.
--
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 #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
URL: https://github.com/apache/spark/pull/38276
--
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] zhengruifeng commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
zhengruifeng commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r996756205
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectDeduplicateSuite.scala:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql.connect.planner
+
+import org.apache.spark.sql.{Dataset, Row, SparkSession}
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.sql.types.{IntegerType, StringType, StructField, StructType}
+
+/**
+ * [[SparkConnectPlanTestWithSparkSession]] contains a SparkSession for the connect planner.
+ *
+ * It is not recommended to use Catalyst DSL along with this trait because `SharedSparkSession`
+ * has also defined implicits over Catalyst LogicalPlan which will cause ambiguity with the
+ * implicits defined in Catalyst DSL.
+ */
+trait SparkConnectPlanTestWithSparkSession extends SharedSparkSession with SparkConnectPlanTest {
+ override def getSession(): SparkSession = spark
+}
+
+class SparkConnectDeduplicateSuite extends SparkConnectPlanTestWithSparkSession {
+ lazy val connectTestRelation = createLocalRelationProto(
+ Seq(AttributeReference("id", IntegerType)(),
+ AttributeReference("key", StringType)(),
+ AttributeReference("value", StringType)()))
+
+ lazy val sparkTestRelation = {
+ spark.createDataFrame(
+ java.util.List.of[Row](),
+ StructType(Seq(
+ StructField("id", IntegerType),
+ StructField("key", StringType),
+ StructField("value", StringType)
+ )))
+ }
+
+
+ test("Test basic deduplicate") {
+ val connectPlan = {
+ import org.apache.spark.sql.connect.dsl.plans._
+ Dataset.ofRows(spark, transform(connectTestRelation.distinct()))
+ }
+
+ val sparkPlan = sparkTestRelation.distinct()
+ comparePlans(connectPlan.queryExecution.analyzed, sparkPlan.queryExecution.analyzed, false)
+
+ val connectPlan2 = {
+ import org.apache.spark.sql.connect.dsl.plans._
+ Dataset.ofRows(spark, transform(connectTestRelation.deduplicate(Seq("key", "value"))))
+ }
Review Comment:
```suggestion
import org.apache.spark.sql.connect.dsl.plans._
val connectPlan = Dataset.ofRows(spark, transform(connectTestRelation.distinct()))
val sparkPlan = sparkTestRelation.distinct()
comparePlans(connectPlan.queryExecution.analyzed, sparkPlan.queryExecution.analyzed, false)
val connectPlan2 = Dataset.ofRows(spark, transform(connectTestRelation.deduplicate(Seq("key", "value"))))
```
##########
connector/connect/src/main/protobuf/spark/connect/relations.proto:
##########
@@ -41,6 +41,7 @@ message Relation {
Aggregate aggregate = 9;
SQL sql = 10;
LocalRelation local_relation = 11;
+ Deduplicate deduplicate = 13;
Review Comment:
12?
```suggestion
Deduplicate deduplicate = 12;
```
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectPlannerSuite.scala:
##########
@@ -31,8 +31,11 @@ import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
* test cases.
*/
trait SparkConnectPlanTest {
+
+ def getSession(): SparkSession = None.orNull
Review Comment:
is it possible to reuse `SharedSparkSessionBase`?
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r997473877
##########
connector/connect/src/main/protobuf/spark/connect/relations.proto:
##########
@@ -163,6 +164,14 @@ message Sort {
}
}
+// Relation of type [[Deduplicate]] which have duplicate rows removed, could consider either only
+// the subset of columns or all the columns.
+message Deduplicate {
+ Relation input = 1;
+ repeated string deduplicate_columns = 2;
+ bool is_all = 3;
Review Comment:
It is a nice convenience that users do know want to know the previous schema of the LogicalPlan but just want to chain a `DISTINCT`. In this case users leave the backend to resolve it.
Well from another perspective if we want to match existing DataFrame API (which is `distinct()`) this becomes a necessary
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r997476071
##########
connector/connect/src/main/protobuf/spark/connect/relations.proto:
##########
@@ -41,6 +41,7 @@ message Relation {
Aggregate aggregate = 9;
SQL sql = 10;
LocalRelation local_relation = 11;
+ Deduplicate deduplicate = 13;
Review Comment:
12 is take by `Sample` :) https://github.com/apache/spark/pull/38227
This line will cause merge conflict. Whether which PR is in first, it takes 12 and the other takes 13.
##########
connector/connect/src/main/protobuf/spark/connect/relations.proto:
##########
@@ -41,6 +41,7 @@ message Relation {
Aggregate aggregate = 9;
SQL sql = 10;
LocalRelation local_relation = 11;
+ Deduplicate deduplicate = 13;
Review Comment:
12 is take by `Sample` tough that PR is not merged :) https://github.com/apache/spark/pull/38227
This line will cause merge conflict. Whether which PR is in first, it takes 12 and the other takes 13.
--
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] HyukjinKwon commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r996574650
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectDeduplicateSuite.scala:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql.connect.planner
+
+import org.apache.spark.sql.{Dataset, Row, SparkSession}
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.sql.types.{IntegerType, StringType, StructField, StructType}
+
+/**
+ * [[SparkConnectPlanTestWithSparkSession]] contains a SparkSession for the connect planner.
+ *
+ * It's not recommended to use Catalyst DSL along with this trait.
Review Comment:
Can we explain why? Also lets avoid abbreviation in the docs It's -> It is
--
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] amaliujia commented on pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on PR #38276:
URL: https://github.com/apache/spark/pull/38276#issuecomment-1284839078
@cloud-fan let me fix. It is very easy to have conflict on the auto generated python proto files. `python/pyspark/sql/connect/proto/relations_pb2.py`.
They are acting as "single point of failures".
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1002848070
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -91,6 +93,37 @@ class SparkConnectPlanner(plan: proto.Relation, session: SparkSession) {
transformRelation(rel.getInput))
}
+ private def transformDeduplicate(rel: proto.Deduplicate): LogicalPlan = {
+ if (!rel.hasInput) {
+ throw InvalidPlanInput("Deduplicate needs a plan input")
+ }
+ if (rel.getAllColumnsAsKeys && rel.getColumnNamesCount > 0) {
+ throw InvalidPlanInput("Cannot deduplicate on both all columns and a subset of columns")
+ }
+ if (!rel.getAllColumnsAsKeys && rel.getColumnNamesCount == 0) {
Review Comment:
> I mean, we don't need `rel.getAllColumnsAsKeys` condition because we can know that's the case when `rel.getColumnNamesCount == 0`.
I want to clarify this specifically:
This is one of the Connect proto API design principle: we need to differentiate if a field is set or not set explicitly, or put it in another way, every intention should be expressed explicitly. Ultimately, this is to avoid ambiguity on the API surface.
One example is Project. If we see a Project without anything in the project list, then how do we interpret that? Does the user want to indicate a `SELECT *`? Does the user actually generate an invalid plan. The problem now is there are two possibilities for a plan, and the worse part is, one possibility is a valid plan, another is not. This led us explicitly encode `SELECT *` into the proto https://github.com/apache/spark/pull/38023.
So one of the reasons that we have a bool flag here is to not use `rel.getColumnNamesCount == 0` to infer distinct on all columns which has caused ambiguity problem.
This might not be great because a few more fields could bring another problem: what if the user set them all. In terms of ambiguity, this is not an issue: we know that is an invalid plan without second choice.
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1002848070
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -91,6 +93,37 @@ class SparkConnectPlanner(plan: proto.Relation, session: SparkSession) {
transformRelation(rel.getInput))
}
+ private def transformDeduplicate(rel: proto.Deduplicate): LogicalPlan = {
+ if (!rel.hasInput) {
+ throw InvalidPlanInput("Deduplicate needs a plan input")
+ }
+ if (rel.getAllColumnsAsKeys && rel.getColumnNamesCount > 0) {
+ throw InvalidPlanInput("Cannot deduplicate on both all columns and a subset of columns")
+ }
+ if (!rel.getAllColumnsAsKeys && rel.getColumnNamesCount == 0) {
Review Comment:
> I mean, we don't need `rel.getAllColumnsAsKeys` condition because we can know that's the case when `rel.getColumnNamesCount == 0`.
I want to clarify this specifically:
This is one of the Connect proto API design principle: we need to differentiate if a field is not set explicitly, or put it in another way, every intention should be expressed explicitly. Ultimately, this is to avoid ambiguity on the API surface.
One example is Project. If we see a Project without anything in the project list, then how do we interpret that? Does the user want to indicate a `SELECT *`? Does the user actually generate an invalid plan. The problem now is there are two possibilities for a plan, and the worse part is, one possibility is a valid plan, another is not. This led us explicitly encode `SELECT *` into the proto https://github.com/apache/spark/pull/38023.
So one of the reasons that we have a bool flag here is to not use `rel.getColumnNamesCount == 0` to infer distinct on all columns which is to ambiguity.
--
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] HyukjinKwon commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r996633113
##########
connector/connect/src/main/protobuf/spark/connect/relations.proto:
##########
@@ -163,6 +164,14 @@ message Sort {
}
}
+// Relation of type [[Deduplicate]] which have duplicate rows removed, could consider either only
+// the subset of columns or all the columns.
+message Deduplicate {
+ Relation input = 1;
+ repeated string deduplicate_columns = 2;
+ bool isAll = 3;
Review Comment:
should it be isAll or is_all?
--
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] HyukjinKwon commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r996849228
##########
connector/connect/src/main/protobuf/spark/connect/relations.proto:
##########
@@ -163,6 +164,14 @@ message Sort {
}
}
+// Relation of type [[Deduplicate]] which have duplicate rows removed, could consider either only
+// the subset of columns or all the columns.
+message Deduplicate {
+ Relation input = 1;
+ repeated string deduplicate_columns = 2;
Review Comment:
no biggie but I would match this name to what we have in `Dataset.scala`. e.g., col_names
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r997473877
##########
connector/connect/src/main/protobuf/spark/connect/relations.proto:
##########
@@ -163,6 +164,14 @@ message Sort {
}
}
+// Relation of type [[Deduplicate]] which have duplicate rows removed, could consider either only
+// the subset of columns or all the columns.
+message Deduplicate {
+ Relation input = 1;
+ repeated string deduplicate_columns = 2;
+ bool is_all = 3;
Review Comment:
It is a nice convenience that users do not want to know the previous schema of the LogicalPlan but just want to chain a `DISTINCT`. In this case users leave the backend to resolve the complete schema of the previous operation. Though users can `val t = df.schema(); df.deduplicate(t)`
Well from another perspective if we want to match existing DataFrame API (which is `distinct()`) this becomes a necessary
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r997522129
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -71,6 +73,42 @@ class SparkConnectPlanner(plan: proto.Relation, session: SparkSession) {
session.sessionState.sqlParser.parsePlan(sql.getQuery)
}
+ private def transformDeduplicate(rel: proto.Deduplicate): LogicalPlan = {
+ if (!rel.hasInput) {
+ throw InvalidPlanInput("Deduplicate needs a plan input")
+ }
+ if (rel.getIsAll && rel.getDeduplicateColumnsCount > 0) {
+ throw InvalidPlanInput("Cannot deduplicate on both all columns and a subset of columns")
+ }
+ if (!rel.getIsAll && rel.getDeduplicateColumnsCount == 0) {
+ throw InvalidPlanInput(
+ "Deduplicate requires to either deduplicate on all columns or a subset of columns")
+ }
+ val queryExecution = new QueryExecution(session, transformRelation(rel.getInput))
+ val resolver = session.sessionState.analyzer.resolver
+ val allColumns = queryExecution.analyzed.output
+ val toGroupColumnNames = {
+ if (rel.getIsAll) {
+ allColumns.map(t => t.name).toSeq
Review Comment:
done.
--
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 diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1001312001
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala:
##########
@@ -528,6 +528,8 @@ package object dsl {
}
def deduplicate(colNames: Attribute*): LogicalPlan = Deduplicate(colNames, logicalPlan)
+
+ def distinct(): LogicalPlan = Deduplicate(logicalPlan.output, logicalPlan)
Review Comment:
seems this is not used?
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r997483148
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectPlannerSuite.scala:
##########
@@ -31,8 +31,11 @@ import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
* test cases.
*/
trait SparkConnectPlanTest {
+
+ def getSession(): SparkSession = None.orNull
Review Comment:
This is relevant to the comment above: the `SharedSparkSession` and its base defines implicit which will cause ambiguity with Catalyst implicit. For current `SparkConnectProtoSuite`, we cannot let it inherit `SharedSparkSession`. Meanwhile for the testing purpose on this `Deduplicate` implementation, we need a session. This is why this PR does some refactoring on the testing suites have a separation.
--
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 #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #38276:
URL: https://github.com/apache/spark/pull/38276#issuecomment-1286471467
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] HyukjinKwon commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1002838067
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -91,6 +93,37 @@ class SparkConnectPlanner(plan: proto.Relation, session: SparkSession) {
transformRelation(rel.getInput))
}
+ private def transformDeduplicate(rel: proto.Deduplicate): LogicalPlan = {
+ if (!rel.hasInput) {
+ throw InvalidPlanInput("Deduplicate needs a plan input")
+ }
+ if (rel.getAllColumnsAsKeys && rel.getColumnNamesCount > 0) {
+ throw InvalidPlanInput("Cannot deduplicate on both all columns and a subset of columns")
+ }
+ if (!rel.getAllColumnsAsKeys && rel.getColumnNamesCount == 0) {
Review Comment:
I mean, we don't need `rel.getAllColumnsAsKeys` condition because we can know that's the case when `rel.getColumnNamesCount == 0`.
--
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] HyukjinKwon commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1002839336
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -91,6 +93,37 @@ class SparkConnectPlanner(plan: proto.Relation, session: SparkSession) {
transformRelation(rel.getInput))
}
+ private def transformDeduplicate(rel: proto.Deduplicate): LogicalPlan = {
+ if (!rel.hasInput) {
+ throw InvalidPlanInput("Deduplicate needs a plan input")
+ }
+ if (rel.getAllColumnsAsKeys && rel.getColumnNamesCount > 0) {
+ throw InvalidPlanInput("Cannot deduplicate on both all columns and a subset of columns")
+ }
+ if (!rel.getAllColumnsAsKeys && rel.getColumnNamesCount == 0) {
Review Comment:
ohh, okay. when it has all duplicated columns
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1001022068
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectDeduplicateSuite.scala:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql.connect.planner
+
+import org.apache.spark.sql.{Dataset, Row, SparkSession}
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.sql.types.{IntegerType, StringType, StructField, StructType}
+
+/**
+ * [[SparkConnectPlanTestWithSparkSession]] contains a SparkSession for the connect planner.
+ *
+ * It is not recommended to use Catalyst DSL along with this trait because `SharedSparkSession`
Review Comment:
We decided to with Catalyst DSL in https://github.com/apache/spark/pull/37994. However that also caused the pain of does the small scope to avoid implicit conflicts.
Given that we need a session based test, migrate all such tests to the same place that uses session and also DataFrame API just makes sense. Also I believe in this approach we don't have implicit conflict anymore in the same scope.
How about after this PR, let me send a follow up PR for the testing refactoring?
Or we can the have refactoring happen (I prefer a separate PR) then I can rebase this one. Either way is fine to me.
--
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] amaliujia commented on pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on PR #38276:
URL: https://github.com/apache/spark/pull/38276#issuecomment-1286399701
I will monitor the build job. Seeing more flaky than usual.
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r997479516
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectDeduplicateSuite.scala:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql.connect.planner
+
+import org.apache.spark.sql.{Dataset, Row, SparkSession}
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.sql.types.{IntegerType, StringType, StructField, StructType}
+
+/**
+ * [[SparkConnectPlanTestWithSparkSession]] contains a SparkSession for the connect planner.
+ *
+ * It is not recommended to use Catalyst DSL along with this trait because `SharedSparkSession`
+ * has also defined implicits over Catalyst LogicalPlan which will cause ambiguity with the
+ * implicits defined in Catalyst DSL.
+ */
+trait SparkConnectPlanTestWithSparkSession extends SharedSparkSession with SparkConnectPlanTest {
+ override def getSession(): SparkSession = spark
+}
+
+class SparkConnectDeduplicateSuite extends SparkConnectPlanTestWithSparkSession {
+ lazy val connectTestRelation = createLocalRelationProto(
+ Seq(AttributeReference("id", IntegerType)(),
+ AttributeReference("key", StringType)(),
+ AttributeReference("value", StringType)()))
+
+ lazy val sparkTestRelation = {
+ spark.createDataFrame(
+ java.util.List.of[Row](),
+ StructType(Seq(
+ StructField("id", IntegerType),
+ StructField("key", StringType),
+ StructField("value", StringType)
+ )))
+ }
+
+
+ test("Test basic deduplicate") {
+ val connectPlan = {
+ import org.apache.spark.sql.connect.dsl.plans._
+ Dataset.ofRows(spark, transform(connectTestRelation.distinct()))
+ }
+
+ val sparkPlan = sparkTestRelation.distinct()
+ comparePlans(connectPlan.queryExecution.analyzed, sparkPlan.queryExecution.analyzed, false)
+
+ val connectPlan2 = {
+ import org.apache.spark.sql.connect.dsl.plans._
+ Dataset.ofRows(spark, transform(connectTestRelation.deduplicate(Seq("key", "value"))))
+ }
Review Comment:
Yeah here are some context that people may not know:
Scala seems to not allow two implicit defined in the same scope even though there is no ambiguity. In this case, Scala chooses to ignore one of the implementation. The workaround was to use sub-scope to limit one implicit (which is for connect) then its parent scope imports another implicit.
See comment here for the context: https://github.com/apache/spark/blob/4201a5924b5bd12d2c89f31b6049272748c58728/connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectProtoSuite.scala#L42
--
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 #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #38276:
URL: https://github.com/apache/spark/pull/38276#issuecomment-1280477946
Can one of the admins verify this patch?
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1001022068
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectDeduplicateSuite.scala:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql.connect.planner
+
+import org.apache.spark.sql.{Dataset, Row, SparkSession}
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.sql.types.{IntegerType, StringType, StructField, StructType}
+
+/**
+ * [[SparkConnectPlanTestWithSparkSession]] contains a SparkSession for the connect planner.
+ *
+ * It is not recommended to use Catalyst DSL along with this trait because `SharedSparkSession`
Review Comment:
We decided to with Catalyst DSL in https://github.com/apache/spark/pull/37994. However that also caused the pain of does the small scope to avoid implicit conflicts.
Given that we need to need a session based test, migrate all such tests to the same place that uses session and also DataFrame API just makes sense. Also I believe in this approach we don't have implicit conflict anymore in the same scope.
However about after this PR, let me send a follow up PR for the testing refactoring?
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1001316040
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala:
##########
@@ -528,6 +528,8 @@ package object dsl {
}
def deduplicate(colNames: Attribute*): LogicalPlan = Deduplicate(colNames, logicalPlan)
+
+ def distinct(): LogicalPlan = Deduplicate(logicalPlan.output, logicalPlan)
Review Comment:
oh yes because I had to use DataFrame API for testing this ime.
Removed.
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1001022068
##########
connector/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectDeduplicateSuite.scala:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql.connect.planner
+
+import org.apache.spark.sql.{Dataset, Row, SparkSession}
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.sql.types.{IntegerType, StringType, StructField, StructType}
+
+/**
+ * [[SparkConnectPlanTestWithSparkSession]] contains a SparkSession for the connect planner.
+ *
+ * It is not recommended to use Catalyst DSL along with this trait because `SharedSparkSession`
Review Comment:
We decided to with Catalyst DSL in https://github.com/apache/spark/pull/37994. However that also caused the pain of does the small scope to avoid implicit conflicts.
Given that we need to need a session based test, migrate all such tests to the same place that uses session and also DataFrame API just makes sense. Also I believe in this approach we don't have implicit conflict anymore in the same scope.
How about after this PR, let me send a follow up PR for the testing refactoring?
Or we can the refactoring happen (I prefer a separate PR) then I can rebase this one. Either way is fine to me.
--
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 diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1002833441
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -91,6 +93,37 @@ class SparkConnectPlanner(plan: proto.Relation, session: SparkSession) {
transformRelation(rel.getInput))
}
+ private def transformDeduplicate(rel: proto.Deduplicate): LogicalPlan = {
+ if (!rel.hasInput) {
+ throw InvalidPlanInput("Deduplicate needs a plan input")
+ }
+ if (rel.getAllColumnsAsKeys && rel.getColumnNamesCount > 0) {
+ throw InvalidPlanInput("Cannot deduplicate on both all columns and a subset of columns")
+ }
+ if (!rel.getAllColumnsAsKeys && rel.getColumnNamesCount == 0) {
Review Comment:
actually, column name is unknown either, as the input plan is unresolved.
--
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] amaliujia commented on a diff in pull request #38276: [SPARK-40812][CONNECT] Add Deduplicate to Connect proto and DSL
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38276:
URL: https://github.com/apache/spark/pull/38276#discussion_r1002848070
##########
connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -91,6 +93,37 @@ class SparkConnectPlanner(plan: proto.Relation, session: SparkSession) {
transformRelation(rel.getInput))
}
+ private def transformDeduplicate(rel: proto.Deduplicate): LogicalPlan = {
+ if (!rel.hasInput) {
+ throw InvalidPlanInput("Deduplicate needs a plan input")
+ }
+ if (rel.getAllColumnsAsKeys && rel.getColumnNamesCount > 0) {
+ throw InvalidPlanInput("Cannot deduplicate on both all columns and a subset of columns")
+ }
+ if (!rel.getAllColumnsAsKeys && rel.getColumnNamesCount == 0) {
Review Comment:
> I mean, we don't need `rel.getAllColumnsAsKeys` condition because we can know that's the case when `rel.getColumnNamesCount == 0`.
I want to clarify this specifically:
This is one of the Connect proto API design principle: we need to differentiate if a field is set or not set explicitly, or put it in another way, every intention should be expressed explicitly. Ultimately, this is to avoid ambiguity on the API surface.
One example is Project. If we see a Project without anything in the project list, then how do we interpret that? Does the user want to indicate a `SELECT *`? Does the user actually generate an invalid plan. The problem now is there are two possibilities for a plan, and the worse part is, one possibility is a valid plan, another is not. This led us explicitly encode `SELECT *` into the proto https://github.com/apache/spark/pull/38023.
So one of the reasons that we have a bool flag here is to not use `rel.getColumnNamesCount == 0` to infer distinct on all columns which is to ambiguity.
This might not be great because a few more fields could bring another problem: what if the user set them all. In terms of ambiguity, this is not an issue: we know that is an invalid plan without second choice.
--
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