Harmonizes SchemaConfig to a named block structure #89
Conversation
vitrivr-engine-core/src/main/kotlin/org/vitrivr/engine/core/config/schema/SchemaConfig.kt
Show resolved
Hide resolved
| @@ -31,7 +31,8 @@ fun main(args: Array<String>) { | |||
|
|
|||
| /* Setup schema manager. */ | |||
| val manager = SchemaManager() | |||
| for (schema in config.schemas) { | |||
| for ((name, schema) in config.schemas) { | |||
| schema.name = name | |||
There was a problem hiding this comment.
This logic does not really belong here. It makes sense that the schema knows its own name, but that indirection should be resolved in the config object itself.
There was a problem hiding this comment.
Thank you for your review! I understand your point. Would it be preferable to assign the name in the ServerConfig (during read) or in an extra step with a dedicated method in the SchemaConfig?
There was a problem hiding this comment.
It would make sense to have this inside an init block inside the schema config; then, it would also be called once this is deserialized.
|
I have trialed some different approaches; I don't love this approach but it is a bit of a tricky situation to know the name during the initial deserialization. I don't quite see how an The other approach that was brought up would be in a similar manner to the initial approach - passing the name to |
…ema name assignment.
|
Kotlinx serializers cannot natively handle the case where a key from a map shall be unpacked into a class attribute in the way this config requires. The only workaround for that is by defining a custom SchemaSerializer, which seems both overkill and generates maintenance overhead for SchemaConfig. I propose this as a simpler alternative, with the name assignment handled by the SchemaManager (in my interpretation, that appears somewhat logical). |
| @@ -20,15 +20,15 @@ import java.nio.file.Paths | |||
| @Serializable | |||
| data class SchemaConfig( | |||
| /** Name of the [Schema]. */ | |||
| val name: String = "vitrivr", | |||
| var name: String = "vitrivr", | |||
There was a problem hiding this comment.
There is probably still a nicer way to handle this that does not require this field to be publicly writable, but that can be looked at at a later date and should not prevent us from merging this in a timely fashion.
In the schema config, the changes in this PR remove the "name" property for schemas, fields, exporters, and extractionPipelines, and instead places the name as a key in a map. Addresses issue #80