-
Notifications
You must be signed in to change notification settings - Fork 19
Fix /*/_mapping and /*/_field_caps handling
#1475
Fix /*/_mapping and /*/_field_caps handling
#1475
Conversation
/*/_mapping and /*/_field_caps handling
…doesn't check if we can query across multiple tables.
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.
Pull Request Overview
This pull request addresses issues with handling mappings and field capabilities for all indices by introducing a Meta pipeline and updating the table resolver logic. Key changes include:
- Adding a MetaPipeline constant and associated resolver support.
- Refactoring decision merging to use a basicDecisionMerger interface.
- Enhancing endpoint handlers and e2e tests to properly filter out a common table index.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| platform/v2/core/decision.go | Added MetaPipeline constant. |
| platform/table_resolver/table_resolver.go | Changed decisionMerger to an interface; updated index update logic. |
| platform/table_resolver/rules.go | Modified basicDecisionMerger implementation and usage. |
| platform/processors/es_to_ch_query/elasticsearch_to_clickhouse_query_processor.go | Updated HandleGetIndexMapping call to include context and log manager. |
| platform/functionality/field_capabilities/field_caps.go | Added filtering of common table from index list. |
| platform/frontend_connectors/router_v2.go | Updated router registrations to use the new MetaPipeline. |
| platform/frontend_connectors/route_handlers.go | Enhanced HandleGetIndexMapping to iterate over filtered indexes. |
| platform/frontend_connectors/es_responses.go | Updated response generation functions for mappings. |
| http_requests/_mapping.http | Added new HTTP request tests for mapping and field_caps endpoints. |
| ci/it/testcases/test_common_table_and_regular_table.go | Added e2e tests for common and regular table handling. |
| ci/it/integration_test.go | Included integration test for the common table scenario. |
| ci/it/configs/quesma-common-table-and-regular-table.yml.template | New configuration template supporting common table along with regular tables. |
| mappings := elasticsearch.GenerateMappings(hierarchicalSchema) | ||
| allMappings := make(map[string]map[string]any) | ||
|
|
||
| var filteredIndexes []string |
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.
The same as below, it would be better to filter out common_table in ResolveIndexPattern
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.
Fixed
| const ( | ||
| QueryPipeline = "Query" | ||
| IngestPipeline = "Ingest" | ||
| MetaPipeline = "Meta" |
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.
Have some concerns about these pipelines, but that's a matter of broader discussion
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.
Yes. This is quite controversial. We should reassign the responsibility of the table resolver.
|
/run-it |
|
It seems that the integration tests currently take more than 10 minutes and are failing due to this time limit. |
|
/run-it |
e405e4a to
7527a51
Compare
|
/run-it |
This PR fixes getting mappings and fields caps for all indices.
Details: