Skip to content

proof of concept for polymorphic query#977

Open
pjhuck wants to merge 1 commit intoDapperLib:mainfrom
pjhuck:polymoprhic-query
Open

proof of concept for polymorphic query#977
pjhuck wants to merge 1 commit intoDapperLib:mainfrom
pjhuck:polymoprhic-query

Conversation

@pjhuck
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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