Implement tables.get_import_preview RPC endpoint
#3635
Conversation
There was a problem hiding this comment.
Overall, I think this PR is quite tidy, and well-put-together. I would request that you more closely replicate the logic from the Python version, since it shouldn't add too much difficulty and it avoids a class of error that we currently handle gracefully (i.e., the current python version handles gracefully).
db/sql/00_msar.sql
Outdated
| WITH preview_cte AS ( | ||
| SELECT string_agg( | ||
| 'CAST(' || | ||
| __msar.build_cast_expr( | ||
| msar.get_column_name(tab_id, (col_cast ->> 'attnum')::integer), col_cast -> 'type' ->> 'name' | ||
| ) || | ||
| ' AS ' || | ||
| msar.build_type_text(col_cast -> 'type') || | ||
| ')'|| ' AS ' || msar.get_column_name(tab_id, (col_cast ->> 'attnum')::integer), | ||
| ', ' | ||
| ) AS cast_expr | ||
| FROM jsonb_array_elements(col_cast_def) AS col_cast | ||
| WHERE NOT msar.is_mathesar_id_column(tab_id, (col_cast ->> 'attnum')::integer) |
There was a problem hiding this comment.
There's a bug here (or at least it throws an exception when the old function didn't) whenever the table doesn't have an id column. I think you should extract some kind of get column or cast expression function from this so you can sort that out more smoothly (similarly to the python version; it should just return the column name for its expression when appropriate, and the cast blob otherwise).
There was a problem hiding this comment.
I don't think I quite understand the bug you are referring to, since the function(get_preview) is meant to be able to preview a mathesar table which is guaranteed to have an id col, I've taken the liberty to assume that it'll always be present for a table which is being previewed. id is also essential as it provides as a means for the returned records to be uniquely identifiable.
it should just return the column name for its expression when appropriate, and the cast blob otherwise
What's the downside of having a cast blob for a column where both target and column_type are same? I guess it would be a little efficient if we only use column name in such case but is it worth giving up the readability?
There was a problem hiding this comment.
I suppose I'm okay with just casting all columns (including id). I realize that in our current UI flow, the preview is only used for importing, but that's not enforced by the API, and the function itself is quite useful for previewing any proposed change to the column types of a table. Thus, I'd prefer avoiding breaking the current functionality that lets the function be used against any table regardless of whether it has an id column.
There was a problem hiding this comment.
Cool, I've made the requested changes in a20e663.
Anish9901
force-pushed
the
import_preview
branch
from
bb544b4 to
084a4ee
Compare
|
@Anish9901 You can merge once the conflicts are resolved, or re-request review if you think the merge resolution is complicated enough to warrant it. |
Fixes #3634
Checklist
Update index.md).developbranch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin