Skip to content

[SPARK-30940][SQL] Remove attributeId in auto-generated arguments when Explain SQL query #27685

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
wants to merge 4 commits into from
Closed

[SPARK-30940][SQL] Remove attributeId in auto-generated arguments when Explain SQL query #27685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19ddd4f
Only remote auto-generated attrId
Eric5553 Mar 2, 2020
62065ac
refine code to provide better atomicity
Eric5553 Mar 3, 2020
ea136d1
fix pattern match null
Eric5553 Mar 3, 2020
c7daaa6
remove one function in Expression
Eric5553 Mar 10, 2020
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
12 changes: 10 additions & 2 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,8 +272,16 @@ abstract class Expression extends TreeNode[Expression] {

override def simpleString(maxFields: Int): String = toString

override def toString: String = prettyName + truncatedString(
flatArguments.toSeq, "(", ", ", ")", SQLConf.get.maxToStringFields)
override def toString: String = {
val argumentStrings = flatArguments.map {
case e: Expression => e.argumentString
case arg => s"$arg"
}
prettyName + truncatedString(
argumentStrings.toSeq, "(", ", ", ")", SQLConf.get.maxToStringFields)
}

def argumentString: String = toString
Copy link
Contributor

@cloud-fan cloud-fan Mar 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

is it possible to only add one method? I'm worried about adding to many methods to the framework.

Copy link
Contributor Author

@Eric5553 Eric5553 Mar 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

IMO, the argumentString is needed because AttributeReference already overwrite toString, thus we need the new abstract string function to switch to non-exprid format. For flatArgumentStrings, it only have two callers. I refactored the toAggString of AggregateFunction, then we don't need to add the method flatArgumentStrings in Expression but just implement it within toString. See commit b74c500.


/**
* Returns SQL representation of this expression. For expressions extending [[NonSQLExpression]],
Expand Down
7 changes: 5 additions & 2 deletions ...alyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/interfaces.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,8 +234,11 @@ abstract class AggregateFunction extends Expression {

/** String representation used in explain plans. */
def toAggString(isDistinct: Boolean): String = {
val start = if (isDistinct) "(distinct " else "("
prettyName + flatArguments.mkString(start, ", ", ")")
if (isDistinct) {
toString.patch(prettyName.size + 1, "distinct ", 0)
} else {
toString
}
}
}

Expand Down
9 changes: 9 additions & 0 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/namedExpressions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,15 @@ case class AttributeReference(

override def toString: String = s"$name#${exprId.id}$typeSuffix$delaySuffix"

override def argumentString: String = {
val endingMsg = s"$typeSuffix$delaySuffix"
if (endingMsg.nonEmpty) {
s"${name}#$endingMsg"
} else {
name
}
}

// Since the expression id is not in the first constructor it is missing from the default
// tree string.
override def simpleString(maxFields: Int): String = {
Expand Down
Loading