feat: don't pivot when we don't need to #32904
Conversation
|
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment
|
|
/korbit-review |
There was a problem hiding this comment.
Pull Request Overview
This PR prevents an unnecessary pivot operation by ensuring that pivoting only occurs when an x-axis, at least one metric, and at least one column are present.
- Added a condition to check for the presence of columns before pivoting.
- Updated the inline comment to explain the additional condition.
Comments suppressed due to low confidence (1)
superset-frontend/packages/superset-ui-chart-controls/src/operators/pivotOperator.ts:41
- Consider adding tests to verify that the pivot operation is skipped when columns is undefined or an empty array.
if (xAxisLabel && metricLabels.length && columns?.length) {
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/packages/superset-ui-chart-controls/src/operators/pivotOperator.ts | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ❌ Issue Categories
Category Enabled Documentation ✅ Logging ✅ Error Handling ✅ Readability ✅ Design ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
Note
Korbit Pro is free for open source projects 🎉
Looking to add Korbit to your team? Get started with a free 2 week trial here
betodealmeida
force-pushed
the
dont-pivot-unnecessarily
branch
from
7d6355c to
4279d34
Compare
betodealmeida
force-pushed
the
dont-pivot-unnecessarily
branch
from
4279d34 to
e95cfbc
Compare
|
Still need more testing. |
|
Might be related to #32436. |
SUMMARY
Pivoting without dimensions is an unnecessary operation, and worse, it breaks sorting.
(This is a simpler solution than #27616.)
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
Note that when we have dimensions sorting the query doesn't make much sense. Instead, we can sort by the x-axis:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION