Skip to content

[SPARK-34923][SQL] Metadata output should be empty for more plans #32017

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

[SPARK-34923][SQL] Metadata output should be empty for more plans #32017

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6b4cba3
Metadata output should be empty by default
karenfeng Mar 31, 2021
6c3dde2
Clean up
karenfeng Mar 31, 2021
ce3ac0e
Revert unintended change
karenfeng Apr 1, 2021
a2d72ef
Address comments and merge conflicts
karenfeng Apr 1, 2021
a4a7d05
By default, propagate metadata output
karenfeng Apr 5, 2021
5a04a7e
Use blocklist
karenfeng Apr 5, 2021
73219d4
Retrigger tests
karenfeng Apr 5, 2021
b1c0183
Retrigger tests
karenfeng Apr 6, 2021
e8e6e7d
Retrigger tests
karenfeng Apr 6, 2021
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
Next Next commit
Address comments and merge conflicts 
Signed-off-by: Karen Feng <[email protected]>
  • Loading branch information
@karenfeng
karenfeng committed Apr 1, 2021
commit a2d72ef544fcb3ea3e746a9b583e23ff8f49af9b
13 changes: 1 addition & 12 deletions ...st/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,6 @@ case class Intersect(
leftAttr.withNullability(leftAttr.nullable && rightAttr.nullable)
}

override def metadataOutput: Seq[Attribute] = children.flatMap(_.metadataOutput)

override protected lazy val validConstraints: ExpressionSet =
leftConstraints.union(rightConstraints)

Expand All @@ -212,7 +210,6 @@ case class Except(
override def nodeName: String = getClass.getSimpleName + ( if ( isAll ) "All" else "" )
/** We don't use right.output because those rows get excluded from the set. */
override def output: Seq[Attribute] = left.output
override def metadataOutput: Seq[Attribute] = left.metadataOutput

override protected lazy val validConstraints: ExpressionSet = leftConstraints
}
Expand Down Expand Up @@ -277,8 +274,6 @@ case class Union(
}
}

override def metadataOutput: Seq[Attribute] = children.flatMap(_.metadataOutput)

override lazy val resolved: Boolean = {
// allChildrenCompatible needs to be evaluated after childrenResolved
def allChildrenCompatible: Boolean =
Expand Down Expand Up @@ -486,8 +481,6 @@ case class View(

override def output: Seq[Attribute] = child.output

override def metadataOutput: Seq[Attribute] = child.output

override def simpleString(maxFields: Int): String = {
s"View (${desc.identifier}, ${output.mkString("[", ",", "]")})"
}
Expand Down Expand Up @@ -954,11 +947,6 @@ case class SubqueryAlias(
child.output.map(_.withQualifier(qualifierList))
}

override def metadataOutput: Seq[Attribute] = {
val qualifierList = identifier.qualifier :+ alias
child.metadataOutput.map(_.withQualifier(qualifierList))
}
Copy link
Contributor

@rdblue rdblue Apr 1, 2021

Choose a reason for hiding this comment

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

Why is the logic here removed? Won't this cause resolution failures when referencing a metadata column via an alias? Like SELECT s._file FROM (SELECT ...) s?

Copy link
Contributor Author

@karenfeng karenfeng Apr 1, 2021

Choose a reason for hiding this comment

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

@cloud-fan, should we support this case if it requires changing the query during analysis after being resolved?

Copy link
Contributor

@cloud-fan cloud-fan Apr 5, 2021

Choose a reason for hiding this comment

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

I think we should only expose the metadata column in a single SELECT group, e.g. SELECT _file FROM t, SELECT t1._file FROM t1 JOIN t2.

It's super weird if we can propagate the metadata column through SELECT groups, e.g. SELECT s._file FROM (SELECT a, b FROM t) s. The s is a subquery alias and the subquery has a clear output list which is a, b. It may surprise users if they can access s._file.

However, I do agree with @rdblue that simple alias should be supported. For example, SELECT t1._file FROM t t1 JOIN t t2. SELECT s._file FROM (SELECT ...) s won't work anyway because Project can't propagate the metadata columns.

That said, let's propagate metadata columns in SubqueryAlias


override def doCanonicalize(): LogicalPlan = child.canonicalized
}

Expand Down Expand Up @@ -1034,6 +1022,7 @@ abstract class RepartitionOperation extends UnaryNode {
override final def maxRows: Option[Long] = child.maxRows
override def output: Seq[Attribute] = child.output
override def metadataOutput: Seq[Attribute] = child.metadataOutput
def partitioning: Partitioning
}

/**
Expand Down
You are viewing a condensed version of this merge commit. You can view the full changes here.