more clarity on open questions

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>

Author: connortsui20

proposals/0005-extension-types.md

@@ -22,18 +22,18 @@ Vortex provides an `Extension` variant of `DType` to help with this. Currently,
 a new extension type by defining an extension ID (for example, `vortex.time` or `vortex.date`) and
 specifying a canonical storage type that behaves like the "physical" type of the extension type.
 For example, the time extension types use a primitive storage type, meaning they wrap the primitive
-scalars or primitive arrays with some extra logic on top of it (mostly validating that the
-timestamps are valid).
+scalars or primitive arrays with some extra logic on top (mostly validating that the timestamps are
+valid).
 
 We would like to add many more extension types. Some notable extension types (and their likely
 storage types) include:
 
 - **Matrix / Tensor**: This would be an extension over `FixedSizeList`, where dimensions correspond
   to levels of nesting. There are many open questions on the design of this, but that is out of
   scope of this RFC.
-- **Union**: The sum type of an algebraic data type, like a Rust enum. We would likely implement
+- **Union**: The sum type of an algebraic data type, like a Rust enum. One approach is to implement
   this with a type tag paired with a `Struct` (so `Struct { Primitive, Struct { types } }`).
-  Vortex may be well suited to represent this because it can compress each of the type field arrays
+  Vortex is well suited to represent this because it can compress each of the type field arrays
   independently, so we do not need to distinguish between a "Sparse" or "Dense" Union.
 - **UUID**: Since this is a 128-bit number, we likely want to add `FixedSizeBinary`. This is out of
   scope for this RFC.
@@ -46,16 +46,16 @@ Take the time extension types as an example of where this limitation does not ma
 run a `compare` expression over a timestamp array, we just run the `compare` over the underlying
 primitive array. For simple types like timestamps, this is sufficient (and this is what we do right
 now). For types like Tensors (which are simply type aliases over `FixedSizeList`), this is also
-probably fine.
+fine.
 
-However, for more complex types like UUID, Union, or JSON, this is likely not sufficient. It
-remains an open question whether forwarding to the storage type is always enough, but assuming we do
-need richer behavior, we want a more robust implementation path (instead of wrapping
-`ExtensionArray` and performing significant internal dispatch work).
+However, for more complex types like UUID, Union, or JSON, forwarding to the storage type is likely
+insufficient as these types need custom compute logic. Given that, we want a more robust
+implementation path instead of wrapping `ExtensionArray` and performing significant internal
+dispatch work.
 
 ## Design
 
-### Completed Work
+### Background
 
 [vortex#6081](https://github.com/vortex-data/vortex/pull/6081) introduced vtables (virtual tables,
 or Rust unit structs with methods) for extension `DType`s. Each extension type (e.g., `Timestamp`)
@@ -66,10 +66,12 @@ There were a few blockers (detailed in the tracking issue
 [vortex#6547](https://github.com/vortex-data/vortex/issues/6547)),
 but now that those have been resolved, we can move forward.
 
-### Current Work
+### Proposed Design
 
 Now that `vortex-scalar` and `vortex-dtype` have been merged into `vortex-array`, we can place
-all extension logic (for types, scalars, and arrays) onto an `ExtVTable` (it has been renamed now).
+all extension logic (for types, scalars, and arrays) onto an `ExtVTable` (renamed from
+`ExtDTypeVTable`).
+
 It will look something like the following:
 
 ```rust
@@ -168,25 +170,23 @@ impl ExtDTypeRef {
 }
 ```
 
-We do not yet know what the API for extension arrays should look like, so it is hard to say what
-other methods should exist on `ExtDTypeRef`.
+**Open question**: What should the API for extension arrays look like? The answer will determine
+what additional methods `ExtDTypeRef` needs beyond the scalar-related ones shown above.
 
 ## Compatibility
 
 This should not break anything because extension types are mostly related to in-memory APIs (since
 data is read from and written to disk as the storage type).
 
-There could potentially be performance optimizations we could make with the new extension type
-system, but it is hard to know for sure.
-
 ## Drawbacks
 
-As stated before, this might be overkill.
+If forwarding to the storage type turns out to be sufficient for all extension types, the
+additional vtable surface area adds complexity without clear benefit.
 
 ## Alternatives
 
-We could have many `ExtensionArray` wrappers with custom logic. This
-might not even work, but even if it did, it would likely be very clunky.
+We could have many `ExtensionArray` wrappers with custom logic. This approach would be clunky and
+may not scale.
 
 ## Prior Art
 
@@ -197,9 +197,11 @@ and also provides a
 
 ## Unresolved Questions
 
-It is not yet clear whether changes to our extension types are strictly necessary, or whether
-forwarding to the storage type will always suffice. We also do not yet know how to define compute
-expressions over extension arrays.
+- Is forwarding to the storage type insufficient, and which extension types genuinely need custom
+  compute logic?
+- What should the `ExtVTable` API for extension arrays look like? What methods beyond
+  `validate_array` are needed?
+- How should compute expressions be defined and dispatched for extension types?
 
 ## Future Possibilities
 
@@ -231,4 +233,4 @@ If we can get extension types working well, we can add all of the following type
 
 [^2]:
     We likely cannot implement `Variant` as an extension type because we have no way of defining
-    what the storage type would be (since we have no idea what the schema is for each row).
+    what the storage type would be (since the schema is not known ahead of time for each row).