Extract type mapping postprocessing to an external visitor #33308
Conversation
roji
force-pushed
the
RefactorTypeMappingProcessor
branch
from
3dcabef
to
1251ad3
Compare
Part of API review (dotnet#33220)
roji
force-pushed
the
RefactorTypeMappingProcessor
branch
from
1251ad3
to
d1428e1
Compare
|
|
||
| return query6; | ||
| return query7; |
There was a problem hiding this comment.
I see a weakness in this naming scheme
There was a problem hiding this comment.
Yeah.. The main goal here was to allow examining the different stages of postprocessing when debugging, i.e. not overwrite the same query variable with each successive pipeline step. Adding steps seems rare enough that it's OK - but I'm open to other ideas...
There was a problem hiding this comment.
You could name them according to the last step: baseProcessedQuery, typeMappedQuery, projectionAppliedQuery, etc.
There was a problem hiding this comment.
Or go the QBASIC route: query10, query20, query30 ...
There was a problem hiding this comment.
I'm worried about scalability, with future visitors coming to visit we may need to go into the hundreds/thousands here.
| @@ -1328,11 +1323,11 @@ protected override ShapedQueryExpression TranslateUnion(ShapedQueryExpression so | |||
| /// Inferred type mappings for queryable constants/parameters collected during translation. These will be applied to the appropriate | |||
| /// nodes in the tree. | |||
| /// </param> | |||
| [Obsolete("Override RelationalQueryTranslationPostprocessor.ProcessTypeMappings() instead.", error: true)] | |||
| protected virtual Expression ApplyInferredTypeMappings( | |||
| Expression expression, | |||
| IReadOnlyDictionary<(string, string), RelationalTypeMapping?> inferredTypeMappings) | |||
There was a problem hiding this comment.
You'd need to revert this to IReadOnlyDictionary<(TableExpressionBase, string), RelationalTypeMapping?> for the obsolete attribute to make sense
There was a problem hiding this comment.
Re-introducing TableExpressionBase seems like a bit much... Given that the visitor base class (RelationalInferredTypeMappingApplier) is also being obsoleted, I'll just update the message there and remove this altogether - it should be more than enough to point provider writers in the right direction.
| switch (expression) | ||
| { | ||
| case JsonEachExpression jsonEachExpression | ||
| when TryGetInferredTypeMapping(jsonEachExpression.Alias, "value", out var typeMapping): |
There was a problem hiding this comment.
Should these literals be extracted?
There was a problem hiding this comment.
Yeah, a cleanup around this is probably in order.
Part of API review (#33220)
This extracts RelationalInferredTypeMappingApplier out of the translation visitor and makes it its own, independent postprocessing visitor. Note that I ended up not introducing a parameter object, but just passing in the general postprocessing dependencies that RelationalQueryTranslationPostprocessor has.