Lower default field list size limit to 10k - #6749
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
https://github.com/quickwit-oss/quickwit/blob/77d2c15f3ea1f1b2eb44968facff1b7594dadaf7/quickwit-search/src/list_fields/mod.rs#L39
For _field_caps and _mapping requests matching more than 10,000 dynamic field/type pairs, this new default silently omits entries that were previously returned, but the public API documentation neither discloses the cap nor tells operators about QW_FIELD_LIST_SIZE_LIMIT to preserve the old behavior. Document the changed default and truncation semantics so clients can account for incomplete responses.
AGENTS.md reference: AGENTS.md:L23-L24
https://github.com/quickwit-oss/quickwit/blob/77d2c15f3ea1f1b2eb44968facff1b7594dadaf7/quickwit-search/src/list_fields/mod.rs#L39
With a distributed request where each leaf has just over 10,000 candidates, leaf::merge_fields_metadata applies this same cap before root_list_fields performs the global merge. For example, a field occurring in two splits on each of two leaves is discarded locally if each leaf has 10,000 leaf-specific fields occurring in three splits, even though globally that discarded field occurs in four splits and should outrank every retained field. Lowering the shared cap makes this incorrect global top-field selection affect 10,001-field datasets that previously merged completely; retain enough leaf candidates or apply the final cap only after global frequencies are known.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd57cc0769
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| index_uri: index_meta.index_uri.to_string(), | ||
| field_patterns: search_request_for_leaf.field_patterns.clone(), | ||
| split_offsets: job_group.into_iter().map(|job| job.offsets).collect(), | ||
| limit: search_request_for_leaf.limit, |
There was a problem hiding this comment.
When a request spans multiple leaf requests, forwarding the exact limit causes every leaf to discard candidates before their global num_splits totals are known. For example, with limit=1, if each of two leaves has a different local field in 100 splits and the same common field in 99 splits, both leaves discard the common field even though its global count of 198 should make it the retained field. Use a leaf cap of at least max(request_limit, configured_default) and apply the requested limit only at the root so the documented most-common-field selection remains globally correct.
Useful? React with 👍 / 👎.
| start_timestamp: params.start_timestamp, | ||
| end_timestamp: params.end_timestamp, | ||
| query_ast: None, | ||
| limit: params.limit, |
There was a problem hiding this comment.
For _mapping(s) requests, this limits only the ListFieldsResponse; ElasticsearchMappingsResponse::from_doc_mapping still unconditionally inserts every declared field from each index's doc mapping before merging those limited entries. Consequently an index with 1,000 declared fields still returns all 1,000 for ?limit=10 (and even ?limit=0), contradicting the documented maximum and leaving mapping responses effectively uncapped for explicit schemas. Apply the limit while assembling the final mapping, accounting for both declared and dynamic fields.
Useful? React with 👍 / 👎.
|
|
||
| // Control if the request will fail if split_ids contains a split that does not exist. | ||
| // optional bool fail_on_missing_index = 6; | ||
| // optional bool fail_on_missing_index = 7; |
There was a problem hiding this comment.
i'm not familiar with this field. Di you know if it should be considered reserved, or is it some documentation for a potential future improvement?
There was a problem hiding this comment.
I added it in the original PR, It was probably planned for a follow-up.
https://github.com/quickwit-oss/quickwit/pull/4242/changes#diff-5eb9791a06ce038953b37caed18b46a0e2980c3276c70d9833c62336d8913f39R128
I'll change it to reserved, I don't think a commented entry makes sense.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/quickwit-oss/quickwit/blob/b24b35379c4efb1ef608e56cd1b9e11c537d0f5e/quickwit-serve/src/elasticsearch_api/model/field_capability.rs#L235
When a caller uses the new limit=0, or a low limit that removes every field contributed by one matched index, convert_to_es_field_capabilities_response derives its top-level indices array solely from the limited entries, so matched indices disappear (and zero returns indices: []). Since limit is documented as limiting fields rather than index resolution, preserve the resolved index IDs independently of the truncated field entries.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| | `allow_no_indices` | `Boolean` | If `true`, missing or closed indices are not an error. | (Optional) | | ||
| | `expand_wildcards` | `String` | Controls what kind of indices that wildcard patterns can match. | (Optional) | | ||
| | `ignore_unavailable` | `Boolean` | If `true`, unavailable indices are ignored. | (Optional) | | ||
| | `limit` | `Integer` | *(Quickwit-specific)* Maximum number of fields to return. Overrides `QW_FIELD_LIST_SIZE_LIMIT` for this request. | (Optional) | |
There was a problem hiding this comment.
The table labels limit merely as optional and never states that omitting it now truncates responses at 10,000 fields, even though lowering that default is the principal public behavior change in this commit. Users upgrading without an explicit environment setting therefore cannot tell from the API reference why fields have disappeared or which value restores prior behavior; document the 10,000 default and the truncation behavior here.
AGENTS.md reference: AGENTS.md:L23-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a73f507abf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Ok(merge_entries_with_limit( | ||
| entry_groups, | ||
| field_list_size_limit(), | ||
| field_list_size_limit(limit), |
There was a problem hiding this comment.
When one field has multiple observed types—for example, id appearing as both long and double—and the caller sends ?fields=id&limit=1, merge_entries_with_limittruncates the individual(field_name, field_type)entries to one._field_caps` therefore silently omits one capability type even though the response contains only one field name and has not exceeded the requested maximum; select field names first and retain all type entries belonging to each selected name.
Useful? React with 👍 / 👎.
| start_timestamp: search_params.start_timestamp, | ||
| end_timestamp: search_params.end_timestamp, | ||
| query_ast: query_ast_json, | ||
| limit: search_params.limit, |
There was a problem hiding this comment.
When a caller sends _field_caps?limit=0, the root correctly returns no field entries, but convert_to_es_field_capabilities_response derives the response's top-level indices array exclusively from those retained entries, so it also reports that no indices matched. The same loss occurs for any matched index whose fields are all truncated, even though limit is documented as limiting fields only; carry the resolved/matched index list separately so applying the field cap cannot erase response metadata.
Useful? React with 👍 / 👎.
Allow field capabilities and mappings requests to override the default field limit, and propagate the limit through root and leaf merges.
3e7e4ed to
91a5b52
Compare
3 participants
Summary
QW_FIELD_LIST_SIZE_LIMITfrom 100,000 to 10,000, as responses containing 100,000 fields may exceed gRPC message size limitslimitquery parameter to the_field_caps,_mapping, and_mappingsAPIsQW_FIELD_LIST_SIZE_LIMITwhen no request limit is providedTests
make fmtcargo nextest run -p quickwit-search list_fields