feature-wide(es):add field mappings to existing index via _mapping api #3079
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3079 +/- ##
==========================================
+ Coverage 79.05% 79.25% +0.20%
==========================================
Files 131 131
Lines 19049 19189 +140
==========================================
+ Hits 15059 15209 +150
+ Misses 3990 3980 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for updating Elasticsearch index field mappings via the _mapping API endpoint during online operations. This enables users to add new fields to existing indices without downtime.
- Adds validation logic in
execute_checkto ensure PUT requests to_mappingcontain apropertiesfield - Implements
__put_mappingmethod to handle mapping updates viaconn.indices.put_mapping - Updates error messages and the list of supported API endpoints to include
_mapping
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| sql/engines/elasticsearch.py | Adds _mapping endpoint support with validation logic in execute_check, implements __put_mapping execution method, and updates supported endpoint list |
| sql/engines/test_elasticsearch.py | Adds comprehensive test coverage for _mapping API including validation tests, successful execution, error handling, and edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sql=doc.sql, | ||
| ) | ||
| else: | ||
| if is_pass is False: |
There was a problem hiding this comment.
像这种语句可以简洁的写成
| if is_pass is False: | |
| if not is_pass: |
| if is_pass is False: | ||
| is_pass = True |
There was a problem hiding this comment.
| if is_pass is False: | |
| is_pass = True | |
| is_pass = True |
这个判断在我看来没有意义, 你看看这样简单写行不行
| ) | ||
| else: | ||
| if is_pass == False: | ||
| if is_pass is False: |
There was a problem hiding this comment.
@copilot 这里的 is_pass 有点过于简单, 能否帮忙换一个变量名让他表意更明确
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| response = conn.indices.put_mapping( | ||
| index=doc.index_name, body=doc.doc_data_body | ||
| ) | ||
| successful_count = response.get("_shards", {}).get("successful", None) |
There was a problem hiding this comment.
The variable successful_count is not initialized when an exception occurs and the condition 'index_not_found_exception' in error_message.lower() is False. This will cause a NameError when trying to use it in the return statement at line 972. The variable should be initialized before the try block, similar to other methods like __update and __create_index.
| index=doc.index_name, body=doc.doc_data_body | ||
| ) | ||
| successful_count = response.get("_shards", {}).get("successful", None) | ||
| response_str = str(response) |
There was a problem hiding this comment.
The variable response_str is not initialized when an exception occurs and the condition 'index_not_found_exception' in error_message.lower() is False. This will cause a NameError when trying to use it in the return statement at line 970. The variable should be initialized before the try block.
| mockElasticsearch.return_value = mock_conn | ||
|
|
||
| workflow = Mock() | ||
| workflow.sqlworkflowcontent.sql_content = 'PUT /nonexistent_index/_mapping {"properties": {"new_field": {"type": "text"}}}' |
There was a problem hiding this comment.
[nitpick] This line exceeds typical line length conventions (appears to be >100 characters). Consider breaking the SQL string into a multi-line format for better readability, consistent with the formatting used in other test methods like test_execute_workflow_put_mapping_success.
| workflow.sqlworkflowcontent.sql_content = 'PUT /nonexistent_index/_mapping {"properties": {"new_field": {"type": "text"}}}' | |
| workflow.sqlworkflowcontent.sql_content = ( | |
| 'PUT /nonexistent_index/_mapping {"properties": {"new_field": {"type": "text"}}}' | |
| ) |
2 participants
Supports adding fields to existing indices in Elasticsearch during online operations