search

spine-tools / Spine-Database-API

Database interface to Spine generic data model
https://www.tools-for-energy-system-modelling.org/
GNU Lesser General Public License v3.0
6 stars 5 forks source link

Write type information for simple types as well #356

Closed soininen closed 1 week ago

soininen commented 7 months ago

Currently the type and default_type fields in parameter_value and parameter_definition tables are set to null if the value is string, float or boolean. It works for now, but would break if we introduced e.g. integers as a new type because the meaning of e.g. JSON "1" would be ambiguous. If we were to switch from JSON to binary representation, a bag of 64 bits could mean anything from float to int to boolean. To be future proof, we should store the value type for every type we have in the database.

manuelma commented 3 months ago

This probably impacts SpineInterface, maybe substantially, any thoughts on that?

soininen commented 3 months ago

This probably impacts SpineInterface, maybe substantially, any thoughts on that?

It will impact, so we need to coordinate updates between spinedb_api and SpineInterface. I have bumped the DB server version to 8 in the PR so old versions of SpineInterface should give a proper warning. I could try to fix SpineInterface myself but I would rather see someone more knowledgeable in Julia to work on it. In any case, I am not planning to merge the PR until SpineInterface is up-to-date. I have sent a message about this to the SpineOpt channel in Element, perhaps we could continue the discussion there?

manuelma commented 3 months ago

Isn't there an alternative? The JSON standard doesn't require the type and disambiguates values depending on the internal structure ("1" is a string, 1 is a number, true is a boolean). We could use the same and also introduce an extra rule where floating point numbers need to be specified including the dot - so if one wants the number 'one' as a floating point number, they should enter 1.0. Question is, would that work? And if yes, is that better than including the type explicitely? And should we consider backwards-compatibility when answering the previous question?

soininen commented 3 months ago

To me, the biggest issue is inconsistency. If the type column is null, we need to rely on json to parse the value to get its type and this happens only in case of certain types. This far it has not been a huge problem but I think it will simplify things in the future. For example, if we were to introduce integers, we would have to ensure that the type we write into the DB is the actual type so we do not mix up floats and integers. On a hindsight, I feel we should have had the type information for all values all along.

Also, this change codifies the supported types into spinedb_api and paves way for spine-tools/Spine-Toolbox#2791, which allows limiting parameter values to certain types.

When it comes to backwards compatibility, I am unsure if the impact is too drastic. You may need to fix some parsing functions as is the case with SpineInterface but as far as I can see, that is not totally impossible. I might be overlooking lots of stuff here, though, so let me know if you see this as a problem.

manuelma commented 3 months ago

Thanks @soininen - you're definitely right in that the impact is not impossible to absorb. I guess my main worry is the limited resources to work on SpineInterface which might require some strategic action.

soininen commented 3 months ago

Merging the PR is not urgent. It is the summer holiday season anyway.

soininen commented 2 months ago

Looks like the changes to SpineInterface were pretty minimal, see spine-tools/SpineInterface.jl#127