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

Adding documentation for custom function mapping #2899

Merged
merged 1 commit into from
Dec 11, 2020
Merged

Adding documentation for custom function mapping #2899

merged 1 commit into from
Dec 11, 2020

Conversation

maumar
Copy link
Contributor

@maumar maumar commented Nov 24, 2020

Fixes #500

@maumar maumar requested a review from a team November 24, 2020 06:15
@maumar maumar force-pushed the udfs branch 2 times, most recently from 5765a08 to 2dbd6a0 Compare November 24, 2020 06:38
@maumar maumar force-pushed the udfs branch 2 times, most recently from 47daaa5 to 1441d2c Compare November 24, 2020 08:40
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.

Pedantic English review FTW! Feel free to ignore points if disagree.

BTW see build warnings in the CI checks.

entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
Copy link
Contributor

@smitpatel smitpatel left a comment

Choose a reason for hiding this comment

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

Also add

  • The HasFunction overload which takes lambda with default params to register a function
  • DbFunctionAttribute and properties over that.
  • Specifying a schema or marking function as built-in
  • Mapping function return type or parameter to particular store type when not natively supported and scenario where it works.
  • argument propagates nullability API and link to the null semantics docs.
  • Ideally, we should put down the example of mapping to JSON function How to write DbFunction's translation? efcore#11295 (comment) Probably most used usecase for the UDF so far
  • Mention that TVF cannot use HasTranslation API
  • Few more cautions about HasTranslation API and need to understand the SQL expression tree.

entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
entity-framework/core/modeling/custom-function-mapping.md Outdated Show resolved Hide resolved
@maumar maumar force-pushed the udfs branch 5 times, most recently from f940be5 to f8c0098 Compare December 2, 2020 07:54
entity-framework/toc.yml Outdated Show resolved Hide resolved
@maumar maumar force-pushed the udfs branch 3 times, most recently from 51b5370 to 1ba91a4 Compare December 8, 2020 03:29
args.First(),
args.Skip(1).First(),
typeof(bool),
new SqlServerBoolTypeMapping("bit")),
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider translating to ABS(a - b) to avoid this warning and also give example of SqlFunctionExpression translation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ended up going with slightly more complicated translation to both show the SqlFunction and custom specification of type mappings - should we add a note/caution that this is an advanced scenario?

@maumar maumar force-pushed the udfs branch 2 times, most recently from f3d503c to 0f8f1c6 Compare December 10, 2020 22:55

#region HasTranslationFunctionConfiguration
// 100 * ABS(first - second) / ((first + second) / 2)
var doubleTypeMapping = new SqlServerDoubleTypeMapping("float");
Copy link
Contributor

Choose a reason for hiding this comment

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

You can avoid type mapping and associated warning if you make first/second double in C# and just reuse their type mapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't we want to show people how to create the mapping themselves? like when you divide/multiply by a double constant, you would have to do it anyway no?

@maumar maumar merged commit b29f878 into master Dec 11, 2020
@smitpatel smitpatel deleted the udfs branch December 15, 2020 22:50
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.

New feature topic: custom function mapping
4 participants