Skip to content

Adds table valued functions support#38

Merged
rwasef1830 merged 1 commit intonpgsql:devfrom
neisbut:dev
Jul 12, 2016
Merged

Adds table valued functions support#38
rwasef1830 merged 1 commit intonpgsql:devfrom
neisbut:dev

Conversation

@neisbut
Copy link
Copy Markdown
Contributor

@neisbut neisbut commented Jun 9, 2016

Hello guys, this PR adds support for table valued stored functions.

  1. I've added a tiny TVF to context which can query Blog records with simple ILIKE.
  2. Added all needed function declarations in BloggingContext.CreateModel
  3. And fixed core a bit of course.

Please review and let me know what you think. Thanks!

@dkabracadabra
Copy link
Copy Markdown

Where have you been 2 days ago:( I've done exactly the same yesterday. I could save a day wasted on investigation and debugging. Anyway THANK YOU!

@neisbut
Copy link
Copy Markdown
Contributor Author

neisbut commented Jun 10, 2016

Hi, @dkabracadabra. Sorry, I was collecting information 😄 Hope guys will check this soon. We are keen to have this feature!

@neisbut
Copy link
Copy Markdown
Contributor Author

neisbut commented Jun 20, 2016

Hi @roji, is there a chance you will check this PR soon?

@roji
Copy link
Copy Markdown
Member

roji commented Jun 20, 2016

I'm really sorry but it's unlikely I'll be able to review this anytime soon... I know the EF6 provider hasn't received much attention recently. @Emill, if you have any time for this that would be great. Otherwise you guys will have to work with your own fork until I have more free time...

@rwasef1830
Copy link
Copy Markdown
Contributor

Hello @neisbut,
Could you rebase this PR on latest dev for review ?

@neisbut
Copy link
Copy Markdown
Contributor Author

neisbut commented Jul 10, 2016

Hi @rwasef1830, done!

@rwasef1830
Copy link
Copy Markdown
Contributor

rwasef1830 commented Jul 10, 2016

@neisbut When I try to run the test under a debugger, I hit many assert failures inside EntityFramework itself:

Debug.Assert(column.TypeUsage.EdmType.DataSpace == DataSpace.SSpace);

This happens in the constructor of ScalarPropertyMapping in the call stack of dbModel.Compile() when running the unit test setup.

Although the test passes, the asserts mean that some core assumption inside EF is broken. I am not exactly familiar with the gory details of the CSpace and SSpace. Could you check it ?

Also, there is a project on GitHub https://github.com/Dixin/EntityFramework.Functions which seems to be adding TVF support to EF in general.

Does this work with EntityFramework6.Npgsql ?

@neisbut
Copy link
Copy Markdown
Contributor Author

neisbut commented Jul 11, 2016

@rwasef1830, oh, sorry, I forgot to check this. I made a mistake in function declaration. Fixed now. Could you please try again?

Actually my main goal is to support exactly EntityFramework.Functions :) For now it works for scalar function, but not for TVF. EntityFramework.Functions just generates function import code, but Npgsql still need to translate it to SQL. This is what I've implemented in this PR.

{
ParameterTypeSemantics = ParameterTypeSemantics.AllowImplicitConversion,
Schema = "dbo",
IsComposable = true,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be IsComposable = false ? Or you can use TVF inside LINQ ? (I'm not familiar with TVFs in general, so I'm just making sure).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rwasef1830, that's right, this flag allows to use this inside of LINQ expressions like this:
var query2 = from b in context.GetBlogsByName("blog1") select new { b.Name, Something = 1 };

@rwasef1830
Copy link
Copy Markdown
Contributor

@neisbut The test passes now, just a few questions, formatting nits left.

null);
dbModel.ConceptualModel.Container.AddFunctionImport(getBlogsFuncModel);

dbModel.ConceptualToStoreMapping.AddFunctionImportMapping(new FunctionImportMappingComposable(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rwasef1830, done, refactored long names.

@rwasef1830
Copy link
Copy Markdown
Contributor

@roji Please review this so I can merge it.
@neisbut Only a couple of nits and with @roji's approval it will be merged.

@roji
Copy link
Copy Markdown
Member

roji commented Jul 11, 2016

I definitely don't have time to dive into this at the moment... Formatting/cosmetics-wise things look good to me after @rwasef1830's comments, am OK for merging after they're fixed.

@rwasef1830 rwasef1830 merged commit 3d59c71 into npgsql:dev Jul 12, 2016
@rwasef1830
Copy link
Copy Markdown
Contributor

@neisbut Thanks for contributing this feature!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants