Skip to content
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

Write type information for simple types as well #356

Open
soininen opened this issue Feb 9, 2024 · 7 comments
Open

Write type information for simple types as well #356

soininen opened this issue Feb 9, 2024 · 7 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@soininen
Copy link
Collaborator

soininen commented Feb 9, 2024

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.

@soininen soininen added enhancement New feature or request 0.8 labels Feb 9, 2024
@soininen soininen added this to the v0.8.0 milestone Feb 9, 2024
@soininen soininen modified the milestones: v0.8.0, v0.8.1 Apr 30, 2024
@soininen soininen removed the 0.8 label Apr 30, 2024
@soininen soininen self-assigned this May 30, 2024
soininen added a commit that referenced this issue Jun 26, 2024
Floats, booleans and strings now have explicit types in parameter_definition,
parameter_value and list_value tables.

Re #356
soininen added a commit to spine-tools/Spine-Toolbox that referenced this issue Jun 26, 2024
spinedb_api now requires type information for scalar parameter values.

Re spine-tools/Spine-Database-API#356
soininen added a commit to spine-tools/Spine-Toolbox that referenced this issue Jun 26, 2024
@manuelma
Copy link
Collaborator

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

@soininen
Copy link
Collaborator Author

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
Copy link
Collaborator

manuelma commented Jun 27, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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

soininen added a commit that referenced this issue Jul 12, 2024
ParameterValueTypeMapping now exports real scalar types, not just
'single_value'.

Re #356
@soininen
Copy link
Collaborator Author

soininen commented Jul 12, 2024

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

soininen added a commit to spine-tools/Spine-Toolbox that referenced this issue Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants