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.

Sign up for GitHub

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

Jump to bottom

feat: add sorting to pivot query #11464

Merged
merged 8 commits into from
Sep 17, 2024
Merged

feat: add sorting to pivot query #11464

merged 8 commits into from
Sep 17, 2024

Conversation

rephus
Copy link
Collaborator

@rephus rephus commented Sep 9, 2024

Closes: #11358

Description:

Screencast.from.09-09-24.09.59.10.webm

Reviewer actions

  • I have manually tested the changes in the preview environment
  • I have reviewed the code
  • I understand that "request changes" will block this PR from merging

@owlas owlas requested a deployment to add-sorting-pivot-query - jaffle_db_pg_13 PR #11464 September 9, 2024 08:05 — with Render Abandoned
@owlas owlas deployed to add-sorting-pivot-query - headless-browser PR #11464 September 9, 2024 08:05 — with Render Active
@netlify Netlify
Copy link

netlify bot commented Sep 9, 2024 •
edited
Loading

✅ Deploy Preview for peaceful-bassi-cbf284 canceled.

Name Link
🔨 Latest commit 0da8107
🔍 Latest deploy log https://app.netlify.com/sites/peaceful-bassi-cbf284/deploys/66e98dbcb1871d0008584785

@rephus
Copy link
Collaborator Author

rephus commented Sep 9, 2024

This only covers the sorting in pivot query not the sql runner chart specific part . If I remove the other selects, this ccan be merged separately..

@rephus rephus marked this pull request as ready for review September 10, 2024 06:55
@owlas owlas deployed to add-sorting-pivot-query - headless-browser PR #11464 September 12, 2024 17:10 — with Render Active
@owlas owlas deployed to add-sorting-pivot-query - headless-browser PR #11464 September 16, 2024 08:28 — with Render Active
@joaoviana
Copy link
Contributor

Since the issue mentions work to Add getSortOptions for the SQLResultsRunner we can move forward with this PR without the frontend/... changes, what do you think?
Tested the sorting and seems like it's working great, but needs some refinement on the sorting when the user changes the x axis reference for example.

owlas
owlas reviewed Sep 16, 2024
View reviewed changes
packages/common/src/visualizations/types/index.ts Outdated
@@ -47,6 56,11 @@ export type VizPivotLayoutOptions = {
reference: string;
};

export type VizSortBy = {
reference: string;
type?: SortByType;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the purpose of having this type here? Can we derive this from thereference? i.e. if x.reference = sort.reference then x-sorted else y-sorted

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that is to know if we are sorting by axis or bar heights I was not sure if the reference was pointing to a different axis, in some cases this might be redundant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll remove it

packages/backend/src/services/ProjectService/ProjectService.ts
@@ -2087,6 2093,12 @@ export class ProjectService extends BaseService {
),
];

const orderBy: string = sortBy
Copy link
Collaborator

Choose a reason for hiding this comment

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

The orderBy also needs to be applied to line 2106 where we compute the row_index. This is only valid for a sort that matches the indexColumn reference. Any other sorts could be ignored in the pivot

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Before

Screencast.from.16-09-24.13.35.47.webm

After

Screencast.from.16-09-24.13.35.13.webm

@owlas owlas deployed to add-sorting-pivot-query - headless-browser PR #11464 September 16, 2024 11:28 — with Render Active
@owlas owlas temporarily deployed to add-sorting-pivot-query - lightdash PR #11464 September 16, 2024 11:28 — with Render Destroyed
@rephus rephus marked this pull request as draft September 16, 2024 11:28
@owlas owlas deployed to add-sorting-pivot-query - headless-browser PR #11464 September 16, 2024 11:37 — with Render Active
@rephus rephus marked this pull request as ready for review September 16, 2024 11:37
owlas
owlas reviewed Sep 17, 2024
View reviewed changes
packages/frontend/src/components/DataViz/config/CartesianChartFieldConfiguration.tsx Outdated
]}
onChange={(value) => {
if (value) {
const direction = value as SortByDirection;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@joaoviana how do we type mantine form fields?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this will work:

onChange={(value: SortByDirection) => {

because data is static

@owlas owlas deployed to add-sorting-pivot-query - headless-browser PR #11464 September 17, 2024 13:41 — with Render Active
@owlas owlas temporarily deployed to add-sorting-pivot-query - lightdash PR #11464 September 17, 2024 14:10 — with Render Destroyed
@owlas owlas temporarily deployed to add-sorting-pivot-query - headless-browser PR #11464 September 17, 2024 14:10 — with Render Destroyed
@rephus rephus merged commit e2218c9 into main Sep 17, 2024
42 of 52 checks passed
@rephus rephus deleted the add-sorting-pivot-query branch September 17, 2024 14:32
lightdash-bot pushed a commit that referenced this pull request Sep 17, 2024
@semantic-release-bot
chore(release): 0.1261.0
7f6c0c7 
# [0.1261.0](0.1260.1...0.1261.0) (2024-09-17)

### Features

* add sorting to pivot query ([#11464](#11464)) ([e2218c9](e2218c9))
* enable saving sql runner model ([#11578](#11578)) ([873d095](873d095))
@lightdash-bot
Copy link
Collaborator

🎉 This PR is included in version 0.1261.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

@owlas owlas owlas approved these changes

@joaoviana joaoviana Awaiting requested review from joaoviana

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add sorting to charts in the sql runner
4 participants
@rephus @joaoviana @lightdash-bot @owlas