-
-
Notifications
You must be signed in to change notification settings - Fork 362
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set up Column
Django model with support for display options
#658
Comments
It seems that the We were confused before, and treating |
I will go with the approach @mathemancer mentioned. I am planning to model it just like the existing
As for the display_option field is concerned:
We might have to move from storing display_options field as a json into a proper table(either as a sparse table or a generic foreign key related table) depending upon the need to query the options, something to decide on before a stable release as the change would be possible only with a data transformation. |
@silentninja Your plan generally sounds good. Some notes:
|
I wasn't comfortable using oid either(oid will be unique and can be described but wont be possible with attnum) but I was thinking of it as a identifier, moreover columns are the only objects without a oid and thought of going with it. I will create a different base model instead of making oid as optional as that would unnecessarily remove constraints of other objects.
While it is true jsonb can be queried, there are few missing operators when using a index and could make the query not so trivial. Moreover updating the fields won't be a straight forward migration. But display_options look more like user settings and json seems much suitable in this case. Moreover will be helpful in the case of custom datatype extensions or plugins. I am more concerned about migration than querying now, but I will go for jsonb as it fits the use case |
@silentninja Sounds good. |
This spec brings breaking change to the API(referring to django column id instead of column index), will it be okay to introduce display options along with the breaking changes or should I make display options backward compatible and then introduce the breaking change. It is possible to make it backward compatible by referring to SQL alchemy column index in the apis and using it to get the Django columns Would like to know your opinion @kgodey @pavish @seancolsen |
@silentninja I think it would be better to first change column reference to id instead of index in a separate PR. We'll make changes on the frontend and add commits to the same PR. The display options PR can base off that branch. |
Display options is an optional field and an enhancement , so it won’t affect anything I can add it in the same PR |
I don't think we need backwards compatibility at this stage. Our API is v0 because it's unstable, see our API standards page. |
Problem
Some of our data types have display options. We need to store these display options associated with columns of those data types.
Proposed solution
(1) Create a
Column
Django model with adisplay_options
JSON field to store display options./api/v0/columns/
to use the Django model, including using the auto-generatedid
for the ID.(2) While reflecting columns from the database:
id
(we never update Column objects).(3) If we are updating column names/types from within Mathesar, we will update the existing Column object.
(4) For the purposes of this issue, do not allow display options to be set. We will implement setting display options in each specific data type issue.
Additional context
OID
and column index, which is what we'd be storing anyway. This way, we don't have to touch the database (people may already be using column comments for other things).The text was updated successfully, but these errors were encountered: