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
[AUTHZ] Displays the columns involved in extracting the aggregation operator #4532
Conversation
Could you please take a look if you find time, thanks @yaooqinn @bowenliang123 Although the |
Make sense to me. Can we also add some tests with having/grouping cases? |
I suggest providing relevant tests for evaluation with/without extracting aggregation columns. |
As new tests for aggregation pass successfully without extracting columns from Aggregate, I prefer to keep the tests but not the extraction. |
Do you mean just add test coverage instead of changing the code? |
Yes, I think so. |
In fact, this PR is not a bug fix, existing logic can also extract agg columns, this is because Spark pushes agg columns down to the project operator, while kyuubi overrides the project operator. Adding this pr is just an optimization in case the agg column is not pushed down to project after the spark optimizer rule is changed |
Well noticed, it's good to add ut for extending coverage in aggregation with and without aggregation columns used in output projections. |
@@ -1645,6 +1645,26 @@ class HiveCatalogPrivilegeBuilderSuite extends PrivilegesBuilderSuite { | |||
val accessType = ranger.AccessType(po, operationType, isInput = false) | |||
assert(accessType === AccessType.CREATE) | |||
} | |||
|
|||
test("Displays the columns involved in extracting the aggregation operator") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we also test the having-clause
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KYUUBI #4532:
83eec61
to
28e6174
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov Report
@@ Coverage Diff @@
## master #4532 +/- ##
============================================
- Coverage 53.35% 53.32% -0.04%
Complexity 13 13
============================================
Files 571 571
Lines 31372 31376 +4
Branches 4224 4225 +1
============================================
- Hits 16738 16730 -8
- Misses 13057 13069 +12
Partials 1577 1577
... and 9 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
thanks all, merged to master/1.7 |
…e aggregation operator ### _Why are the changes needed?_ This PR aims to display the columns involved in extracting the aggregation operator. ### _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](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request Closes #4532 from Yikf/agg-authZ. Closes #4532 6a3468f [Yikf] Displays the columns involved in extracting the aggregation operator Authored-by: Yikf <yikaifei@apache.org> Signed-off-by: Yikf <yikaifei@apache.org> (cherry picked from commit 709899f) Signed-off-by: Yikf <yikaifei@apache.org>
Why are the changes needed?
This PR aims to display the columns involved in extracting the aggregation operator.
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