Skip to content

[SPARK-40926][CONNECT] Refactor server side tests to only use DataFrame API #38406

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
amaliujia wants to merge 7 commits into from
Closed

[SPARK-40926][CONNECT] Refactor server side tests to only use DataFrame API #38406

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0e20ec9
[SPARK-40926][CONNECT] Refactor server side tests to only use DataFra…
amaliujia Oct 21, 2022
e0423dd
[SPARK-40926][CONNECT] Refactor server side tests to only use DataFra…
amaliujia Oct 27, 2022
e928f67
remove unnecessary change
amaliujia Oct 27, 2022
2359183
update
amaliujia Oct 27, 2022
e09b85a
update
amaliujia Oct 27, 2022
5e773d4
update
amaliujia Oct 27, 2022
4564218
update
amaliujia Oct 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions ...tor/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class SparkConnectPlanner(plan: proto.Relation, session: SparkSession) {
case proto.Relation.RelTypeCase.AGGREGATE => transformAggregate(rel.getAggregate)
case proto.Relation.RelTypeCase.SQL => transformSql(rel.getSql)
case proto.Relation.RelTypeCase.LOCAL_RELATION =>
transformLocalRelation(rel.getLocalRelation)
transformLocalRelation(rel.getLocalRelation, common)
Copy link
Contributor

@cloud-fan cloud-fan Oct 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is common? Every logical plan has an optional alias?

Copy link
Contributor Author

@amaliujia amaliujia Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is legacy design that I believe it thinks only relations have the optional alias.

Every logical plan could have an optional alias, in that case I prefer to move that alias out of the common to have its own message. This is because by that we can differentiate

.xx()
.xx().as("") // probably invalid but user can write down such API
.xx().as("alias_1")

I can also change this in this PR if you think this is a right time.

Copy link
Contributor Author

@amaliujia amaliujia Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sent a PR for this topic (to avoid complicate current refactoring PR too much): #38415

case proto.Relation.RelTypeCase.SAMPLE => transformSample(rel.getSample)
case proto.Relation.RelTypeCase.RELTYPE_NOT_SET =>
throw new IndexOutOfBoundsException("Expected Relation to be set, but is empty.")
Expand Down Expand Up @@ -125,9 +125,16 @@ class SparkConnectPlanner(plan: proto.Relation, session: SparkSession) {
}
}

private def transformLocalRelation(rel: proto.LocalRelation): LogicalPlan = {
private def transformLocalRelation(
rel: proto.LocalRelation,
common: Option[proto.RelationCommon]): LogicalPlan = {
val attributes = rel.getAttributesList.asScala.map(transformAttribute(_)).toSeq
new org.apache.spark.sql.catalyst.plans.logical.LocalRelation(attributes)
val relation = new org.apache.spark.sql.catalyst.plans.logical.LocalRelation(attributes)
if (common.nonEmpty && common.get.getAlias.nonEmpty) {
logical.SubqueryAlias(identifier = common.get.getAlias, child = relation)
} else {
relation
}
}

private def transformAttribute(exp: proto.Expression.QualifiedAttribute): Attribute = {
Expand Down
68 changes: 0 additions & 68 deletions ...ct/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectDeduplicateSuite.scala
View file
Open in desktop

This file was deleted.

8 changes: 3 additions & 5 deletions ...onnect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectPlannerSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,20 +22,18 @@ import scala.collection.JavaConverters._
import org.apache.spark.SparkFunSuite
import org.apache.spark.connect.proto
import org.apache.spark.connect.proto.Expression.UnresolvedStar
import org.apache.spark.sql.SparkSession
import org.apache.spark.sql.catalyst.expressions.AttributeReference
import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
import org.apache.spark.sql.test.SharedSparkSession

/**
* Testing trait for SparkConnect tests with some helper methods to make it easier to create new
* test cases.
*/
trait SparkConnectPlanTest {

def getSession(): SparkSession = None.orNull
trait SparkConnectPlanTest extends SharedSparkSession {

def transform(rel: proto.Relation): LogicalPlan = {
new SparkConnectPlanner(rel, getSession()).transform()
new SparkConnectPlanner(rel, spark).transform()
}

def readRel: proto.Relation =
Expand Down
Loading