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

RN-175: Add order columns transform (p1. switch to managing columns explicitly in report-server) #4191

Merged
merged 17 commits into from
Oct 11, 2022

Conversation

rohan-bes
Copy link
Collaborator

@rohan-bes rohan-bes commented Sep 28, 2022

Issue RN-175:

Part 1 of 3.

We want to introduce an ability to order the columns of the data in the viz-builder. However, before we do this, we need to actually have an explicit order to the columns in the report-server in the first place.

There are other reasons it is advantageous to switch to tracking an explicit column order as well:

  • It opens opportunities for controlling how vizes are presented more easily
  • It allows us to introduce excel style columns range lookups (eg. sum(col1:col5))
  • It makes behaviour of transforms a bit more consistent

Here we are introducing a simple data-structure called a TransformTable. It manages the rows of the table as before, but also separately tracks an array of columns. It has a number of utility methods for manipulating the rows/columns atomically, so that the caller doesn't need to ensure they're kept in sync manually.

Tried to make this review as digestable as possible for the reviewer by breaking up changes to each transform step into its own commit, so going commit-by-commit might be the easiest approach.

Copy link
Contributor

@biaoli0 biaoli0 left a comment

Choose a reason for hiding this comment

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

@rohan-bes Woohoo! Really love this new TransformTable feature! Can't wait to see the power and flexibilities it brings to report-server!

Just got a few minor suggestions on the codes and unit tests.

@rohan-bes
Copy link
Collaborator Author

Sorry @billli0 I merged the other part PRs before noticing that you wanted to re-review this one 🤦

I addressed the issues you mentioned in these two commits of you wanna review them?

  • Make tests use strictEquals: 9278f74
  • Use simpler logic for keyBy aliases: 7c0ac43

@rohan-bes rohan-bes requested a review from biaoli0 October 9, 2022 23:25
@rohan-bes rohan-bes merged commit 3c376b0 into dev Oct 11, 2022
@rohan-bes rohan-bes deleted the rn-175-p1-transform-table branch October 11, 2022 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants