-
Notifications
You must be signed in to change notification settings - Fork 126
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
Feat: flat pivot table #6727
Feat: flat pivot table #6727
Conversation
@AdityaHegde tagging you for review of URL and relevant proto changes, @ericpgreen2 for other files. |
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.
You also need to update dashboard.proto
and add proto conversions. Bookmarks and public urls still use the old proto.
web-common/src/features/dashboards/url-state/convertPresetToExploreState.ts
Outdated
Show resolved
Hide resolved
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.
Approving from URL params POV. PivotRowJoinType
is already in dashboard.proto so it is not in the diffs.
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.
Looks pretty good!
In general, I'm wary of many isFlat
conditionals that now permeate the pivot code. There's a lot of nested conditionals. As the pivot/table configuration grows, we should think about how we can avoid even more conditionals (though I don't have a concrete proposal at the moment).
@@ -516,25 +515,61 @@ export function getSortFilteredMeasureBody( | |||
return { sortFilteredMeasureBody, isMeasureSortAccessor, sortAccessor }; | |||
} | |||
|
|||
function getValuesForFlatTable( | |||
tableData: PivotDataRow[], | |||
rowDimensions: string[], |
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.
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.
This is because the way implementation is set up for Pivot tables. Column dimension internally are treated as values on which measures would be split and hence have a very different way of working. For flat table we need just a basic query so it made sense to use rowDimensions as a proxy for implementing it. I have added a comment for this in pivot-data-config
.
if (metricsExplorer.pivot.sorting.length) { | ||
const accessor = metricsExplorer.pivot.sorting[0].id; | ||
const anchorDimension = metricsExplorer.pivot.rows.dimension?.[0].id; | ||
if (accessor !== anchorDimension) { | ||
metricsExplorer.pivot.sorting = []; | ||
|
||
if (metricsExplorer.pivot.rowJoinType === "flat") { | ||
const validAccessors = value.map((d) => d.id); | ||
if (!validAccessors.includes(accessor)) { | ||
metricsExplorer.pivot.sorting = []; | ||
} | ||
} else { | ||
const anchorDimension = metricsExplorer.pivot.rows.dimension?.[0]?.id; | ||
if (accessor !== anchorDimension) { | ||
metricsExplorer.pivot.sorting = []; | ||
} | ||
} | ||
} |
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.
Note: there's a lot of nesting here
import type { PivotDataRow } from "./types"; | ||
|
||
// State props | ||
export let assembled: boolean; |
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.
What's this mean?
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.
There are multiple stages in a pivot construction. assembled: true
signifies that all the stages have been completed.
Have made the proto changes to rename FWith regards to nesting, the other option is to create a separate data store for flat table. Though, the implementation paths for both are very similar with flat table needed few steps so a lot of the conditionals are used to skip those steps. We can create a different data store for flat but it would mean duplicating a lot of existing nested table code. |
I discussed it with Claude-3.7-sonnet in Cursor, and I got this high-level recommendation: // Create specialized stores
function createFlatPivotDataStore(ctx, config) {
// Flat-specific implementation
}
function createNestedPivotDataStore(ctx, config) {
// Nested-specific implementation
}
// Facade that delegates to the appropriate store
export function createPivotDataStore(ctx, configStore) {
return derived(configStore, (config, set) => {
const storeFactory = config.isFlat
? createFlatPivotDataStore
: createNestedPivotDataStore;
return storeFactory(ctx, config).subscribe(set);
});
} For any of the common processing steps, we could of course use a common utility function. But I know this might be a largish refactor of what you have, so I will defer to you on what you think is best. |
web-common/src/features/dashboards/url-state/url-state-variations.spec.ts
Show resolved
Hide resolved
["news.google.com", "", "4.2k", "12.1k"], | ||
[], | ||
]; | ||
|
||
test.describe("pivot run through", () => { |
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.
Nit: Going forward, it'd be nice to break out the "run through" tests into smaller pieces. Then, when they fail, we can more easily see which sub-feature is the culprit. In this case, I could see a test dedicated to the Flat Table.
Addresses #6711
Remaining Items -
Checklist: