Implement showing and changing a column's type

[kgodey]

Problem

Users might want to change the data type of an existing column on their table.

Proposed solution

The "Working with Columns" design spec has a solution for showing and changing column types, which we need to implement on the frontend.

Please note that we're only implementing changing the Mathesar data type in this milestone. Options specific to individual data types will be implemented in the next milestone.

Number data types should save as NUMERIC.
Text data types should save as VARCHAR.
Date/time data types can be disabled for now since they're not fully implemented on the backend.

Additional context

• Backend work:
  • Type mapping between Mathesar and database types #532 to get the list of types
  • Create a Column API #199 to get valid target types and change types
• Design issue: Design for editing column properties and table constraints #324
• Design discussion: Working with Columns #436
• Mapping DB data types to Mathesar data types #269

[asharonbaltazar]

Please assign this to me, @kgodey

[asharonbaltazar]

I have some suggestions in regards to the implementation. Figma linked in the wiki wasn't very helpful, unfortunately. If I shouldn't be asking this here, let me know.

1. Within the column dropdown, have another dropdown listing all available valid data types. Clicking a dropdown option will close the dropdown and change the data type.
2. Allow the small indicator (i.e. T mathesar id) to be clickable, which will display a dropdown listing all available valid data types.

[kgodey]

@asharonbaltazar could you elaborate on Figma not being helpful? it's a clickable prototype that shows you different actions you can take, and there should indeed be list of types in the column dropdown. Here are some screenshots I took from the prototype:

[asharonbaltazar]

Oh, sorry about that. I was referring to the Figma linked in the Working with Columns wiki page.

Thank you for this.

[kgodey]

@asharonbaltazar This is the same Figma prototype linked from the page. :). Let me know if you can't see the interactivity, there's more detail there that's not captured in the screenshots.

[dmos62]

Please assign this to me, @kgodey.

[dmos62]

I'm trying to figure out the broad algorithm of changing a type from frontend's perspective. As I see it at the moment:

1. Given any column, present user with a choice of the same set of Mathesar types that are fully implemented at the moment (i.e. Number and Text);
2. Given a column, let the user request changing its type to one of above;
3. If user requests to change a column's type to some Mathesar type, check what is the default target DB-layer type for that Mathesar type (e.g. NUMERIC for Number);
4. Forward a request to the backend to change that column's DB-layer type to above DB-layer-type.

Does that sound right? Also, am I right in assuming that valid_target_types is irrelevant at this point?

You'll notice, I introduced the term default target DB-layer type to describe what is the default DB-layer type when switching a column to some Mathesar type. It's essetially the information needed to request the backend to change a column's type (since the backend expects DB-layer types, but user is dealing in Mathesar types).

The /databases/{db_id}/types/ endpoint provides the information needed to know which DB-layer type belongs to which Mathesar type (a Mathesar type is a type category or a super-type in that sense).

What I'm getting at is that the Mathesar type construct has these two parts, mapping of DB-layers to it and mapping of it to DB-layers, but at the moment only one of them is formally defined.

A simple fix could be to add a default_target_type field to the objects returned by database types endpoint (/databases/{db_id}/types/). E.g.:

{
  "identifier": "text",
  "name": "Text",
  "db_types": [
    "CHAR",
    "VARCHAR",
    "TEXT"
  ],
  "default_target_type": "VARCHAR"
}

But, there could be valid reasons to keep Mathesar type descriptions on the frontend, like letting the user change the default target type or a more over-arching architectural choice to not use Mathesar types on the backend. In fact, I think @kgodey mentioned somewhere that Mathesar types are owned by the web service, which means the frontend, right?

Either way, concerning the definition of a Mathesar type (what it means to convert from and to it) it would be intuitive to keep it all in one place, either backend or frontend.

Also, if we were to centralize Mathesar type information, another bit of it is whether a given Mathesar type can be converted to (conversion to it is implemented). For example, at the moment Number and Text can be converted to.

To sum up, I'm proposing to centralize the Mathesar type description and to make some insights into them explicit.

[pavish]

@dmos62 There are multiple points here. I'll cover them individually.

The algorithm on the frontend:

The steps you've mentioned is mostly correct. However, we also need to factor in valid_target_types.
Step 1 changes into:

1. Given any column, we will present the user with Mathesar types that have db type mappings, which are present in valid_target_types.
   If the user tries to change to a db type that is not present in valid_target_types, it will always return an error. It is better to only present the options that are valid.

Options:

For the purpose of this issue, we will present the user with all valid choices based on valid_target_types (Not just number and text).

Source of truth, for types:

I agree with providing the default target type in the API. Essentially, the frontend should not contain any type related checks.
Until the default target type is provided in the API, we can have a temporary hardcoded map on the frontend, which will contain Mathesar type -> default db type information.

Moving forward, all type related information, including icons, conditions to show for filters, grouping options etc., will only be maintained on the backend. The goal is to support addition of dynamic types. If new types are added, there should be no code changes on the frontend.

