Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TASK][EASY] LATERAL SQL miss check the whole plan #5503

Closed
3 of 4 tasks
Tracked by #5474
AngersZhuuuu opened this issue Oct 23, 2023 · 0 comments
Closed
3 of 4 tasks
Tracked by #5474

[TASK][EASY] LATERAL SQL miss check the whole plan #5503

AngersZhuuuu opened this issue Oct 23, 2023 · 0 comments
Assignees
Labels
kind:bug This is a clearly a bug priority:major

Comments

@AngersZhuuuu
Copy link
Contributor

AngersZhuuuu commented Oct 23, 2023

Code of Conduct

Search before asking

  • I have searched in the issues and found no similar issues.

Describe the bug

For lateral sql

test("xxx") {
    val db1 = defaultDb
    val table1 = "table1"
    val table2 = "table2"
    val view1 = "view1"
    withCleanTmpResources(
      Seq((s"$db1.$table1", "table"), (s"$db1.$table2", "table"), (s"$db1.$view1", "view"))) {
      doAs(admin, sql(s"CREATE TABLE IF NOT EXISTS $db1.$table1 (id int, scope int)"))
      doAs(admin, sql(s"CREATE TABLE IF NOT EXISTS $db1.$table2 (id int, age int)"))
      doAs(admin, sql(
        s"""
           |SELECT t1.id, age
           |FROM $db1.$table1 t1,
           |LATERAL (
           |  SELECT *
           |  FROM $db1.$table2 t2
           |  WHERE t1.id = t2.id
           |)
           |""".stripMargin).explain(true))
    }
  }

When it only check the subquery, miss check whole plan
截屏2023-10-23 下午6 37 12

Affects Version(s)

master, 1.8.0

Kyuubi Server Log Output

No response

Kyuubi Engine Log Output

No response

Kyuubi Server Configurations

No response

Kyuubi Engine Configurations

No response

Additional context

No response

Are you willing to submit PR?

  • Yes. I would be willing to submit a PR with guidance from the Kyuubi community to fix.
  • No. I cannot submit a PR at this time.
@AngersZhuuuu AngersZhuuuu added kind:bug This is a clearly a bug priority:major labels Oct 23, 2023
AngersZhuuuu added a commit to AngersZhuuuu/incubator-kyuubi that referenced this issue Oct 23, 2023
AngersZhuuuu added a commit to AngersZhuuuu/incubator-kyuubi that referenced this issue Oct 26, 2023
AngersZhuuuu added a commit to AngersZhuuuu/incubator-kyuubi that referenced this issue Oct 30, 2023
pan3793 pushed a commit that referenced this issue Oct 31, 2023
…e been verified

### _Why are the changes needed?_
To close #5503
For sql such as lateral join in test `[KYUUBI #5503][AUTHZ] Check plan auth checked should not set tag to all child nodes`, it will first verify subquery in `lateral` then verify whole plan, if there is a view, when verify the whole plan, the `PermanentViewMarker` will be remove by spark's optimizer.
Then it will verify both source table `table1` and `table2`.
So I think we need to do 3 things:

1. Mark all PermanentViewMarker's children's all nodes as checked and Subquery's all child marks as checked.
2. `isAuthChecked` should only check the first level of the plan to avoid skipping the check of the whole plan in the demo test
3. in `buildQuery`, if the current node has the tag, we just skip it.

Without this pr, the SQL in test will both check `table1` and `table2`

### _How was this patch tested?_
- [x] 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](https://kyuubi.readthedocs.io/en/master/contributing/code/testing.html#running-tests) locally before make a pull request

### _Was this patch authored or co-authored using generative AI tooling?_
No

Closes #5563 from AngersZhuuuu/KYUUBI-5503-FOLLOWUP.

Closes #5503

c1a427f [Angerszhuuuu] Update Authorization.scala
d6b2899 [Angerszhuuuu] update
633bc91 [Angerszhuuuu] Update Authorization.scala
7a006b1 [Angerszhuuuu] [KYUUBI #5503][FOLLOWUP][AUTHZ] Authz should skip inner plan that have been verified

Authored-by: Angerszhuuuu <angers.zhu@gmail.com>
Signed-off-by: Cheng Pan <chengpan@apache.org>
@pan3793 pan3793 changed the title [Bug] LATERAL SQL miss check the whole plan [TASK][EASY] LATERAL SQL miss check the whole plan Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment