st.beta_table - Part 1 #2612
st.beta_table - Part 1 #2612
Conversation
karriebear
left a comment
karriebear
left a comment
There was a problem hiding this comment.
Got lazy and didn't look up the apache arrow documentation. It would be helpful for the review if you could include the structure of the table produced. I had to keep scrolling up and down to remember what each item was. I'll take a deeper look tomorrow but wanted to get you these comments for now. Also need to go through arrow.py
The only thing critical is the try/except for the import pyarrow. When you do the final review, you should try testing on python 3.9 as well to make sure we can install and run streamlit hello
| import Alert from "components/elements/Alert/" | ||
| import { getAlertKind } from "components/elements/Alert/Alert" | ||
| import { Kind } from "components/shared/AlertContainer" | ||
| import BetaTable from "components/elements/BetaTable/" |
There was a problem hiding this comment.
Not something that really matters but you only really have to include beta in streamlit/__init__.py. It's just the API that needs to mention beta. All these things are internal so we don't have to call it out as beta here
| A dataframe to convert. | ||
| """ | ||
| import pyarrow as pa |
There was a problem hiding this comment.
I would put this in a try/except since it is a conditional dependency.
| dataRows: number | ||
| dataColumns: number | ||
| rows: number | ||
| columns: number |
There was a problem hiding this comment.
Potentially more of a problem when reading through without any typing information but could we rename these so its obvious they're numbers? When I saw these variables in use after they've been destructured, I had assumed these were the actual data of rows, cols, etc. If we don't destructure them it would probably be fine but destructuring is awesome.
| const isMultiIndex = this.schema.column_indexes.length > 1 | ||
| return unzip( | ||
| this.schema.columns | ||
| .map(column => column.field_name) |
There was a problem hiding this comment.
You don't really need this map. I assume columns should not get expensive so it's probably fine
| ?.get(rowIndex) | ||
| ?.toString() |
There was a problem hiding this comment.
Not important at all but I find the ? on the new line confusing when it's modifying the previous line. My initial thought was this was supposed to be a ternary statement.
| throw new Error("Column index is out of range.") | ||
| } | ||
|
|
||
| const isBlankCell = rowIndex < headerRows && columnIndex < headerColumns |
There was a problem hiding this comment.
This sounds like a header cell instead but I'm not sure why this would be blank unless this is the cell where the row header and column header meets?
|
|
||
| switch (type) { | ||
| case "blank": { | ||
| return <th key={columnIndex} className={classNames}></th> |
There was a problem hiding this comment.
Would this work for a header column?
|
|
||
| return ( | ||
| <StyledTableContainer data-testid="stTable"> | ||
| {tableStyles && <style>{tableStyles}</style>} |
There was a problem hiding this comment.
Is there anyway to convert this to a styled component? I'm guessing this is a string of CSS?
See streamlit#2136 Shouldn't conflict with Henrikh's in-flight streamlit#2612
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
2 participants
Since there some name conflicts in the midst of the integration, the names used are not final and are subject to change.
Change list (back):
Arrow.proto. It will replaceArrowTable.protowhen the integration is complete.Element.proto.ArrowMixinclass that providesst.beta_table, and its marshalling functions.data_frame_to_bytesandbytes_to_data_frametotype_util.Change list (front):
.mjsfiles to our webpack config (it's required by theapache-arrowmodule).apache-arrowdependency topackage.json.BetaTablecomponent to render new table elements.Table.tsx.Arrow.ts) with helper functions for Arrow tables.P.S. All tests are ready, but in order not to spook the reviewer, they will be presented in the next PR.