[SPARK-14191][SQL] Remove invalid Expand operator constraints

[viirya]

What changes were proposed in this pull request?

JIRA: https://issues.apache.org/jira/browse/SPARK-14191

Expand operator now uses its child plan's constraints as its valid constraints (i.e., the base of constraints). This is not correct because Expand will set its group by attributes to null values. So the nullability of these attributes should be true.

E.g., for an Expand operator like:

val input = LocalRelation('a.int, 'b.int, 'c.int).where('c.attr > 10 && 'a.attr < 5 && 'b.attr > 2)
Expand(
  Seq(
    Seq('c, Literal.create(null, StringType), 1),
    Seq('c, 'a, 2)),
  Seq('c, 'a, 'gid.int),
  Project(Seq('a, 'c), input))

The Project operator has the constraints IsNotNull('a), IsNotNull('b) and IsNotNull('c). But the Expand should not have IsNotNull('a) in its constraints.

This PR is the first step for this issue and remove invalid constraints of Expand operator.

How was this patch tested?

A test is added to ConstraintPropagationSuite.

[SparkQA]

Test build #54301 has finished for PR 11995 at commit 84821b4.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[marmbrus]

This parameter is not documented.

Also, this operator is kind of violating the style of logical operators. The goal here is for query nodes to be correct by construction (i.e. figure out things like constraints by inspecting themselves and their children).

It would be great if we could move constraint pruning logic into Expand.

[SparkQA]

Test build #54495 has finished for PR 11995 at commit 23d6b37.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[viirya]

@marmbrus I've updated this and tests passed. Please take a look. Thanks!

[marmbrus]

This still feels kind of hacky to me and I think it comes down to the fact that this operator is poorly designed. Since we use this operator for more than just grouping, it seems kind of odd to add a new parameter called groupByAttrs to it. Also, we still have the property that its not always going to be correct by construction. If a user of this class incorrectly sets the groupByAttrs everything will still work, but it will just lie about its constraints.

I think the root problem is that projections and output are defined separately in the constructor. Everywhere else in logical plans, you either have an AttributeReference or, if you are producing a new value, you have an Alias. When you follow this pattern, the constraint system just works.

However, in Expand we have logic that decides to replace a column with null (which should be a new AttributeReference), but instead we impersonate the original value.

Until we come up with a principled solution, maybe we should just set validConstraints to be empty?

[viirya]

Thanks for pointing the problem out. Agreed after re-thinking about it. As separating projections and output causes the problem. How about we get the output from projections?

As there are more than one projection, we can just get the output from the first projection and verify its consistency with other projections.

[marmbrus]

Yeah, I do think it would be good if it just took a Seq[Seq[NamedExpression]] (or at least I can't come up with anything better). I'd still consider breaking this into two PRs. Simple fix for now that just removes invalid constraints and a refactoring that add back in valid ones.

[viirya]

Ok. Let me remove the constraints first.

[SparkQA]

Test build #54601 has finished for PR 11995 at commit ab89e62.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[viirya]

ping @marmbrus

[marmbrus]

It is best to avoid spurious changes because it pollutes git blame. I can revert this while merging this time.

[viirya]

Ok. Got it.

[marmbrus]

Thanks, merging to master.

[marmbrus]

I'm also going to add a comment here to explain why this is empty.

[viirya]

Thanks.