[SPARK-10593] [SQL] fix resolve output of Generate

[davies]

The output of Generate should not be resolved as Reference.

[SparkQA]

Test build #42442 has finished for PR 8755 at commit dd11d2c.

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

[cloud-fan]

Should we do this for transformExpressionsUp, transformAllExpressions too?

[davies]

Good question, ResolveReferences only uses transformExpressionsUp, also Generate is the only special case that need it. So it's enough for now.

[davies]

To be safe, I will move this change into Generate.

[cloud-fan]

Hi @davies , I think the idea of only transform expressions that defined in expressions is not good enough. The key problem I see is we are using Attribute to represent the output name and type. However, the type info is already in generator.elementTypes and we only need names, i.e. use Seq[String] instead of Seq[Arrtibute] for generatorOutput.

What do you think?

[SparkQA]

Test build #42495 has finished for PR 8755 at commit 887474e.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[marmbrus]

Duplicating this code is kind of scary to me. I think I like the other fix better. Though ideally, I think that we would just fix the Analyzer to understand that generate is special.

[davies]

The reason I switched to in way is to isolate the change, because expressions.contains(e) could be expensive for wider tables (there are many columns). Should we improve the previous one (using lazy val and Set)?

[SparkQA]

Test build #1757 has finished for PR 8755 at commit 887474e.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #1764 has finished for PR 8755 at commit 887474e.

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

[davies]

@cloud-fan I tried to change the output to be Seq[String] but failed, because after resolve Generate, we still need to a Generate with Attribute.

[yhuai]

generator.elementTypes is used to stored the data types. We can create new attributes based on the name and data types. Actually, can we fix it by changing ResolveReferences (having a case to handle generate)?

[davies]

@yhuai @cloud-fan I had changed to use a special case for Generate in ResolveReferrence, please take another look, thanks!

[SparkQA]

Test build #42772 has finished for PR 8755 at commit 6b02a41.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #1782 has finished for PR 8755 at commit 6b02a41.

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

[cloud-fan]

This is a simple fix and it works. However, it still seems hacky to me and I'm thinking about introducing an UnresolvedGenerate. That will require more code changes and I'm not sure if we should do it. Maybe we should merge this one first.

[marmbrus]

I think this seems fine for now. Generate has always been a weird case since its one of the few places new attributes just appear in non-leaf nodes.

[yhuai]

It will be good to say that ResolveGenerate will create AttributeReferences for output. I will fix it when merging it.

[yhuai]

LGTM. Merging to branch 1.5 and master.