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

proof of concept for polymorphic query #977

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pjhuck
Copy link

@pjhuck pjhuck commented Mar 20, 2018

Furthering the discussion from #976, here's a proof of concept of the changes. There are simple tests for proving it works and supports horizontal partitioning.

In theory, the only public API change could be the SqlMapper.RegisterPolymorphicLoader method.

@NickCraver
Copy link
Member

I'm noticing at a glance here that all of the SequentialAccess is removed - this would be a performance regression since several primary providers have specialized low-allocation code paths that are taken when this is a known behavior. For example, here's SqlClient -> SqlDataReader.

IMO, this needs to be done without such a regression in play, and I'm not sure how to do that barring (in the provider or on-our-side) intermediate collections for rows for a very minority use case. Otherwise it's a performance burden on everyone for the benefit of very few...that's a hard API tradeoff to make.

@pjhuck
Copy link
Author

pjhuck commented Apr 2, 2018

yeah, I figured they were there for performance reasons. Agree that you don't wan't to take that hit for all. I can see a few ways to avoid it:

  1. As @mgravell noted, we could require the discriminator column to be first. I'd like to avoid this, as it makes the API much less usable since you'd have to be very careful with your SQL.
  2. We could make the make the CommandBehavior somehow configurable for the type. This PR already introduces the "custom deserializer" as state for a given type. Seems reasonable to keep custom CommandFlags there. So the performance hit would only be taken for the necessary types.

If your open to the concept of 2, then I can update this proof of concept with that.

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.

2 participants