Other types:

The db types in 'other' mathesar type will not be presented to the user while switching types.
Mathesar also supports existing DBs. This is for displaying proper type information, for tables not created from Mathesar client.
In this case, we show the mathesar type as 'other' and then the db type, but do not allow switching to those types.

Let me know if this answers all your queries.

@kgodey Please take a look to see if I missed something here.

[dmos62]

@pavish you answered my queries.

Moving type logic to backend

That brings up another concern. If the backend has all information necessary to fulfil column type conversion requests that are parametrized in Mathesar types, it might be preferable to move that logic to backend.

This could be exposed to the frontend together with what is now valid_target_types. You'd have valid_target_db_types (or valid_target_sa_types, depending on convention) and valid_target_mathesar_types:

  "valid_target_db_types": [
    "BIGINT",
    "BOOLEAN",
    ...
    "REAL",
    "SMALLINT",
    "VARCHAR"
  ],
  "valid_target_mathesar_types": [
    "Number",
    "Text"
  ]

Rationale to expose both is the user or frontend might choose to deal in either Mathesar types or DB-layer types depending on circumstance (probably depending on whether fine grained type control is desired).

Then the PATCH /tables/{table_id}/columns/{column_id} type change request would use { "mathesar_type": "Number" } or { "db_type": "NUMERIC" }.

Location of valid target types declaration

At the moment valid_target_types are declared per column on the columns endpoint (tables/{table_id}/columns/), but as far as I could tell they're not column-specific. If I've read the backend code correctly, valid_target_types are specific to DB-layer types. So for example, all VARCHAR columns will have the same valid_target_types. Since DB-layer types are database specific, I'd make valid_target_types a sub-resource on the database resource and have it be keyed by DB-layer type. So on /databases/{db_id}/valid-target-types:

{
  "VARCHAR": {
    "valid_target_db_types": [
      ...
    ],
    "valid_target_mathesar_types": [
      ...
    ]
  },
  "NUMERIC": {
    "valid_target_db_types": [
      ...
    ],
    "valid_target_mathesar_types": [
      ...
    ]
  },
  ...
}

I suspect there's plans to make valid target types column-specific, so this last point might be moot.

PS

I'm slightly self-conscious about suggesting refactors and, for example, pushing work to backend, given that I'm a new contributor and was invited to work on a smaller scope issue than what we're discussing now. I'm aware that social norms might dictate more timidity on my part at this point, but I'm working under the assumption that I can be more helpful if I forego that. I hope that my proposals or insights are useful.

That said if this leads to work on the backend, I'd be happy to do it.

[pavish]

@dmos62 We work incrementally, and often come up with refactors and new ideas with the entire scope of a feature, while implementing a smaller portion. Suggesting refactors and bringing in a new perspective is more than welcome :)

Regarding your points:

Moving type logic to backend

I do not think it is essential to provide both valid_target_db_types and valid_target_mathesar_types.

• We already have a mapping between mathesar types -> db types. This can be used to create two maps on the frontend.

   mathesarTypeMap -> Map<MathesarType, Set<DBType>>
   dbTypeMap -> Map<DBType, Set<MathesarType>>
  

  This will offer O(1) access wherever required, and since this information is per db, it can be cached. We could the db store for this.
  Sending only the valid_target_db_types from the backend would suffice, since sending both would be redundant.

• The PATCH endpoint should follow REST conventions. The actual column model only stores the type (The DB type). It is the single source of truth. Having the endpoint work with both mathesar_type and db_type would not be a good idea.

  • One of our goals is to allow users to directly use the APIs we provide. They could use it to build their own client, or just to make things easier.
  • Since most API users would only work with DB types, it would only complicate things if we provide mathesar_types here.
  • The mathesar_types is useful only for our frontend at the moment (or anyone who might want to build a frontend for Mathesar), and it is only for representational purposes.
    (These are my opinions on this. @kgodey and @mathemancer may have different thoughts).

Location of valid target types declaration

• For the MVP, the valid types depend on the current type of the column. However, eventually it may depend on the data present in the rows for that column.
• Placing the valid_target_types on the column model offers more flexibility, for future work.
• You've already mentioned this. :)

[dmos62]

Central issue seems to be whether or not Mathesar types are specific to the frontend. If they are, backend ideally wouldn't be aware of them (and wouldn't hold related definitions). If they aren't specific to the frontend, it would be desirable to centralize the definitions and logic on the backend. The situation I'm trying to side-step is where we have logic and definitions split between front and back ends.

[pavish]

@dmos62 The logic and definitions are supposed to be only on the backend, as mentioned earlier.

The only logic portion on the frontend is mapping between the db types -> backend types (bi-directional). This information is also fetched from the backend, nothing is hardcoded on the frontend.

I do not see the logic and definition overlap you're mentioning.

[pavish]

