feat: Autoassigns column datatype in table widget #16701
Conversation
- Limited to only number, string and boolean datatypes
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
github-actions
bot
added
App Viewers Pod
| } | ||
|
|
||
| try { | ||
| parsedColumnValue = _.isString(columnValue) |
There was a problem hiding this comment.
We don't need to parse. we should distinguish "false" from false. Because checkbox treats "false" as true value. So if we set the type checkbox. it will show up as checked.
There was a problem hiding this comment.
Should we remove the whole try catch block here then? Since its only for the parsing and nothing else @sbalaji1192
There was a problem hiding this comment.
yeah. remove the block
There was a problem hiding this comment.
Without the parsing typeof "23" will give us string, hence the column will get TEXT type instead of NUMBER type. Is this going to be an issue? @sbalaji1192
There was a problem hiding this comment.
that's alright. We should only consider 23 as a number type.
| : columnValue; | ||
| } catch (e) { | ||
| // eslint-disable-next-line no-console | ||
| console.error( |
There was a problem hiding this comment.
need to return the default type here.
Adjust jest tests accordingly
…E/12831-autoassign-columntype-in-table-widget
| Limited to checking upto maxRowsToCheck | ||
| */ | ||
| while ( | ||
| (columnValue === null || columnValue === undefined) && |
|
/ok-to-test sha=e775589 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3051047438. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/3051047438. Click to view performance test results| | Run 1 (ms)| Run 2 (ms)| Run 3 (ms)| Run 4 (ms)| Run 5 (ms)| Minimum (ms)| Median (ms)| Mean (ms)| Range (%) | SD.Sample (%) | SD.Population (%)| |
|
@souma-ghosh On changing the type of a value, it isn’t reflecting on the table (In this case, I changed a boolean value to string). Not sure if this pertains to this PR. Could you have a look and confirm please? |
@sbalaji1192 @dilippitchika Should we try to switch the column data type if the user changes the table data manually as shown in the above video? |
|
@souma-ghosh We don't have to. We're only going to guess the type when a column is generated. we don't have to keep guessing every time the table data changes. |
|
If everything else is working fine we can go ahead with the current solution then @laveena-en |
|
Agreed with balaji, we only guess the first time. After that, it's the users responsibility to manage them. |
|
Yes, we can go ahead with this.
|
|
@laveena-en what about dates? |
|
@dilippitchika Dates have not been handled yet. For us to confirm whether a column data is date type or not it has to be a valid ISO or UTC format or another format we explicitly specify. So we cannot guess a column to be of type date unless we know what format/formats we are expecting |
…E/12831-autoassign-columntype-in-table-widget
|
/ok-to-test sha=fd70e6b |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3065773253. |
|
/ok-to-test sha=fd70e6b |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3081404518. |
|
/ok-to-test sha=518bf59 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3096507382. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/3096507382. Click to view performance test results| | Run 1 (ms)| Run 2 (ms)| Run 3 (ms)| Run 4 (ms)| Run 5 (ms)| Minimum (ms)| Median (ms)| Mean (ms)| Range (%) | SD.Sample (%) | SD.Population (%)| |
…E/12831-autoassign-columntype-in-table-widget
|
/ok-to-test sha=95dd1f0 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3103250969. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/3103250969. Click to view performance test results| | Run 1 (ms)| Run 2 (ms)| Run 3 (ms)| Run 4 (ms)| Run 5 (ms)| Minimum (ms)| Median (ms)| Mean (ms)| Range (%) | SD.Sample (%) | SD.Population (%)| |
|
@souma-ghosh As discussed, it seems like for a specific date format (22/09/2022 18:30), the date seems to be seen as a number instead. Please do have a look. |
As we saw in the computed value for the api we are using here, we get a number not a string for |
|
LGTM! |
…E/12831-autoassign-columntype-in-table-widget
|
/ok-to-test sha=59ef853 |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3111691048. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/3111691048. Click to view performance test results
|
|
/ok-to-test sha=3b95e5b |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3115940165. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/3115940165. Click to view performance test results
|
souma-ghosh
deleted the
FEATURE/12831-autoassign-columntype-in-table-widget
branch
Description
When creating a column from tableData we were auto-assigning columns to "Plain Text" type by default without looking at the data of the column.
In this PR (In order to auto-assign better column types) before creating each column
Note: Currently limited the check for non-null values to
maxRowsToCheckrows, wheremaxRowsToCheck = 5Datatypes being checked for and the corresponding column type being assigned:
number: Numberboolean: Checkboxstring: Plain TextFixes #12831
Type of change
How Has This Been Tested?
Checklist: