scovich
commented
5 months ago Note: In some cases it might be safe to simply ignore items that lack a visitor function from the engine, instead of panicking. For example, it is always safe (if potentially sub-optimal) to suppress a data skipping filter expression. However, that kind of optimization requires a domain-specific analysis of each case; by default we must assume that failing to fully visit all items could produce silent incorrect behavior.
I don't have any immediate intuition whether it's safe for a read path schema visitor to ignore unrecognized types. It might merely render the affected columns invisible and unreadable, which seems not so terrible? On write path, missing schema elements would almost certainly lead to "fun" problems.
Kernel FFI defines several "visitor" type structs, which engine is expected to populate with non-null function pointers. See EngineSchemaVisitor, for example:
Unfortunately, C/C++ does not trigger any compiler diagnostics for the code that initializes a POD, if the struct is extended with additional members. Any un-mentioned struct members are simply default-initialized by the compiler and it's up to the human to somehow "know" the additional fields need to be initialized, e.g. https://github.com/duckdb/duckdb_delta/pull/22/
Thus, to avoid creating UB risk in rust whenever we extend a type, we need to protect the visitor function pointers with
Option(andpanic!properly on null pointer access). We could also make the visitor struct opaque with an initializer function in kernel -- so the changing signature causes a compilation error -- but that would require kernel heap allocation, a drop function, etc. Unclear which of those approaches is the lesser evil?