RM-9637 tvp infrastruktur herrichten #1397
Conversation
a-ctor
force-pushed
the
feature/RM-9637-TVP-Infrastruktur-herrichten
branch
from
101561b to
25d3083
Compare
a-ctor
force-pushed
the
feature/RM-9637-TVP-Infrastruktur-herrichten
branch
from
25d3083 to
f4555ea
Compare
| /// <summary> | ||
| /// Provides the <see cref="RecordDefinition"/>s for TVPs that can be used to efficiently batch table manipulations. | ||
| /// </summary> | ||
| public interface ITableManipulationRecordDefinitionProvider |
There was a problem hiding this comment.
RecordDefinition is located in Rdmbs.Parameters namespace. Right now, they're meant just for use as parameters. at some point, we probably want to move more stuff to Model namespace. Ask Martin why we kept it in Parameters?
| /// <summary> | ||
| /// Gets the Delete TVP <see cref="RecordDefinition"/> for the specified <paramref name="classDefinition"/>. | ||
| /// </summary> | ||
| RecordDefinition GetDeleteRecordDefinition (ClassDefinition classDefinition); |
There was a problem hiding this comment.
We could create a single one for the entire IRdbmstorageProviderDefinition.
| /// <summary> | ||
| /// Gets the Lock TVP <see cref="RecordDefinition"/> for the specified <paramref name="classDefinition"/>. | ||
| /// </summary> | ||
| RecordDefinition GetLockRecordDefinition (ClassDefinition classDefinition); |
There was a problem hiding this comment.
We could create a single one for the entire IRdbmstorageProviderDefinition.
| /// This method is intended to be used in script generation. | ||
| /// Use the other methods on this interface to access a specific TVP <see cref="RecordDefinition"/>. | ||
| /// </remarks> | ||
| IEnumerable<RecordDefinition> GetAllRecordDefinitions (IEnumerable<ClassDefinition> classDefinitions); |
There was a problem hiding this comment.
Not sure about this one. Is there a reason we can't just iterate the other methods?
There was a problem hiding this comment.
Sure, I mirrored the design from the other TVP interface. If you want we can remove this method and do the iteration in the consuming class.
There was a problem hiding this comment.
Yeah, the ISingleScalarStructuredTypeDefinitionProvider.GetAllStructuredTypeDefinitions() is the only way to know for which types to create a TVP. Here, it's more an "give me the TVP for this ClassDefinition". My overarching goal is to not have redundant APIs (or helper APIs) on the interface, just those that actually do stuff no other member of the interface does. Here, this API is something an extension method/utility method could do just as well.
-> I don't mind having an extension method / something that's testable. Just not redundant on the interface please :)
| public IReadOnlyCollection<IRdbmsStructuredTypeDefinition> GetTypeDefinitions (RdbmsProviderDefinition storageProviderDefinition, IEnumerable<ClassDefinition> classDefinitions) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(storageProviderDefinition); | ||
|
|
There was a problem hiding this comment.
add nullchecks immediately or once we're done with the API?
| return CreateSingleScalarStructuredTypeDefinitionProvider(storageTypeInformationProvider); | ||
| } | ||
|
|
||
| public virtual ITableManipulationRecordDefinitionProvider CreateTableManipulationRecordDefinitionProvider (RdbmsProviderDefinition storageProviderDefinition) |
There was a problem hiding this comment.
not sure if both methods should be virtual. possibly, it's just... I haven't looked at the rest of the factory for guidance.
| private record TvpTableTypeDefinitions ( | ||
| TableTypeDefinition DeleteTableTypeDefinition, | ||
| ImmutableArray<TvpColumnMetadata> DeleteColumns, | ||
| TableTypeDefinition InsertTableTypeDefinition, | ||
| ImmutableArray<TvpColumnMetadata> InsertColumns, | ||
| TableTypeDefinition LockTableTypeDefinition, | ||
| ImmutableArray<TvpColumnMetadata> LockColumns, | ||
| TableTypeDefinition UpdateTableTypeDefinition, | ||
| ImmutableArray<TvpColumnMetadata> UpdateColumns); |
There was a problem hiding this comment.
The immutable types should only be used if manipulation is a relevant factor. Otherwise, they're significantly more expensive than Frozen types or for private stuff, just sticking with an Array. As needed, use the IReadOnlyList/IReadOnlyCollection interface.
https://codeblog.jonskeet.uk/2025/12/31/changing-immutable-collections/
There was a problem hiding this comment.
I think you might be confusing ImmutableList<T> and ImmutableArray<T>. The linked blog article mentions ImmutableArray<T> as the right solution for "created once" immutable arrays. I don't see a reason to change the code here for performance reasons.
| private static readonly ImmutableDictionary<Type, object> s_optionalColumnsDefaultValues; | ||
|
|
||
| static TableManipulationRecordDefinitionProvider () | ||
| { | ||
| // Doing this in the initializer is a bit annoying so we do it in the static constructor. | ||
| var builder = ImmutableDictionary.CreateBuilder<Type, object>(); | ||
| builder.Add(typeof(string), string.Empty); | ||
| builder.Add(typeof(byte[]), Array.Empty<byte>()); | ||
|
|
||
| s_optionalColumnsDefaultValues = builder.ToImmutable(); | ||
| } |
There was a problem hiding this comment.
Since this is static readonly: I weould suggest we just make this into an IReadOnlyDicitionary and keep it based on Dictionary. This should yield better performance and we could use a field initializer
There was a problem hiding this comment.
This would be the place to use FrozenDictionary<T, K> as the dictionary implementation is inherently more complex than the immutable array and might not be the most efficient in terms of accesses. Funnily enough, using a FrozenDictionary<T, K> might even use an array and linear search instead of a dictionary structure as that is probably faster for two elements ^^
There was a problem hiding this comment.
it does? You have no idea how much I labored to not suggest the various "just two items" patterns, incuding the iterator types .net now offers :)
So... let's watch https://en.wikipedia.org/wiki/Frozen_%282013_film%29 ?
| private readonly ConcurrentDictionary<TableDefinition, Lazy<TvpTableTypeDefinitions>> _tableTypeDefinitions = new(); | ||
| private readonly ConcurrentDictionary<ClassDefinition, Lazy<TvpRecordDefinitions?>> _recordDefinitions = new(); | ||
|
|
||
| public TableManipulationRecordDefinitionProvider ( |
There was a problem hiding this comment.
continue review here, then review nested types
No description provided.