[Lens][Datatable] Fix share export and inspect data #193780
Conversation
- Add transposed table to list of tables - Fix table select dropdown styles
- export transpose datatable from share - support sorting transposed columns - sort transposed columns by args column order (zippered) - removed hidden columns from share output - sort rows when column sorting is present - push empty value to bottom
- Set transposed table as default selection in inspector - Export only transposed table on share - Remove unused prop-types
|
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
nickofthyme
added
Team:Visualizations
| <EuiFlexItem> | ||
| <div> | ||
| <EuiPopover | ||
| id="inspectorTableChooser" | ||
| button={ | ||
| <EuiButtonEmpty | ||
| iconType="arrowDown" | ||
| iconSide="right" | ||
| size="s" | ||
| onClick={this.togglePopover} | ||
| data-test-subj="inspectorTableChooser" | ||
| > | ||
| <FormattedMessage | ||
| id="data.inspector.table.inspectorTableChooserButton" | ||
| defaultMessage="Table {index}" | ||
| values={{ index: currentIndex + 1 }} | ||
| /> | ||
| </EuiButtonEmpty> | ||
| } | ||
| isOpen={this.state.isPopoverOpen} | ||
| closePopover={this.closePopover} | ||
| panelPaddingSize="none" | ||
| anchorPosition="downLeft" | ||
| repositionOnScroll | ||
| > | ||
| <EuiContextMenuPanel | ||
| items={this.props.tables.map(this.renderTableDropdownItem)} | ||
| data-test-subj="inspectorTableChooserMenuPanel" | ||
| /> | ||
| </EuiPopover> | ||
| </div> |
There was a problem hiding this comment.
This fixed the full width dropdown placement...
Before
After
| sortedColumns?: string[]; | ||
| columnSorting?: EuiDataGridColumnSortingConfig[]; |
There was a problem hiding this comment.
Here we pass both the sorted columns and now any column sorting config the user has defined.
There was a problem hiding this comment.
the EuiDataGridColumnSortingConfig is an array of id and direction, can we just use this instead of having both the sortedColumns and this array?
We can sort that configs array the same way as the sortedColums
There was a problem hiding this comment.
Are you saying to combine them such that the order of the columnSorting matches that of the sortedColumns?
// do this...
const columnSorting = [{ id: 'col-a', direction: 'none'}, { id: 'col-b', direction: 'desc'}];
// instead of this...
const sortedColumns = ['col-a', 'col-b'];
const columnSorting = [{ id: 'col-b', direction: 'desc'}];In that case I don't think so. The columnSorting is already an array who's order indicates the hierarchy of multiple filters. This is the same behavior in eui. Even though the current Table vis only supports a singular column sort, I don't think it would be a good idea.
Even in terms of usability of this function, I think it is much easier on the user as is rather than having to combine them, let alone understand what the order means.
There was a problem hiding this comment.
Removed this option in favor of sorting the datatable prior to passing to this util. See 44baf77.
| const colOrderMap = new Map(args.columns.map((c, i) => [c.columnId, i])); | ||
| firstTable.columns.sort((a, b) => { | ||
| return (colOrderMap.get(a.id) ?? 0) - (colOrderMap.get(b.id) ?? 0); | ||
| }); |
There was a problem hiding this comment.
Aligns the table column order to that of the args. This is to done to preserve the order of transposed columns as described in #193780 (comment).
x-pack/plugins/lens/public/app_plugin/csv_download_provider/csv_download_provider.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/lens/public/visualizations/datatable/visualization.tsx
Outdated
Show resolved
Hide resolved
| ) { | ||
| if (!columnSorting) return () => 0; | ||
|
|
||
| const columnIdMap = new Map(columnSorting.map(({ id }) => [id, sortedColumnIds.indexOf(id)])); |
There was a problem hiding this comment.
This is quadratic. Can we make leverage a lookup map?
There was a problem hiding this comment.
Removed in 44baf77, in favor of @kbn/sort-predicates utilities.
| const sortedIds = sortedColumns | ||
| ? columns | ||
| .map((c) => { | ||
| // need to find original id for transposed column | ||
| const sortIndex = sortedColumns.findIndex((id) => c.id.endsWith(id)); | ||
| return { | ||
| id: c.id, | ||
| sortIndex, | ||
| isTransposed: (sortedColumns[sortIndex] ?? '') !== c.id, | ||
| }; | ||
| }) |
There was a problem hiding this comment.
This is quadratic in terms of time.
Can you use a lookup table here in order to reduce this from N^2 to 2 * N?
I think the transposed logic should not be exposed here, rather the sortedColumns should be computed on the Lens side where it is aaware of the transposed state, and able to extract the original id with the proper function for it.
There was a problem hiding this comment.
Removed column sorting logic from export logic in dfc2221
| if (!columnSorting) return () => 0; | ||
|
|
||
| const columnIdMap = new Map(columnSorting.map(({ id }) => [id, sortedColumnIds.indexOf(id)])); | ||
| return (rowA: string[], rowB: string[]) => { |
There was a problem hiding this comment.
Have you considered leveraging the getSortingCriteria from the @kbn/sort-predicates package used by the Lens datatable? This will ensure consistency between the two.
Can find an example usage here: x-pack/plugins/lens/public/visualizations/datatable/components/table_actions.ts
There was a problem hiding this comment.
This was a great call, took a bit of time to wrangle all the necessary inputs but finally got it working. Updated in 44baf77.
- used sorted column order from state - use column order from transposed table via activeData - remove transpose logic from csv export logic - use prepareLogTable to strip out ghost columns ids from formulas
…rted to the csv export utility - move column & row sorting into vis definition - use `@kbn/sort-predicates` to sort rows - remove sorted columns logic - remove column sorting logic
| const datatables = | ||
| exportDatatables.length > 0 ? exportDatatables : Object.values(activeData ?? {}); | ||
| const sharingData = { | ||
| datatables, | ||
| csvEnabled, |
There was a problem hiding this comment.
Fallback to activeData if no tables are provided. Not sure there there could be a need to want to export no data.
| raw?: boolean; | ||
| columnsSorting?: string[]; | ||
| } | ||
|
|
||
| export function datatableToCSV( | ||
| { columns, rows }: Datatable, | ||
| { csvSeparator, quoteValues, formatFactory, raw, escapeFormulaValues, columnsSorting }: CSVOptions | ||
| { csvSeparator, quoteValues, formatFactory, raw, escapeFormulaValues }: CSVOptions | ||
| ) { | ||
| const escapeValues = createEscapeValue({ | ||
| separator: csvSeparator, | ||
| quoteValues, | ||
| escapeFormulaValues, | ||
| }); | ||
|
|
||
| const sortedIds = columnsSorting || columns.map((col) => col.id); | ||
|
|
||
| // Build an index lookup table | ||
| const columnIndexLookup = sortedIds.reduce((memo, id, index) => { | ||
| memo[id] = index; | ||
| return memo; | ||
| }, {} as Record<string, number>); | ||
|
|
||
| // Build the header row by its names | ||
| const header: string[] = []; | ||
| const sortedColumnIds: string[] = []; | ||
| const formatters: Record<string, ReturnType<FormatFactory>> = {}; |
There was a problem hiding this comment.
@dej611 I removed the column sort logic from here altogether. I was trying to find the best approach based on what we talked about and I think instead of having the vis provide the sort order of the columns we just have the vis provide the data to be exported.
This was the columns and rows are sorted as needed in the new vis.getExportDatatables method.
Additionally, the downloadCsvShareProvider.getShareMenuItems loads all tables from the activeData, so when there are multiple it is confusing to have a single list of sorted columns. Now if a vis needs to export more than one datatable, it can sort/filter any active datatables before passing them to the exporter.
LMK what you think...
| const colOrderMap = new Map(args.columns.map((c, i) => [c.columnId, i])); | ||
| firstTable.columns.sort((a, b) => { | ||
| return (colOrderMap.get(a.id) ?? 0) - (colOrderMap.get(b.id) ?? 0); | ||
| }); |
There was a problem hiding this comment.
I believe this fixes #164413 as an alternate to sorting columns in csv exporter.
💔 Build Failed
Failed CI Steps
Test Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
History
|
Summary
This PR fixes #187551 where the table provided in the inspector and the share export, did not match what was visible in the table.
Changes
Viewing the datatable from Inspect action
Zight.Recording.2024-09-23.at.02.06.10.PM.mp4
Exporting table to csv via the Share action
['css › X', 'css › Y', 'deb › X', 'deb › Y']).nullvalues at the bottom to match eui behavior.The Lens table above would output a csv that shows as shown below (in Numbers).
Or in raw form as.
Checklist