[SPARK-42084][SQL] Avoid leaking the qualified-access-only restriction #39596

cloud-fan · 2023-01-16T05:58:47Z

What changes were proposed in this pull request?

This is a better fix than #39077 and #38862

The special attribute metadata __qualified_access_only is very risky, as it breaks normal column resolution. The aforementioned 2 PRs remove the restriction in SubqueryAlias and Alias, but it's not good enough as we may forget to do the same thing for new logical plans/expressions in the future. It's also problematic if advanced users manipulate logical plans and expressions directly, when there is no SubqueryAlias and Alias to remove the restriction.

To be safe, we should only apply this restriction when resolving join hidden columns, which means the plan node right above Project(Join(using or natural join)). This PR simply removes the restriction when a column is resolved from a sequence of Attributes, or from star expansion, and also when adding the Project hidden columns to its output. This makes sure that the qualified-access-only restriction will not be leaked to normal column resolution, but only metadata column resolution.

Why are the changes needed?

To make the join hidden column feature more robust

Does this PR introduce any user-facing change?

No

How was this patch tested?

existing tests

cloud-fan · 2023-01-16T05:59:47Z

cc @viirya @gengliangwang

gengliangwang · 2023-01-17T23:27:31Z

@cloud-fan I feel the qualifiedAccessOnly attributes are still complicated. We have to check them here and there. Since it is for certain Joins only, shall we decouple it from the metadataOutput and have a new TreeNode method to infer these hidden lists? E.g. hiddenOutputFromJoin. In this way, we don't need to mark whether an attribute is qualifiedAccessOnly.

cloud-fan · 2023-01-18T02:54:09Z

You will need to re-implement a propagation framework, as not only the direct parent of the join can access the hidden columns. e.g. Sort -> Project -> Join, we can sort by the join hidden column

cloud-fan · 2023-01-18T07:02:58Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

+    import org.apache.spark.sql.catalyst.util._
+    !Utils.isTesting || (LogicalPlanIntegrity.checkIfExprIdsAreGloballyUnique(currentPlan) &&
+      (!LogicalPlanIntegrity.canGetOutputAttrs(currentPlan) ||
+        !currentPlan.output.exists(_.qualifiedAccessOnly)))


This makes sure that only metadata column resolution will apply the qualified-access-only restriction.

It may not be straightforward to read/understand what it means. Maybe put your comment in the code.

gengliangwang · 2023-01-18T07:37:01Z

You will need to re-implement a propagation framework, as not only the direct parent of the join can access the hidden columns. e.g. Sort -> Project -> Join, we can sort by the join hidden column

OK, let me think more on this recently. I feel that mixing the V2 metadata columns and the hidden join columns are hard to maintain. A bit of duplicate code for the propagation can make future development easier.

gengliangwang

This is the best fix for the hidden column issues so far.

cloud-fan · 2023-01-18T10:43:13Z

thanks for the review, merging to master/3.3!

