Triage render issues with null counts, expanded profiles in data expl…

…orer (#2747) * Fix render issues with null counts, expanded profiles * Also send NotNull filter correctly
posit-dev · Apr 12, 2024 · 101a023 · 101a023
1 parent d3d99ae
commit 101a023
Show file tree

Hide file tree

Showing 9 changed files with 83 additions and 45 deletions.
diff --git a/...ositronDataExplorer/components/dataExplorerPanel/components/rowFilterBar/rowFilterBar.tsx b/...ositronDataExplorer/components/dataExplorerPanel/components/rowFilterBar/rowFilterBar.tsx
@@ -44,7 +44,7 @@ const createRowFilters = (rowFilterDescriptors: RowFilterDescriptor[]) => {
 		} else if (rowFilterDescriptor instanceof RowFilterDescriptorIsNotEmpty) {
 			rowFilters.push({
 				filter_id: rowFilterDescriptor.identifier,
-				filter_type: RowFilterType.IsNull,
+				filter_type: RowFilterType.NotNull,
 				column_index: rowFilterDescriptor.columnSchema.column_index
 			});
 		} else if (rowFilterDescriptor instanceof RowFilterDescriptorIsLessThan) {

diff --git a/src/vs/workbench/services/positronDataExplorer/browser/components/columnNullPercent.tsx b/src/vs/workbench/services/positronDataExplorer/browser/components/columnNullPercent.tsx
@@ -23,11 +23,26 @@ interface ColumnNullPercentProps {
  */
 export const ColumnNullPercent = (props: ColumnNullPercentProps) => {
 	// Render.
+	let svgWidth = 50;
+	if (props.columnNullPercent !== undefined) {
+		svgWidth = props.columnNullPercent === 0.0 ?
+			50 :
+			Math.max(50 * ((100 - props.columnNullPercent) / 100), 3);
+	}
 	return (
 		<div className='column-null-percent'>
-			<div className={positronClassNames('text-percent', { 'zero': props.columnNullPercent === 0.0 })}>
-				{props.columnNullPercent}%
-			</div>
+			{props.columnNullPercent !== undefined ?
+				(
+					<div className={positronClassNames('text-percent', { 'zero': props.columnNullPercent === 0.0 })}>
+						{props.columnNullPercent}%
+					</div>
+				) :
+				(
+					<div className={positronClassNames('text-percent')}>
+						...
+					</div>
+				)
+			}
 			<div className='graph-percent'>
 				<svg viewBox='0 0 52 14' shapeRendering='geometricPrecision'>
 					<g>
@@ -43,10 +58,7 @@ export const ColumnNullPercent = (props: ColumnNullPercentProps) => {
 						<rect className='indicator'
 							x='1'
 							y='1'
-							width={props.columnNullPercent === 0.0 ?
-								50 :
-								Math.max(50 * ((100 - props.columnNullPercent) / 100), 3)
-							}
+							width={svgWidth}
 							height='12'
 							rx='6'
 							ry='6'

diff --git a/src/vs/workbench/services/positronDataExplorer/browser/components/columnSummaryCell.tsx b/src/vs/workbench/services/positronDataExplorer/browser/components/columnSummaryCell.tsx
@@ -76,8 +76,6 @@ export const ColumnSummaryCell = (props: ColumnSummaryCellProps) => {
 	 * @returns The profile component.
 	 */
 	const profile = () => {
-		// Hack just to get things working
-		props.instance.computeColumnSummaryStats(props.columnIndex);
 		// Determine the alignment based on type.
 		switch (props.columnSchema.type_display) {
 			case ColumnDisplayType.Number:
@@ -90,7 +88,10 @@ export const ColumnSummaryCell = (props: ColumnSummaryCellProps) => {
 				return null;
 
 			case ColumnDisplayType.String:
-				return <ProfileString />;
+				return <ProfileString
+					instance={props.instance}
+					columnIndex={props.columnIndex}
+				/>;
 
 			case ColumnDisplayType.Date:
 				return null;

diff --git a/src/vs/workbench/services/positronDataExplorer/browser/components/profileNumber.tsx b/src/vs/workbench/services/positronDataExplorer/browser/components/profileNumber.tsx
@@ -24,8 +24,8 @@ interface ProfileNumberProps {
  * @returns The rendered component.
  */
 export const ProfileNumber = (props: ProfileNumberProps) => {
-	// Hack
 	let stats: any = props.instance.getColumnSummaryStats(props.columnIndex)?.number_stats!;
+	const nullCount = props.instance.getColumnNullCount(props.columnIndex);
 	if (!stats) {
 		stats = {};
 	}
@@ -41,7 +41,7 @@ export const ProfileNumber = (props: ProfileNumberProps) => {
 			</div>
 			<div className='values'>
 				<div className='values-left'>
-					<div className='value'>-999999</div>
+					<div className='value'>{nullCount}</div>
 					<div className='value'>{stats.mean}</div>
 					<div className='value'>{stats.median}</div>
 					<div className='value'>{stats.stdev}</div>

diff --git a/src/vs/workbench/services/positronDataExplorer/browser/components/profileString.tsx b/src/vs/workbench/services/positronDataExplorer/browser/components/profileString.tsx
@@ -7,11 +7,14 @@ import 'vs/css!./profileString';
 
 // React.
 import * as React from 'react';
+import { TableSummaryDataGridInstance } from 'vs/workbench/services/positronDataExplorer/browser/tableSummaryDataGridInstance';
 
 /**
  * ProfileStringProps interface.
  */
 interface ProfileStringProps {
+	instance: TableSummaryDataGridInstance;
+	columnIndex: number;
 }
 
 /**
@@ -20,6 +23,11 @@ interface ProfileStringProps {
  * @returns The rendered component.
  */
 export const ProfileString = (props: ProfileStringProps) => {
+	let stats: any = props.instance.getColumnSummaryStats(props.columnIndex)?.string_stats!;
+	const nullCount = props.instance.getColumnNullCount(props.columnIndex);
+	if (!stats) {
+		stats = {};
+	}
 	return (
 		<div className='tabular-info'>
 			<div className='labels'>
@@ -29,15 +37,15 @@ export const ProfileString = (props: ProfileStringProps) => {
 			</div>
 			<div className='values'>
 				<div className='values-left'>
-					<div className='value'>12</div>
-					<div className='value'>1</div>
-					<div className='value'>4</div>
+					<div className='value'>{nullCount}</div>
+					<div className='value'>{stats.num_empty}</div>
+					<div className='value'>{stats.num_unique}</div>
 				</div>
-				<div className='values-right'>
+				{/* <div className='values-right'>
 					<div className='value'>&nbsp;</div>
 					<div className='value'>.51</div>
 					<div className='value'>.20</div>
-				</div>
+				</div> */}
 			</div>
 		</div>
 	);

diff --git a/src/vs/workbench/services/positronDataExplorer/browser/positronDataExplorerInstance.ts b/src/vs/workbench/services/positronDataExplorer/browser/positronDataExplorerInstance.ts
@@ -9,6 +9,7 @@ import { DataExplorerClientInstance } from 'vs/workbench/services/languageRuntim
 import { TableSummaryDataGridInstance } from 'vs/workbench/services/positronDataExplorer/browser/tableSummaryDataGridInstance';
 import { PositronDataExplorerLayout } from 'vs/workbench/services/positronDataExplorer/browser/interfaces/positronDataExplorerService';
 import { IPositronDataExplorerInstance } from 'vs/workbench/services/positronDataExplorer/browser/interfaces/positronDataExplorerInstance';
+import { DataExplorerCache } from 'vs/workbench/services/positronDataExplorer/common/dataExplorerCache';
 
 /**
  * PositronDataExplorerInstance class.
@@ -21,6 +22,8 @@ export class PositronDataExplorerInstance extends Disposable implements IPositro
 	 */
 	private readonly _dataExplorerClientInstance: DataExplorerClientInstance;
 
+	private readonly _dataCache: DataExplorerCache;
+
 	/**
 	 * Gets or sets the layout.
 	 */
@@ -72,11 +75,14 @@ export class PositronDataExplorerInstance extends Disposable implements IPositro
 
 		// Initialize.
 		this._dataExplorerClientInstance = dataExplorerClientInstance;
+		this._dataCache = new DataExplorerCache(dataExplorerClientInstance);
 		this._tableSchemaDataGridInstance = new TableSummaryDataGridInstance(
-			dataExplorerClientInstance
+			dataExplorerClientInstance,
+			this._dataCache
 		);
 		this._tableDataDataGridInstance = new TableDataDataGridInstance(
-			dataExplorerClientInstance
+			dataExplorerClientInstance,
+			this._dataCache
 		);
 
 		// Add event handlers.

diff --git a/src/vs/workbench/services/positronDataExplorer/browser/tableDataDataGridInstance.tsx b/src/vs/workbench/services/positronDataExplorer/browser/tableDataDataGridInstance.tsx
@@ -39,7 +39,9 @@ export class TableDataDataGridInstance extends DataGridInstance {
 	 * Constructor.
 	 * @param dataExplorerClientInstance The DataExplorerClientInstance.
 	 */
-	constructor(dataExplorerClientInstance: DataExplorerClientInstance) {
+	constructor(dataExplorerClientInstance: DataExplorerClientInstance,
+		dataCache: DataExplorerCache
+	) {
 		// Call the base class's constructor.
 		super({
 			columnHeaders: true,
@@ -64,8 +66,8 @@ export class TableDataDataGridInstance extends DataGridInstance {
 		// Setup the data explorer client instance.
 		this._dataExplorerClientInstance = dataExplorerClientInstance;
 
-		// Allocate and initialize the DataExplorerCache.
-		this._dataExplorerCache = new DataExplorerCache(dataExplorerClientInstance);
+		// Set the shared data cache
+		this._dataExplorerCache = dataCache;
 
 		// Add the onDidUpdateCache event handler.
 		this._register(this._dataExplorerCache.onDidUpdateCache(() =>

diff --git a/src/vs/workbench/services/positronDataExplorer/browser/tableSummaryDataGridInstance.tsx b/src/vs/workbench/services/positronDataExplorer/browser/tableSummaryDataGridInstance.tsx
@@ -53,7 +53,9 @@ export class TableSummaryDataGridInstance extends DataGridInstance {
 	 * Constructor.
 	 * @param dataExplorerClientInstance The DataExplorerClientInstance.
 	 */
-	constructor(dataExplorerClientInstance: DataExplorerClientInstance) {
+	constructor(dataExplorerClientInstance: DataExplorerClientInstance,
+		dataCache: DataExplorerCache
+	) {
 		// Call the base class's constructor.
 		super({
 			columnHeaders: false,
@@ -74,19 +76,23 @@ export class TableSummaryDataGridInstance extends DataGridInstance {
 		// Set the data explorer client instance.
 		this._dataExplorerClientInstance = dataExplorerClientInstance;
 
-		// Allocate and initialize the DataExplorerCache.
-		this._dataExplorerCache = new DataExplorerCache(dataExplorerClientInstance);
+		// Set the shared data cache
+		this._dataExplorerCache = dataCache;
 
 		// Add the onDidUpdateCache event handler.
-		this._register(this._dataExplorerCache.onDidUpdateCache(() =>
-			this._onDidUpdateEmitter.fire()
-		));
-
+		this._register(this._dataExplorerCache.onDidUpdateCache(() => {
+			this._onDidUpdateEmitter.fire();
+			this._dataExplorerCache.cacheColumnSummaryStats([...this._expandedColumns]).then(
+				// Asynchronously update the summary stats for expanded columns then re-render
+				() => this._onDidUpdateEmitter.fire()
+			);
+		}));
 		// Add the onDidSchemaUpdate event handler.
 		this._register(this._dataExplorerClientInstance.onDidSchemaUpdate(async () => {
 			this.setScreenPosition(0, 0);
 			this._expandedColumns.clear();
 			this.fetchData();
+
 		}));
 	}
 
@@ -223,6 +229,10 @@ export class TableSummaryDataGridInstance extends DataGridInstance {
 		);
 	}
 
+	getColumnNullCount(columnIndex: number): number | undefined {
+		return this._dataExplorerCache.getColumnNullCount(columnIndex);
+	}
+
 	getColumnNullPercent(columnIndex: number): number | undefined {
 		const nullCount = this._dataExplorerCache.getColumnNullCount(columnIndex);
 		// TODO: Is floor what we want?
@@ -234,10 +244,6 @@ export class TableSummaryDataGridInstance extends DataGridInstance {
 		return this._dataExplorerCache.getColumnSummaryStats(columnIndex);
 	}
 
-	computeColumnSummaryStats(columnIndex: number) {
-		this._dataExplorerCache.updateColumnSummaryStats([columnIndex]);
-	}
-
 	//#endregion DataGridInstance Methods
 
 	//#region Public Events
@@ -265,12 +271,17 @@ export class TableSummaryDataGridInstance extends DataGridInstance {
 	 * @param columnIndex The columm index.
 	 */
 	toggleExpandColumn(columnIndex: number) {
-		// Tottle expand column.
+		// Toggle expand column.
 		if (this._expandedColumns.has(columnIndex)) {
 			this._expandedColumns.delete(columnIndex);
 		} else {
 			this._expandedColumns.add(columnIndex);
 			this.scrollToRow(columnIndex);
+
+			this._dataExplorerCache.cacheColumnSummaryStats([columnIndex]).then(() => {
+				// Re-render when the column summary stats return
+				this._onDidUpdateEmitter.fire();
+			});
 		}
 
 		// Fire the onDidUpdate event.

diff --git a/src/vs/workbench/services/positronDataExplorer/common/dataExplorerCache.ts b/src/vs/workbench/services/positronDataExplorer/common/dataExplorerCache.ts
@@ -111,19 +111,12 @@ export class DataExplorerCache extends Disposable {
 		this._register(this._dataExplorerClientInstance.onDidSchemaUpdate(async () => {
 			// Clear the column schema cache, row label cache, and data cell cache.
 			this._columnSchemaCache.clear();
-			this._columnNullCountCache.clear();
-			this._columnSummaryStatsCache.clear();
-			this._rowLabelCache.clear();
-			this._dataCellCache.clear();
+			this.invalidateDataCache();
 		}));
 
 		// Add the onDidDataUpdate event handler.
 		this._register(this._dataExplorerClientInstance.onDidDataUpdate(async () => {
-			// Clear the row label cache and data cell cache.
-			this._rowLabelCache.clear();
-			this._dataCellCache.clear();
-			this._columnNullCountCache.clear();
-			this._columnSummaryStatsCache.clear();
+			this.invalidateDataCache();
 		}));
 	}
 
@@ -207,7 +200,12 @@ export class DataExplorerCache extends Disposable {
 		return this._columnSummaryStatsCache.get(columnIndex);
 	}
 
-	async updateColumnSummaryStats(columnIndices: Array<number>) {
+	async cacheColumnSummaryStats(columnIndices: Array<number>) {
+		// Filter out summary stats that are already cached
+		columnIndices = columnIndices.filter(columnIndex =>
+			!this._columnSummaryStatsCache.has(columnIndex)
+		);
+
 		// Request the profiles
 		const results = await this._dataExplorerClientInstance.getColumnProfiles(
 			columnIndices.map(column_index => {