@dmos62 As per your point, fetching this data avoids the overlap.

  "valid_target_db_types": [
    "BIGINT",
    "BOOLEAN",
    ...
    "REAL",
    "SMALLINT",
    "VARCHAR"
  ],
  "valid_target_mathesar_types": [
    "Number",
    "Text"
  ]

But, valid_target_types are currently fetched from the backend in the columns endpoint, and the mapping between mathesar_types and db_types are fetched from the types endpoint. We use them both to determine what to show.

I do not see any co-dependent information split between the frontend and backend here. Could you please elaborate?

[kgodey]

I just caught up on this conversation, the idea of adding a default_target_type parameter to the types endpoint makes sense to me. @dmos62 if you want to do that as part of this work, go ahead.

Mathesar types are a convenience that the API provides, they can be used by other clients as well and are not limited to the frontend. However, we do not want clients to have to use Mathesar types, they're simply a convenience to make types friendlier to non-technical users. The eventual idea is that users could plug in their own types in a particular Mathesar instance and create their own mappings between Mathesar types and DB types and the frontend could work with it automatically.

Right now, the main problem we haven't solved yet is how to store the logic in the backend for choosing the right DB type when multiple DB types map to a Mathesar type. (e.g. when is a type an INTEGER vs. a FLOAT vs. a DECIMAL). This is going to be hardcoded in the frontend for now, and we'll figure out how to abstract it out later. This is also beyond the scope of this particular issue.

[dmos62]

@pavish If all the information needed for constructing the bi-directional mapping between Mathesar and DB types (which includes the default target DB type) is put in the backend, the overlap between front and back ends is that the backend holds the Mathesar type definitions (information necessary for converting to and from it) and the frontend holds the (sole) logic for using those definitions.

Having the definitions on the backend and logic on the frontend is better than having part of the definitions here and part there (ditto for logic), but from what both of you are saying it sounds like there isn't a reason to have any Mathesar-type-related things on the backend.

Should I hardcode the contents of the database types endpoint (/databases/{db_id}/types/), together with default_target_type, on the frontend?

[kgodey]

but from what both of you are saying it sounds like there isn't a reason to have any Mathesar-type-related things on the backend.

@dmos62 There are a couple of reasons to have Mathesar type-related things in the backend:
(1) It allows users to extend the Mathesar types concept to add new types or rearrange the existing mapping of types in the future without having to re-write frontend code.
(2) It allows developers to build alternate Mathesar clients (e.g. a mobile application) and use the same type mappings that they would use on the Mathesar web frontend.

It would be best to continue pulling the database type information from the backend (and add the default_target_type to the API). We can figure out how to store the logic for using type definitions in the backend in a future issue so that it's centralized there. Feel free to open an issue to track that work (or I'll do it later).

[pavish]

@dmos62 All the information needed for the bi-directional mapping, is already in the backend.
The types endpoint gives it as mathesar_type -> db type. Constructing the db type -> mathesar_type can be done using the same response from the endpoint. It's just the reverse.

We don't have to hardcode anything regarding this, on the frontend.

The only information we need to hardcode is the default_target_type. This is temporary, as already mentioned. Once the api endpoint is updated, we can remove this. If you want to add this to the API in the same PR, then we do not have anything to hardcode.

[dmos62]

@kgodey's last comment covers my concerns.

@pavish the hardcoding I suggested in the last comment is moving everything Mathesar type related out of the backend and into the frontend. The rationale was that both the Mathesar type information and the conversion logic would be in the same place.

I'll see if I can salvage @asharonbaltazar's draft PR.

[kgodey]

Thanks @dmos62. FYI (in case it wasn't clear), implementing the type conversion logic to types other than the default_target_type is out of scope for this issue, we'll do that in future issues (e.g. #258). We're waiting to finalize the design work on conversion logic before opening those up for work.

[pavish]

@dmos62 I hope you're referring to the latest UX.

We missed to update the issue description with it. If you could please take a look at the above link,

1. The scope of this issue is to only implement the 'Set column type' option. (You can scroll inside this block, to view all types, in the UX prototype)

   

2. Options below that are not in scope of this issue. The information required for these advanced options is currently not provided by the backend, however the plan is to make sure the backend provides it.

The information required to implement (1) is provided by the backend, except the icon, and default_target_type. We could have a generic common icon on the frontend for the time being, and need not implement any type based check. We have already discussed the default_target_type.

I hope this covers everything, and that we are on the same page.

[dmos62]

I did miss the latest design. Thanks for catching me up. I think this covers everything 👍

[dmos62]

@pavish I'm trying to get the database ID. Is the intended way to do that to take the database name, as derived from the URL in App.svelte or as provided by utils/preloadedData.ts, and check it against the list of database objects, as provided by utils/preloadedData.ts or GET /api/v0/databases?

[pavish]

@dmos62 It can be obtained from the databases store from /stores/databases.ts. It contains all the databases that are preloaded, and if it is fetched manually, the store gets automatically updated.

You could create a new method in the file, getByName, which finds and returns the database from databases.data array.

The current db name, is passed on to each route from App.svelte.