-
Notifications
You must be signed in to change notification settings - Fork 852
feat(format): specify carried-column storage and allow carrying a keyed column #8856
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,21 @@ enum VectorApproxMode { | |
| Accurate = 2; | ||
| } | ||
|
|
||
| /* The covering ("included") index columns a query needs materialized, as dataset | ||
| * field ids. | ||
| * | ||
| * Exists as a message rather than a bare `repeated int32` because proto3 has no | ||
| * presence tracking for repeated fields, and the empty list is a distinct, meaningful | ||
| * state here: | ||
| * | ||
| * * absent: no narrowing computed; materialize every covering column declared. | ||
| * * present and empty: materialize nothing, though the index does declare covering. | ||
| * * present and non-empty: materialize exactly these. | ||
| */ | ||
| message CoveringProjection { | ||
| repeated int32 field_ids = 1; | ||
| } | ||
|
|
||
| // Serialized vector query parameters. | ||
| message VectorQueryProto { | ||
| // Query vector as Arrow IPC bytes (supports Float16, Float32, Float64, UInt8, etc.) | ||
|
|
@@ -46,6 +61,13 @@ message VectorQueryProto { | |
| * indexes, such as IVF_RQ. Other index types ignore this setting. | ||
| */ | ||
| VectorApproxMode approx_mode = 14; | ||
| /* Which covering columns the index must materialize for this query. Absent means | ||
| * "not computed" -- see CoveringProjection. Carried across the wire so a remote | ||
| * executor declares the same search output schema the planner did; without it the | ||
| * executor's node is wider than the plan it came from, and the surrounding nodes | ||
| * were built against the planner's narrower schema. | ||
|
Comment on lines
+66
to
+68
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand the need for the two sides to agree on the schema before-hand but can we do this? If the planner has to specify the covering projection, and the available fields are determined from index storage, does this mean the planner will have to load the index into memory? Isn't that something we probably want to avoid?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is correct. Today it reads both index files' footers and schema metadata, plus their IVF models. But it probably makes sense to build a version that excludes the IVF models. I can do that as a follow up - I do not think that changes this PR though |
||
| */ | ||
| CoveringProjection covering_projection = 15; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding this field changes the generated ReproducerExpected: the feature build succeeds. Observed: compilation exits 101.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 201a9e1: the serializer now emits the intentional absent projection, and the Substrait feature build succeeds. |
||
| } | ||
|
|
||
| /* Serializable form of ANNIvfSubIndexExec — the IVF sub-index search node. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this true? Wouldn't the default be to materialize nothing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absent means nobody computed a projection -- either an encoder older than this
field, or a planner that skipped the narrowing step. Intended to reproduce the behavior from before this field existed, when a covered index always materialized everything it declared