feat: auto alter table during trace ingestion from int to float#7871
feat: auto alter table during trace ingestion from int to float#7871shuiyisong merged 4 commits intoGreptimeTeam:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness of OTLP trace ingestion by introducing dynamic type handling. It enables the system to automatically adapt table schemas to accommodate varying data types from incoming traces, specifically by widening integer columns to floating-point when necessary and performing other compatible type coercions. This prevents ingestion failures and ensures data integrity, making the trace ingestion process more resilient to schema mismatches. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive type coercion and widening capabilities for OTLP trace data ingestion. It enables in-batch type conversions for compatible types (e.g., Int64 to Float64, String to numeric/boolean) and supports cross-batch column widening from Int64 to Float64 by issuing ALTER TABLE statements. New utility functions and integration tests are included to support these features. A review comment highlights a potential data loss or inconsistency issue in the coerce_trace_numeric_columns function due to incorrect handling of missing values and short rows, suggesting a fix to ensure all rows are processed and padded correctly.
Signed-off-by: shuiyisong <xixing.sys@gmail.com>
46afe01 to
35d8623
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements automatic column widening for OTLP traces, specifically allowing existing Int64 columns to be widened to Float64 when incoming data contains floating-point values. It introduces a reconciliation decision mechanism to determine whether to use existing types, request-local types, or perform an alter operation. The ingestion logic was updated to collect and execute these schema changes before processing row data. Feedback was provided regarding the error handling in the new alter_trace_table_columns_to_float64 method, suggesting that the original error's status code and context should be preserved rather than being wrapped in a generic InternalSnafu.
|
@codex review |
Signed-off-by: shuiyisong <xixing.sys@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8121d784aa
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: shuiyisong <xixing.sys@gmail.com>
Signed-off-by: shuiyisong <xixing.sys@gmail.com>
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
#7860
What's changed and what's your intention?
Note: merge #7870 firstThis is the second PR for solving issue 7860:
Note: concurrent update should work but have not yet been tested
PR Checklist
Please convert it to a draft if some of the following conditions are not met.