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

Allow precision on type mappings with scale #11439

Closed
roji opened this issue Mar 27, 2018 · 7 comments
Closed

Allow precision on type mappings with scale #11439

roji opened this issue Mar 27, 2018 · 7 comments
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@roji
Copy link
Member

roji commented Mar 27, 2018

#11419 adds better facet management on relation type mappings (to address #11167).

It's great to see this, but it seems that the options are currently Size or PrecisionAndScale. A precision-only option exists for date/time types in both MySQL and PostgreSQL. PostgreSQL also allows decimal/numeric with a precision but without scale (in which case scale defaults to 0).

@ajcvickers
Copy link
Contributor

@roji Because I was considering whether just precision and just scale were really needed, or whether the more general "size" could be used for all, and also because we don't really fully support "precision" and "scale" yet--I have been adding bits and pieces of support as needed to get other things working.

I assume from a Postgres perspective you are saying that just precision and just scale are need?

@roji
Copy link
Member Author

roji commented Mar 27, 2018

I'm not aware of a case where I can specify scale but not precision, so at least for PostgreSQL just precision should be enough. After all, these are "unnamed positional arguments" on the data type, and it would be impossible to know whether a single number in parentheses means precision or scale... But it may be worth looking at what Oracle/MySQL/whatever do.

@ajcvickers
Copy link
Contributor

"These are 'unnamed positional arguments' on the data type." This is why I was wondering about just using "size" but I think the problem is knowing where that "size" comes from in the type mapping, and consequentially how it interacts with what is set on the parameter. I think the best approach will likely be to allow the time mapping to have size, precision, and scale set independently, and then to orthogonality allow the type mapping to choose whether to append the size, the precision, or the precision and scale, and then just for completeness scale on it's own. (There also seems to be some ambiguity between size and precision for numerics on Oracle.)

@roji
Copy link
Member Author

roji commented Mar 27, 2018

That sounds like a good way forward to me... Another way to approach this is to allow the mapping to render it's complete data type (including size/precision/scale), like it renders literals. This would allow, for example, the PostgreSQL numeric mapping to omit the scale when it's zero (the default). This goes with the general direction in the new mapper of not trying to do too much but rather leaving it up to the provider etc.

It would also have to parse the same, possibly with the help of some basic support from you.

@ajcvickers
Copy link
Contributor

@roji I think a type mapping should be able to override StoreType and do a complete rendering if needed. I don't think EF now does any manipulation of the store type name unless opted into using the new flags.

We already parse out the base name and either a single or pair of postfixed values and put the results in the TypeMappingInfo. However, a single value is always parsed into "size" and two values are always parsed into "precision and scale". These flags could likely be extended to influence this parsing as well, so if the type expects either precision and scale or just precision, then a single value will be parsed into precision instead of size. I'll look into that, but it may be post 2.1.

@roji
Copy link
Member Author

roji commented Mar 28, 2018

I understand. I do hope you'll be able to do this for 2.1, as there have already been so many changes to the type mapping system and some stability in this area going forward would be nice...

@ajcvickers ajcvickers self-assigned this Mar 28, 2018
@ajcvickers ajcvickers added this to the 2.1.0 milestone Mar 28, 2018
ajcvickers added a commit that referenced this issue Mar 30, 2018
Fixes #11439 #11363

Also move Clone extension method to base type.
ajcvickers added a commit that referenced this issue Mar 30, 2018
Fixes #11439 #11363

Also move Clone extension method to base type.
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Mar 30, 2018
@roji
Copy link
Member Author

roji commented Mar 30, 2018

Great, thanks guys... I'll start working on preview3 very soon and will report.

@ajcvickers ajcvickers modified the milestones: 2.1.0-preview2, 2.1.0 Nov 11, 2019
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Development

No branches or pull requests

2 participants