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

Long and ULong properties marked as rowversion automatically convert to binary on SQL Server #29961

Merged
merged 1 commit into from
Jan 3, 2023

Conversation

ajcvickers
Copy link
Member

Fixes #12434

@ajcvickers ajcvickers requested a review from a team January 1, 2023 20:50
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to note that this introduces implicit usage of NumberToBytesConverter, which means it wouldn't be trimmable. I'm not claiming NumberToBytesConverter specifically is big (and we already do this kind of thing with many other converters), but in the trimming/NativeAOT world there's a tradeoff between making everything "just work" out of the box, and reducing binary size.

It may be worth shifting our mindset to requiring explicit configuration at least in some cases, especially where the explicit gesture is very simple.

@ajcvickers
Copy link
Member Author

Keep in mind that NumberToBytesConverter is already brought in anyway.

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case...

@ajcvickers
Copy link
Member Author

@roji I think we need to consider type mapping like model building. Right now, all of type mapping, including converter selection, is referenced always, and much of it is eagerly initialized. We should get to a place where with a compiled model and queries we don't need any but pre-defined type mappings, including converters, and the rest can be trimmed. But that's not trivial.

@roji
Copy link
Member

roji commented Jan 3, 2023

Interesting, we should think about that at some point... It definitely seems less trivial conceptually than model building, where there's a very clear (existing) split between a (runtime) model and all the (non-runtime) infra needed to build it.

Maybe there's some world in which stuff like ValueGeneratorSelector is no longer there at runtime, only in AOT-generated code, I don't know.

@ajcvickers
Copy link
Member Author

Yes, the separation is not as clear, but I think it can be done. Note we already have #25926.

@ajcvickers ajcvickers merged commit 2d70ae9 into main Jan 3, 2023
@ajcvickers ajcvickers deleted the ItsAllBits0101 branch January 3, 2023 11:28
@roji
Copy link
Member

roji commented Jan 3, 2023

Sounds good - let's keep it in mind with the ongoing AOT work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IsRowVersion or [Timestamp] on property that maps to provider byte[] type should generate rowversion column
2 participants