This is a better fix than #39077 and #38862 The special attribute metadata `__qualified_access_only` is very risky, as it breaks normal column resolution. The aforementioned 2 PRs remove the restriction in `SubqueryAlias` and `Alias`, but it's not good enough as we may forget to do the same thing for new logical plans/expressions in the future. It's also problematic if advanced users manipulate logical plans and expressions directly, when there is no `SubqueryAlias` and `Alias` to remove the restriction. To be safe, we should only apply this restriction when resolving join hidden columns, which means the plan node right above `Project(Join(using or natural join))`. This PR simply removes the restriction when a column is resolved from a sequence of `Attributes`, or from star expansion, and also when adding the `Project` hidden columns to its output. This makes sure that the qualified-access-only restriction will not be leaked to normal column resolution, but only metadata column resolution. To make the join hidden column feature more robust No existing tests Closes #39596 from cloud-fan/join. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…mal columns ### What changes were proposed in this pull request? This is a followup of #39596 to fix more corner cases. It ignores the special column flag that requires qualified access for normal output attributes, as the flag should be effective only to metadata columns. ### Why are the changes needed? It's very hard to make sure that we don't leak the special column flag. Since the bug has been in the Spark release for a while, there may be tables created with CTAS and the table schema contains the special flag. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? new analysis test Closes #40961 from cloud-fan/col. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…mal columns This is a followup of #39596 to fix more corner cases. It ignores the special column flag that requires qualified access for normal output attributes, as the flag should be effective only to metadata columns. It's very hard to make sure that we don't leak the special column flag. Since the bug has been in the Spark release for a while, there may be tables created with CTAS and the table schema contains the special flag. No new analysis test Closes #40961 from cloud-fan/col. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 021f02e) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…mal columns ### What changes were proposed in this pull request? This is a followup of apache#39596 to fix more corner cases. It ignores the special column flag that requires qualified access for normal output attributes, as the flag should be effective only to metadata columns. ### Why are the changes needed? It's very hard to make sure that we don't leak the special column flag. Since the bug has been in the Spark release for a while, there may be tables created with CTAS and the table schema contains the special flag. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? new analysis test Closes apache#40961 from cloud-fan/col. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…mal columns This is a followup of apache#39596 to fix more corner cases. It ignores the special column flag that requires qualified access for normal output attributes, as the flag should be effective only to metadata columns. It's very hard to make sure that we don't leak the special column flag. Since the bug has been in the Spark release for a while, there may be tables created with CTAS and the table schema contains the special flag. No new analysis test Closes apache#40961 from cloud-fan/col. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 021f02e) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

github-actions bot added the SQL label Jan 16, 2023

cloud-fan force-pushed the join branch from 68382f7 to 2c326bf Compare January 16, 2023 05:59

cloud-fan force-pushed the join branch from 2c326bf to 644adeb Compare January 17, 2023 08:47

Avoid leaking the qualified-access-only restriction

b54c48e

cloud-fan force-pushed the join branch from 644adeb to b54c48e Compare January 17, 2023 15:23

fix scala 2.13

dab9b50

cloud-fan commented Jan 18, 2023

View reviewed changes

gengliangwang approved these changes Jan 18, 2023

View reviewed changes

viirya approved these changes Jan 18, 2023

View reviewed changes

cloud-fan closed this in 8fde8bd Jan 18, 2023

cloud-fan mentioned this pull request Apr 26, 2023

[SPARK-43293][SQL] __qualified_access_only should be ignored in normal columns #40961

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-42084][SQL] Avoid leaking the qualified-access-only restriction #39596

[SPARK-42084][SQL] Avoid leaking the qualified-access-only restriction #39596

cloud-fan commented Jan 16, 2023 •

edited

cloud-fan commented Jan 16, 2023

gengliangwang commented Jan 17, 2023

cloud-fan commented Jan 18, 2023 •

edited

cloud-fan Jan 18, 2023

viirya Jan 18, 2023

gengliangwang commented Jan 18, 2023

gengliangwang left a comment

cloud-fan commented Jan 18, 2023

[SPARK-42084][SQL] Avoid leaking the qualified-access-only restriction #39596

[SPARK-42084][SQL] Avoid leaking the qualified-access-only restriction #39596

Conversation

cloud-fan commented Jan 16, 2023 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

cloud-fan commented Jan 16, 2023

gengliangwang commented Jan 17, 2023

cloud-fan commented Jan 18, 2023 • edited

cloud-fan Jan 18, 2023

Choose a reason for hiding this comment

viirya Jan 18, 2023

Choose a reason for hiding this comment

gengliangwang commented Jan 18, 2023

gengliangwang left a comment

Choose a reason for hiding this comment

cloud-fan commented Jan 18, 2023

cloud-fan commented Jan 16, 2023 •

edited

cloud-fan commented Jan 18, 2023 •

edited