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
Eric5553 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 1 commit
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
Prev Previous commit
remove one function in Expression
  • Loading branch information
@Eric5553
Eric5553 committed Apr 9, 2020
commit c7daaa6543a51dbd83d56b56030d0d0b4755cb56
15 changes: 8 additions & 7 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 @@ -266,19 +266,20 @@ abstract class Expression extends TreeNode[Expression] {
case single => single :: Nil
}

protected def flatArgumentStrings: Iterator[String] = flatArguments.map {
case e: Expression => e.argumentString
case arg => s"$arg"
}

// Marks this as final, Expression.verboseString should never be called, and thus shouldn't be
// overridden by concrete classes.
final override def verboseString(maxFields: Int): String = simpleString(maxFields)

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

override def toString: String = prettyName + truncatedString(
flatArgumentStrings.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.


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 + flatArgumentStrings.mkString(start, ", ", ")")
if (isDistinct) {
toString.patch(prettyName.size + 1, "distinct ", 0)
} else {
toString
}
}
}

Expand Down