Skip to content

Conversation

@bowenliang123
Copy link
Contributor

@bowenliang123 bowenliang123 commented Mar 30, 2023

Why are the changes needed?

To fix #4617.

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

@bowenliang123 bowenliang123 changed the title [KYUUBI #4617] [AUTHZ] [KYUUBI #4617] [AUTHZ] Collect results for filtered show objects ahead to prevent holding unserilzable spark plan Mar 30, 2023
@bowenliang123 bowenliang123 changed the title [KYUUBI #4617] [AUTHZ] Collect results for filtered show objects ahead to prevent holding unserilzable spark plan [KYUUBI #4617] [AUTHZ] Collect results for filtered show objects ahead to prevent holding unserializable spark plan Mar 30, 2023
@bowenliang123 bowenliang123 marked this pull request as ready for review March 30, 2023 05:44
@bowenliang123 bowenliang123 requested review from yaooqinn and zhaomin1423 and removed request for yaooqinn and zhaomin1423 March 30, 2023 05:46
@codecov-commenter
Copy link

codecov-commenter commented Mar 30, 2023

Codecov Report

Merging #4634 (24a668e) into master (4c2f1e6) will decrease coverage by 4.04%.
The diff coverage is 47.05%.

❗ Current head 24a668e differs from pull request most recent head fe00ef5. Consider uploading reports for the commit fe00ef5 to get more accurate results

@@             Coverage Diff              @@
##             master    #4634      +/-   ##
============================================
- Coverage     57.62%   53.58%   -4.04%     
  Complexity       13       13              
============================================
  Files           579      579              
  Lines         31901    31812      -89     
  Branches       4263     4253      -10     
============================================
- Hits          18383    17048    -1335     
- Misses        11759    13175    +1416     
+ Partials       1759     1589     -170     
Impacted Files Coverage Δ
...n/spark/authz/ranger/FilteredShowObjectsExec.scala 40.74% <46.15%> (-1.37%) ⬇️
...park/authz/ranger/FilterDataSourceV2Strategy.scala 55.55% <50.00%> (-1.59%) ⬇️

... and 48 files with indirect coverage changes

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

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.

LGTM

bowenliang123 added a commit that referenced this pull request Mar 31, 2023
…d to prevent holding unserializable spark plan

### _Why are the changes needed?_

To fix #4617.
- The reason for issue #4617 is that delegated SparkPlan is not serilizable when execution
- Collect results for filtered show objects ahead in FilterDataSourceV2Strategy to prevent holding the delegated plan

### _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

- [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #4634 from bowenliang123/4617-filter.

Closes #4617

fe00ef5 [liangbowen] rename results to result
65ce03a [liangbowen] fix 4617

Authored-by: liangbowen <[email protected]>
Signed-off-by: liangbowen <[email protected]>
(cherry picked from commit 92f191a)
Signed-off-by: liangbowen <[email protected]>
@bowenliang123 bowenliang123 deleted the 4617-filter branch March 31, 2023 05:50
@bowenliang123
Copy link
Contributor Author

Thanks, merged to master/1.7.1 .

@bowenliang123 bowenliang123 added this to the v1.7.1 milestone Mar 31, 2023
@bowenliang123 bowenliang123 self-assigned this Mar 31, 2023
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.

[Bug] [Authz]Task not serializable for show tables with limit

3 participants