[SPARK-27633][SQL] Remove redundant aliases in NestedColumnAliasing #24525

viirya · 2019-05-05T04:54:46Z

What changes were proposed in this pull request?

In NestedColumnAliasing rule, we create aliases for nested field access in project list. We considered that top level parent field and nested fields under it were both accessed. In the case, we don't create the aliases because they are redundant.

There is another case, where a nested parent field and nested fields under it were both accessed, which we don't consider now. We don't need to create aliases in this case too.

How was this patch tested?

Added test.

SparkQA · 2019-05-05T07:05:02Z

Test build #105128 has finished for PR 24525 at commit f24b1d5.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-05-05T10:07:59Z

Test build #105129 has finished for PR 24525 at commit 578663c.

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

viirya · 2019-05-06T01:52:58Z

cc @dongjoon-hyun @cloud-fan

SparkQA · 2019-05-06T04:50:52Z

Test build #105136 has finished for PR 24525 at commit a009d3e.

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

dongjoon-hyun · 2019-05-08T18:55:47Z

Retest this please.

SparkQA · 2019-05-08T21:59:36Z

Test build #105267 has finished for PR 24525 at commit a009d3e.

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

viirya · 2019-05-14T02:35:57Z

ping @dongjoon-hyun

dongjoon-hyun · 2019-05-14T16:23:28Z

@viirya . Actually, I've been investigating this PR multiple times from the first day. But I couldn't make me sure about this approach.

One big thing is nestedFields.forall(f => f == n || child.find(_.semanticEquals(f)).isEmpty). semanticEquals is not a robust choice in Struct related operations. semanticEquals erases many information and frequently it bites us at some corner cases.

I'm interested in this PR and testing this. Could you test this more seriously once more, too?

viirya · 2019-05-15T02:22:05Z

Thanks for review @dongjoon-hyun

I understood your concerns. Thanks for test, also. Can you share few cases that you think might be problematic, if you find any?

In case here, the intent is to see the child of a nested field accessor is already presented. Do you think it is more robust, if comparing them exactly equally, not semantically?

cloud-fan · 2019-05-21T06:40:02Z

...alyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/NestedColumnAliasingSuite.scala

+    val nestedRelation = LocalRelation('a.struct('b.struct('c.int,
+      'd.struct('f.int, 'g.int), 'e.int)))
+
+    val first = GetStructField('a, 0, Some("b"))


can we use expression DSL to write tests?

Seems only Add have DSL? Don't see DSL for GetStructField. I replaced Add.

Got it. We have getField.

SparkQA · 2019-05-21T17:12:43Z

Test build #105622 has finished for PR 24525 at commit 412401b.

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

SparkQA · 2019-05-22T04:53:32Z

Test build #105649 has finished for PR 24525 at commit 3fd4cc5.

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

viirya · 2019-08-08T05:28:37Z

@cloud-fan @dongjoon-hyun Please let me know if you have more comments or thoughts on this? thanks.

dongjoon-hyun · 2019-08-08T06:33:51Z

@viirya . I has been not sure about this PR due to the old reason, but I don't have a counter example you asked. Sorry about that. If the other committers think okay, I will not be against this PR. Thanks for working on nested column improvement, @viirya .

cc @cloud-fan , @dbtsai , @gatorsmile

SparkQA · 2019-08-08T07:05:01Z

Test build #108795 has finished for PR 24525 at commit 4ff4112.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2019-08-08T21:08:47Z

retest this please.

viirya · 2019-08-08T21:09:52Z

@dongjoon-hyun I see. Thanks for the comment. Do you more agree on this if changing semanticEquals to exact comparing? Can it resolve your concern?

dongjoon-hyun · 2019-08-08T21:24:25Z

It's been just my personal concern, @viirya . You may not need to revise your PR~ Let's wait and get some advice from the seniors.

SparkQA · 2019-08-09T00:54:01Z

Test build #108845 has finished for PR 24525 at commit 4ff4112.

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

viirya · 2019-08-15T23:34:37Z

cc @cloud-fan Do you have more thoughts on this change?

maropu · 2019-08-19T06:12:15Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/NestedColumnAliasing.scala

+          case a @ GetArrayStructFields(child, _, _, _, _) =>
+            nestedFields.forall(f => f == a || child.find(_.semanticEquals(f)).isEmpty)
+          case n @ GetStructField(child, _, _) =>
+            nestedFields.forall(f => f == n || child.find(_.semanticEquals(f)).isEmpty)


Can we merge the two entry into one? e.g.,

case e @ (_: GetStructField | _: GetArrayStructFields) => nestedFields.forall(f => f == e || e.children.head.find(_.semanticEquals(f)).isEmpty)

Also, it seems the added test below passes without f == n? If so, can you add a test for it?

@maropu Thanks. Will address two points.

I think f == a condition is not necessary now. It should be left here by previous commit.

Thanks for the check! Ur, one more comment: can we avoid the loop of length(nestedFields)^2? in: https://github.com/apache/spark/pull/24525/files#diff-43334bab9616cc53e8797b9afa9fc7aaR129

Sorry for reply late.

I thought about this. We can check for a nested field accessor in other accessors which have less ExtractValue. For example, we don't need to check if a.b is redundant by looking at a.b.c, a.b.c.d, etc.

However to do this, we first need to collect the info about number of ExtractValue. Is it worth?

Ur, I see. Yea, I think its ok as it is. thanks for the check.

SparkQA · 2019-08-20T03:36:42Z

Test build #109372 has finished for PR 24525 at commit 7a18a0a.

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

github-actions · 2020-03-02T00:13:03Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

viirya · 2020-03-02T00:25:08Z

retest this please

SparkQA · 2020-03-02T04:50:21Z

Test build #119144 has finished for PR 24525 at commit 7a18a0a.

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

HyukjinKwon · 2020-03-03T02:50:58Z

retest this please

viirya · 2020-03-03T03:31:05Z

@HyukjinKwon Thanks for reopening this. I think I need to sync it up with latest change.

SparkQA · 2020-03-03T04:38:28Z

Test build #119199 has finished for PR 24525 at commit 7a18a0a.

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

SparkQA · 2020-03-03T08:05:01Z

Test build #119206 has finished for PR 24525 at commit a027946.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-03-03T08:08:19Z

retest this please

viirya · 2020-03-03T08:10:18Z

retest this please

SparkQA · 2020-03-03T13:22:56Z

Test build #119213 has finished for PR 24525 at commit a027946.

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

viirya · 2020-03-04T01:55:18Z

ping @cloud-fan

HyukjinKwon · 2020-06-12T08:26:56Z

@viirya, can you rebase this please? I merged a couple of your PRs and seems it caused the conflicts.

viirya · 2020-06-13T00:58:43Z

@HyukjinKwon rebased, thanks.

SparkQA · 2020-06-13T07:05:03Z

Test build #123950 has finished for PR 24525 at commit 40521aa.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-06-13T07:22:10Z

retest this please

SparkQA · 2020-06-13T12:21:29Z

Test build #123974 has finished for PR 24525 at commit 40521aa.

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

...alyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/NestedColumnAliasingSuite.scala

SparkQA · 2020-06-14T20:27:14Z

Test build #124009 has finished for PR 24525 at commit fb1d3f3.

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

HyukjinKwon · 2020-06-15T02:01:43Z

Merged to master.

viirya force-pushed the SPARK-27633 branch from f24b1d5 to 578663c Compare May 5, 2019 07:10

Remove redundant aliases in NestedColumnAliasing.

a009d3e

viirya force-pushed the SPARK-27633 branch from 578663c to a009d3e Compare May 6, 2019 01:52

cloud-fan reviewed May 21, 2019

View reviewed changes

Use DSL.

412401b

Use DSL.

3fd4cc5

dongjoon-hyun added the SQL label Jun 14, 2019

Merge remote-tracking branch 'upstream/master' into SPARK-27633

4ff4112

maropu reviewed Aug 19, 2019

View reviewed changes

Address comment.

7a18a0a

github-actions bot added the Stale label Mar 2, 2020

github-actions bot closed this Mar 3, 2020

HyukjinKwon reopened this Mar 3, 2020

HyukjinKwon removed the Stale label Mar 3, 2020

viirya added 2 commits March 2, 2020 21:44

Merge remote-tracking branch 'upstream/master' into SPARK-27633

81ca2d6

Revise test case to sync up with latest change.

a027946

Merge remote-tracking branch 'upstream/master' into SPARK-27633

40521aa

HyukjinKwon reviewed Jun 14, 2020

View reviewed changes

...alyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/NestedColumnAliasingSuite.scala Outdated Show resolved Hide resolved

Add jira number.

fb1d3f3

HyukjinKwon approved these changes Jun 15, 2020

View reviewed changes

HyukjinKwon closed this in 8282bbf Jun 15, 2020

viirya deleted the SPARK-27633 branch December 27, 2023 18:23

[SPARK-27633][SQL] Remove redundant aliases in NestedColumnAliasing #24525

[SPARK-27633][SQL] Remove redundant aliases in NestedColumnAliasing #24525

Conversation

viirya commented May 5, 2019

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented May 5, 2019

SparkQA commented May 5, 2019

viirya commented May 6, 2019

SparkQA commented May 6, 2019

dongjoon-hyun commented May 8, 2019

SparkQA commented May 8, 2019

viirya commented May 14, 2019

dongjoon-hyun commented May 14, 2019 • edited

viirya commented May 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented May 21, 2019

SparkQA commented May 22, 2019

viirya commented Aug 8, 2019

dongjoon-hyun commented Aug 8, 2019

SparkQA commented Aug 8, 2019

viirya commented Aug 8, 2019

viirya commented Aug 8, 2019

dongjoon-hyun commented Aug 8, 2019 • edited

SparkQA commented Aug 9, 2019

viirya commented Aug 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Aug 20, 2019

github-actions bot commented Mar 2, 2020

viirya commented Mar 2, 2020

SparkQA commented Mar 2, 2020

HyukjinKwon commented Mar 3, 2020

viirya commented Mar 3, 2020

SparkQA commented Mar 3, 2020

SparkQA commented Mar 3, 2020

maropu commented Mar 3, 2020

viirya commented Mar 3, 2020

SparkQA commented Mar 3, 2020

viirya commented Mar 4, 2020

HyukjinKwon commented Jun 12, 2020

viirya commented Jun 13, 2020

SparkQA commented Jun 13, 2020

maropu commented Jun 13, 2020

SparkQA commented Jun 13, 2020

SparkQA commented Jun 14, 2020

HyukjinKwon commented Jun 15, 2020

dongjoon-hyun commented May 14, 2019 •

edited

viirya commented May 15, 2019 •

edited

dongjoon-hyun commented Aug 8, 2019 •

edited