Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
87897a8 to
b8c399f
Compare
f599763 to
eac6b73
Compare
| @dataclass(frozen=True) | ||
| class VectorQuery: | ||
| """Represents a vector search query for a specific field in a collection. | ||
| class Query: |
There was a problem hiding this comment.
Making Query a base class with multiple subclasses—e.g., FTSQuery (with query_string and match_string) and VectorQuery—and exposing concrete query types to users seems preferable to exposing only the generic Query. Is this approach intended/desired?
There was a problem hiding this comment.
Good point, and worth revisiting given the roadmap. FTS support is planned, and Query will likely gain FTS-specific fields (e.g., query_string, match_string, or a nested object).
With that context, the concern in the comment becomes more concrete: a flat Query carrying both vector fields (field_name, id, vector, param) and FTS fields simultaneously creates a combinatorial validation problem—the number of invalid field combinations grows quickly, and the intent of any given Query instance becomes harder to read at a glance.
That said, we'd still push back on the subclass approach. The issue with FTSQuery(Query) is that it implies an is-a relationship that doesn't hold cleanly—an FTS query and a vector query are not specializations of the same concept, they're different query modes. Subclassing also tends to invite isinstance dispatch in the executor, which is a maintenance smell.
The direction we'd prefer is composition over inheritance: introduce a dedicated FTSQuery dataclass that encapsulates FTS-specific parameters, and add it as an optional field on Query:
@dataclass(frozen=True)
class FTSQuery:
query_string: str
match_string: Optional[str] = None
@dataclass(frozen=True)
class Query:
field_name: str
id: Optional[str] = None
vector: VectorType = None
param: Optional[...] = None
fts: Optional[FTSQuery] = None # mutually exclusive with id/vectorThis keeps the public API surface flat (still just Query), makes the field boundaries explicit, and keeps _validate() tractable (one mutual-exclusion check between fts and id/vector). The collection.query(list[Query]) signature also remains unchanged.
We'll revisit the exact shape once FTS requirements are more concrete, but the composition approach gives us a cleaner extension point than a subclass hierarchy.
No description provided.