Skip to content

Conversation

dhruvisompura
Copy link
Contributor

@dhruvisompura dhruvisompura commented Aug 6, 2025

Addresses #8533 #8534 #8806

To view these changes, set "dataExplorer.summaryPanelEnhancements": true in your settings.json file.

This PR adds the dropdown for sorting the summary panel rows and switches the summary panel to use the search/sort backend implementation when the feature flag is on. Searching and sorting the summary panel rows should be mostly functional. The UI will still have some bugs (expanding/collapsing a specific column, and scrollbar management) but these changes can be resolved after we have the changes for pinning columns in the layout manager.

This PR also adds additional safety checks to make sure that changes behind the feature flag do not effect the behavior of the data explorer when the feature flag is off since much of the foundational logic will be changing. We will be maintaining two code paths until this feature is complete.

Release Notes

New Features

  • N/A

Bug Fixes

  • N/A

QA Notes

@:data-explorer @:web @:win @:duck-db

Copy link

github-actions bot commented Aug 6, 2025

E2E Tests 🚀
This PR will run tests tagged with: @:critical @:data-explorer @:web @:win @:duck-db

readme  valid tags

@dhruvisompura dhruvisompura marked this pull request as draft August 6, 2025 23:23
@dhruvisompura dhruvisompura force-pushed the feature/summary-panel-sort-dropdown branch 8 times, most recently from d673ed7 to 2fbb607 Compare August 13, 2025 22:13
@dhruvisompura dhruvisompura changed the title Add UI scaffolding for sort dropdown Add sorting to summary panel Aug 13, 2025
@dhruvisompura dhruvisompura force-pushed the feature/summary-panel-sort-dropdown branch 2 times, most recently from f01297a to 2be60a2 Compare August 18, 2025 20:01
* @param options.sortOption The sort option to apply to the search results, defaults to SearchSchemaSortOrder.Original.
* @returns A promise that resolves to an array of matching column indices.
*/
async searchSchema2(options: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rather than updating the existing searchSchema function, I've created a new one that is called when the feature flag is on. This is needed, because the returned result is a completely different format than the existing search.

Having this change in a separate function allows us to incrementally change the three different caching layers to use the new backend search/sort without breaking the data explorer when the feature flag is off.

searchSchema2 will become the default searchSchema function once we've got the data grid, filter bar, and summary bar caching layers updated to handle the backend search.

screenColumns: this.screenRows
});
showSummaryPanelEnhancements
? await this._tableSummaryCache.update2({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the feature flag is on, the summary panel calls the new update function which has been updated to use the new search schema call.

*/
async setSortOption(sortOption: SearchSchemaSortOrder): Promise<void> {
this._sortOption = sortOption;
await this.fetchData();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When sorting, we never invalidate the cache. This only happens when the search text is cleared.

* _displayPositionOrder[0] = 5 means the first row in the data grid should display data
* for the 6th column (index 5) from the original un-modified dataset.
*/
private _displayPositionOrder: number[] = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic is going to have to be moved into the layout manager layer to play well with the pinned columns feature and new backend search API.

Comment on lines +197 to +202
isColumnExpanded(displayIndex: number) {
const originalIndex = summaryPanelEnhancementsFeatureEnabled(this._configurationService)
? this._displayPositionOrder[displayIndex]
: displayIndex;

return originalIndex !== undefined ? this._expandedColumns.has(originalIndex) : false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Managing the collapsed/expanded state for columns is currently incomplete. There are changes that need to be done in the layout manager side, which can be done in a follow up after the pinned columns changes have landed.

@@ -215,64 +256,49 @@ export class TableSummaryCache extends Disposable {
// Destructure the update descriptor.
const {
invalidateCache,
searchText,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted the changes in update that were search related and instead moved into a new standalone function called udpate2.

*/
private trimCache(startColumnIndex: number, endColumnIndex: number) {
private trimCache(columnIndicesToKeep: number[]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To support trimming arbitrary columns from the cache, this function now takes an array of column indices we want to keep in the cache, and trims the rest. This allows the cache to only keep search results

@dhruvisompura dhruvisompura marked this pull request as ready for review August 18, 2025 21:27
Creating a new wrapper function in the data explorer client instance for searching that utilizes the backend for searching and sorting. The backend powered search is being implemented in an isolated way to avoid regressions in the data grid column filtering functionality which currently works.

As part of adding search and sort, the order the data
should be rendered in (with regards to pinned columns as well)
needs to be maintained separately from the cache.

The cache previously only contained the data we needed to render and was used as the source of truth on what to render
and the order to render it.

Now we utilze an array to determine what to render and in what order
@dhruvisompura dhruvisompura force-pushed the feature/summary-panel-sort-dropdown branch from abbafe4 to 1f92559 Compare August 19, 2025 21:35
Copy link
Contributor

@juliasilge juliasilge left a comment

Choose a reason for hiding this comment

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

As an initial version to iterate on moving forward, this is working great for me!

Screenshot 2025-08-19 at 6 01 38 PM

I do not observe any problems with the current "regular" behavior with the setting not opted in.

@dhruvisompura dhruvisompura merged commit eb8aea2 into main Aug 20, 2025
10 checks passed
@dhruvisompura dhruvisompura deleted the feature/summary-panel-sort-dropdown branch August 20, 2025 00:12
@github-actions github-actions bot locked and limited conversation to collaborators Aug 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants