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.

Sign up for GitHub

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

Jump to bottom

Query: revisit extensibility of null semantics visitor #20204

Closed
maumar opened this issue Mar 6, 2020 · 1 comment · Fixed by #20854
Closed

Query: revisit extensibility of null semantics visitor #20204

maumar opened this issue Mar 6, 2020 · 1 comment · Fixed by #20854
Assignees
@smitpatel
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone
5.0.0

Comments

@maumar
Copy link
Contributor

maumar commented Mar 6, 2020

Currently null semantics visitor has extensibility point to handle provider specific nodes (via VisitInternal), but we don't have good way to custom handling of existing sql expression nodes (e.g. some specific SqlFunctions in postgresql. We should consider if nullable property should be protected rather than public etc.
@smitpatel smitpatel self-assigned this Mar 6, 2020
@ajcvickers ajcvickers added this to the 5.0.0 milestone Mar 9, 2020
@ajcvickers ajcvickers assigned smitpatel and maumar and unassigned smitpatel Mar 9, 2020
@smitpatel
Copy link
Contributor

From API review to keep in mind

  • Rename to NullabilityProcessingSqlExpressionVisitor
    • Use a method (DoNotCache) instead of a setter on CanCache
    • Hide NonNullableColumns, add an Add method
    • Just return SelectExpression from Process (no bool)
    • Just return TResult from VisitInternal and use an output parameter for the bool

@bricelam bricelam mentioned this issue Apr 28, 2020
Closed
95 tasks
smitpatel added a commit that referenced this issue May 6, 2020
@smitpatel
Query: Improve extensibility of null semantics processor
d97c914 
Resolves #20204

For table sources, there is no concept of nullability
- `Process(TableExpressionBase)` which works as dispatcher.
- `Process(SelectExpression)` as a special case to avoid casting.
- No additional methods for individual table sources.

For SQL expressions,
- `Process(SqlExpression, out bool nullable)` which defaults allowOptimizedExpansion to false.
- `Process(SqlExpression, allowOptimizedExpansion, out bool nullable)` which works as dispatcher.
- Individual Process* methods to change bahavior of any particular SqlExpression.

Notes:
- Each individual SQLExpression processes it's children with appropriately set allowOptimizedExpansion flag. It collects nullable flag for all children and return nullable for itself through out parameter.
- NonNullableColumns are being used only in 2 places and both usages are different. Rather than crystal balling an API to expose it, we should just keep it private till a provider asks for it then we can discuss with provider writer what information is needed and how would they like it. Making it private does not cause any bug, may just miss an optimization in SQL.
- The processor does not derive from ExpressionVisitor anymore. It has ExpressionVisitor like dispatch system but non Visit API.
- Since it is not deriving from SqlExpressionVisitor there is no longer abstract method to force implementing for new SqlExpression type. Hence unknown expression type throws exception. Making it pass through could cause incorrect result as we don't visit custom expression's components.
- Added DoNotCache method rather than exposing _canCache.
- Changed API about returning tuple for caching to an out parameter.

TODO: Once API is approved, I will add XML docs.
@smitpatel smitpatel added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed needs-design labels May 6, 2020
@smitpatel smitpatel mentioned this issue May 6, 2020
Merged
smitpatel added a commit that referenced this issue May 7, 2020
@smitpatel
Query: Improve extensibility of null semantics processor
cac39a1 
Resolves #20204

For table sources, there is no concept of nullability
- `Process(TableExpressionBase)` which works as dispatcher.
- `Process(SelectExpression)` as a special case to avoid casting.
- No additional methods for individual table sources.

For SQL expressions,
- `Process(SqlExpression, out bool nullable)` which defaults allowOptimizedExpansion to false.
- `Process(SqlExpression, allowOptimizedExpansion, out bool nullable)` which works as dispatcher.
- Individual Process* methods to change bahavior of any particular SqlExpression.

Notes:
- Each individual SQLExpression processes it's children with appropriately set allowOptimizedExpansion flag. It collects nullable flag for all children and return nullable for itself through out parameter.
- NonNullableColumns are being used only in 2 places and both usages are different. Rather than crystal balling an API to expose it, we should just keep it private till a provider asks for it then we can discuss with provider writer what information is needed and how would they like it. Making it private does not cause any bug, may just miss an optimization in SQL.
- The processor does not derive from ExpressionVisitor anymore. It has ExpressionVisitor like dispatch system but non Visit API.
- Since it is not deriving from SqlExpressionVisitor there is no longer abstract method to force implementing for new SqlExpression type. Hence unknown expression type throws exception. Making it pass through could cause incorrect result as we don't visit custom expression's components.
- Added DoNotCache method rather than exposing _canCache.
- Changed API about returning tuple for caching to an out parameter.

TODO: Once API is approved, I will add XML docs.
smitpatel added a commit that referenced this issue May 7, 2020
@smitpatel
Query: Improve extensibility of null semantics processor
11c1d0a 
Resolves #20204

For table sources, there is no concept of nullability
- `Visit(TableExpressionBase)` which works as dispatcher.
- `Visit(SelectExpression)` as a special case to avoid casting and allow easy overriding.
- No additional methods for individual table sources.

For SQL expressions,
- `Visit(SqlExpression, out bool nullable)` which defaults allowOptimizedExpansion to false.
- `Visit(SqlExpression, allowOptimizedExpansion, out bool nullable)` which works as dispatcher.
- Individual Visit* methods to change bahavior of any particular SqlExpression.

Notes:
- Each individual SQLExpression processes it's children with appropriately set allowOptimizedExpansion flag. It collects nullable flag for all children and return nullable for itself through out parameter.
- NonNullableColumns are being used only in 2 places and both usages are different. Rather than crystal balling an API to expose it, we should just keep it private till a provider asks for it then we can discuss with provider writer what information is needed and how would they like it. Making it private does not cause any bug, may just miss an optimization in SQL.
- Added AddNonNullableColumn API for providers to add aditional column to the list.
- The processor does not derive from ExpressionVisitor anymore. It has ExpressionVisitor like dispatch system and API.
- Since it is not deriving from SqlExpressionVisitor there is no longer abstract method to force implementing for new SqlExpression type. Hence unknown expression type throws exception. Making it pass through could cause incorrect result as we don't visit custom expression's components.
- Added `VisitCustomSqlExpression` method to allow providers to add logic for custom functions without having to deal with non-nullable column reset.
- Added DoNotCache method rather than exposing _canCache.
- Changed API about returning tuple for caching to an out parameter.
smitpatel added a commit that referenced this issue May 7, 2020
@smitpatel
Query: Improve extensibility of null semantics processor (#20854)
3fbad8f 
Resolves #20204

For table sources, there is no concept of nullability
- `Visit(TableExpressionBase)` which works as dispatcher.
- `Visit(SelectExpression)` as a special case to avoid casting and allow easy overriding.
- No additional methods for individual table sources.

For SQL expressions,
- `Visit(SqlExpression, out bool nullable)` which defaults allowOptimizedExpansion to false.
- `Visit(SqlExpression, allowOptimizedExpansion, out bool nullable)` which works as dispatcher.
- Individual Visit* methods to change bahavior of any particular SqlExpression.

Notes:
- Each individual SQLExpression processes it's children with appropriately set allowOptimizedExpansion flag. It collects nullable flag for all children and return nullable for itself through out parameter.
- NonNullableColumns are being used only in 2 places and both usages are different. Rather than crystal balling an API to expose it, we should just keep it private till a provider asks for it then we can discuss with provider writer what information is needed and how would they like it. Making it private does not cause any bug, may just miss an optimization in SQL.
- Added AddNonNullableColumn API for providers to add aditional column to the list.
- The processor does not derive from ExpressionVisitor anymore. It has ExpressionVisitor like dispatch system and API.
- Since it is not deriving from SqlExpressionVisitor there is no longer abstract method to force implementing for new SqlExpression type. Hence unknown expression type throws exception. Making it pass through could cause incorrect result as we don't visit custom expression's components.
- Added `VisitCustomSqlExpression` method to allow providers to add logic for custom functions without having to deal with non-nullable column reset.
- Added DoNotCache method rather than exposing _canCache.
- Changed API about returning tuple for caching to an out parameter.
@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-preview5 May 11, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0-preview5, 5.0.0 Nov 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@smitpatel smitpatel

Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Milestone
5.0.0
Development

Successfully merging a pull request may close this issue.

Query: Improve extensibility of null semantics processor dotnet/efcore
3 participants
@ajcvickers @smitpatel @maumar