Initial version of internal cache, I don't love it yet #265
Conversation
severo
left a comment
severo
left a comment
There was a problem hiding this comment.
I put some random thoughts as comments.
I think this proposal helps shape a better DataFrame, with a decoupled cache.
| } | ||
| } finally { | ||
| data.eventTarget?.removeEventListener('resolve', callback) | ||
| // No cleanup needed |
There was a problem hiding this comment.
ok, so the try finally structure can be removed
| // | ||
| // For static data frames, eventTarget can be undefined. | ||
| eventTarget?: CustomEventTarget<DataFrameEvents> | ||
| // Optional cache listener for dataframes that support caching |
There was a problem hiding this comment.
maybe add some more comments to make it clear that the returned function is used to remove the listener and should be called when the user dismisses the data frame.
| // Simulate async data fetching | ||
| const value = await fetchCellData(row, column) | ||
| // store the fetched data in the cache | ||
| setCell(row, column, value) |
There was a problem hiding this comment.
you have to call setRowNumber too, otherwise, the row numbers will never appear (if I understand well)
There was a problem hiding this comment.
good idea to provide a cache API + an implementation
| getRowNumber: ({ row }) => ({ value: rowIndex }), | ||
| getCell: ({ row, col }) => cache.get(col).get(row), | ||
| getRowNumber: createGetRowNumber({ numRows: 1000000 }), | ||
| getCell: ({ row, column }) => cache.get(column)?.get(row), |
| /** | ||
| * Get all cached rows for a specific column. | ||
| */ | ||
| getCachedRowsForColumn(column: string): number[] { |
| */ | ||
| getCachedRowsForColumn(column: string): number[] { | ||
| const columnCache = cellCache.get(column) | ||
| return columnCache ? Array.from(columnCache.keys()) : [] |
There was a problem hiding this comment.
If we use an array, should we sort?
Or we might prefer to return a Set
| const { cacheKey, numRows, columnDescriptors, fetch, getRowNumber: customGetRowNumber } = options | ||
|
|
||
| // Use a ref to store the cache so it persists across re-renders but can be replaced when cacheKey changes | ||
| const cacheRef = useRef<{ key: string | undefined, cache: DataFrameCache, version: number } | null>(null) |
There was a problem hiding this comment.
not sure of my comment, but... instead of relying on a React ref + a hook, maybe we could transform this file into a simpler function that transform a DataFrame into a cached DataFrame (as we had initially in hightable), optionally passing a cache as an argument/option? The optional cache could still be stored in a React state, or Ref, but it would decouple a bit more.
|
|
||
| // Default getRowNumber implementation | ||
| const defaultGetRowNumber = useCallback((row: number): ResolvedValue<number> => { | ||
| if (row < 0 || row >= numRows || !Number.isInteger(row)) { |
There was a problem hiding this comment.
we have a helper function for that
| setCell, | ||
| setRowNumber, | ||
| clearCache, |
There was a problem hiding this comment.
maybe include these fields as a super interface called CachedDataFrame, that would extend DataFrame?
the returned object is also very similar to DataFrameCache... I think we could simplify and combine these three interfaces: DataFrame, DataFrameCache and this unnamed return type.
Playing around with an internal cache, ran into some inconveniences I need to think through