Skip to content

Conversation

@AngersZhuuuu
Copy link
Contributor

@AngersZhuuuu AngersZhuuuu commented Oct 21, 2023

Why are the changes needed?

To fix #5492
When we use saveAsTable and write as a DataSource table, since CreateTableAsDataSource command's catalogTable was directly constructed by identifier and only will check when executing, so here authz will miss db information as below case
image

This fixes this issue by following the Spark's code
截屏2023-10-21 下午3 37 37

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

Was this patch authored or co-authored using generative AI tooling?

No

@AngersZhuuuu
Copy link
Contributor Author

ping @bowenliang123 @yaooqinn Could you take a look

Copy link
Contributor

@bowenliang123 bowenliang123 left a comment

Choose a reason for hiding this comment

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

LGTM.

@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2023

Codecov Report

Merging #5493 (46867af) into master (03d6223) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #5493   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files         588     588           
  Lines       33480   33480           
  Branches     4405    4405           
======================================
  Misses      33480   33480           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

val e = intercept[AccessControlException] {
doAs(someone, df.write.mode("overwrite").saveAsTable(table1))
}
assert(e.getMessage.contains(s"does not have [create] privilege on [$defaultDb/$table1]"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, you may use the interceptContains method to simplify the assertions. Very handy and clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, you may use the interceptContains method to simplify the assertions. Very handy and clear.

Done. I will refine these test's UT by interceptContains later

Copy link
Member

@yaooqinn yaooqinn left a comment

Choose a reason for hiding this comment

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

setCurrentDatabaseIfMissing shall be used

@AngersZhuuuu
Copy link
Contributor Author

setCurrentDatabaseIfMissing shall be used

Done

@pan3793 pan3793 requested a review from yaooqinn October 23, 2023 06:12
@yaooqinn yaooqinn added this to the v1.8.0 milestone Oct 23, 2023
@yaooqinn yaooqinn closed this in aae38a9 Oct 23, 2023
@yaooqinn
Copy link
Member

Thanks, merged to master and 1.8

@yaooqinn
Copy link
Member

Hi @AngersZhuuuu, it conflicts 1.8, can you send a backport PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[TASK][EASY] saveAsTable create DataSource table miss db and catalog

5 participants