SEPURI-SAI-KRISHNA commented on PR #45456:
URL: https://github.com/apache/superset/pull/45456#issuecomment-6369557892
Thanks, this is an accurate review. All four cases in the 🔴 reproduce on the
base this PR was cut from:
```
before registration after
CSV aggregate 01234,5 1234,5
CSV raw 01234,NA,5 1234,,5
XLSX aggregate header 'code' header 'Unnamed: 0'
XLSX raw first column kept first column dropped
```
Fixed by leaving the AG Grid attachment exactly as it arrives. I kept the
parse for the column and row metadata rather than `continue`ing before it,
since `colnames` is asserted by 34 existing cases.
I took the `ag-grid-table`-only scope you offered rather than covering
`table` too. The broad version fails five `table` tests that pin the rewrite
itself: CSV formula escaping, the configured separator and decimal, NA
handling, default-index suppression and the display-label case. The rewrite is
doing real work there, so removing it belongs in its own fix.
One update since your review: #45527 landed and added `all_columns`,
`groupby` and `verbose_map` to `_read_excel_for_client_processing`, so after
rebasing onto master the two XLSX rows no longer reproduce on their own. The
CSV two still do, and the fix covers all four.
Added `test_ag_grid_table_export_is_left_unchanged` for CSV and XLSX in both
aggregate and raw mode.
On the comment: you are right. `formatColumnValue` only labels
drill-to-detail (`AgGridTableChart.tsx:558-581`) and cells go through
`valueFormatter` (`useColDefs.ts:345`). It now says the plugin picks each
column's formatter with the Table plugin's `transformProps` logic and the same
`column_config` keys.
On the comparison columns: agreed it should not block, and the follow-up I
mentioned earlier in the thread is still owed. It is more than a lookup fix,
since the `Main`, `#`, `â–³` and `%` columns are derived in the browser and do
not exist in the report payload at all, so the issue has to cover porting that
derivation rather than widening the label match. I will post the link here once
it is open.
On the hard-coded viz-type pair: agreed. The new branch needed the viz-type
string twice so it has a local constant, which is not the shared one you
described. `connectors/sqla/models.py:775` carries the pair as well, on top of
the places you listed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
