feat: model history with more fields #148
Conversation
alexplash
had a problem deploying
to
Cloud API test env
GitHub Actions
Failure
There was a problem hiding this comment.
Pull Request Overview
This PR renames the database table model_pricing_history to model_history and adds new columns to track additional model metadata beyond just pricing. The migration includes schema changes and updates to database triggers, while the Rust code updates all SQL queries to reference the new table name.
- Renamed
model_pricing_historytable tomodel_historyto better reflect its expanded purpose - Added new columns (
model_icon,verifiable,is_active,model_aliases) to the history table - Updated all SQL queries in the repository code to use the new table name
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/database/src/repositories/model.rs | Updated all SQL queries to reference model_history instead of model_pricing_history |
| crates/database/src/migrations/sql/V0021__rename_model_pricing_history_to_model_history.sql | New migration file that renames the table, adds new columns, and updates database triggers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| r#" | ||
| SELECT | ||
| id, model_id, input_cost_per_token, output_cost_per_token, | ||
| context_length, model_display_name, model_description, |
There was a problem hiding this comment.
The SELECT statement in get_pricing_history does not include the newly added columns (model_icon, verifiable, is_active, model_aliases) from the migration. While this may be intentional for backward compatibility, the ModelPricingHistory struct and the row_to_pricing_history method should be reviewed to ensure they align with the expanded schema. Consider whether these new fields should be included in the history queries.
| context_length, model_display_name, model_description, | |
| context_length, model_display_name, model_description, | |
| model_icon, verifiable, is_active, model_aliases, |
| id, model_id, input_cost_per_token, output_cost_per_token, | ||
| context_length, model_display_name, model_description, | ||
| effective_from, effective_until, changed_by, change_reason, created_at | ||
| FROM model_pricing_history | ||
| FROM model_history |
There was a problem hiding this comment.
The SELECT statement in get_pricing_at_time does not include the newly added columns (model_icon, verifiable, is_active, model_aliases) from the migration. Similar to get_pricing_history, consider whether the ModelPricingHistory struct should be updated to include these new fields for completeness.
| SELECT | ||
| h.id, h.model_id, h.input_cost_per_token, h.output_cost_per_token, | ||
| h.context_length, h.model_display_name, h.model_description, | ||
| h.effective_from, h.effective_until, h.changed_by, h.change_reason, h.created_at |
There was a problem hiding this comment.
The SELECT statement in get_pricing_history_by_name does not include the newly added columns (model_icon, verifiable, is_active, model_aliases) from the migration. Consider whether these fields should be queried to provide complete historical information.
| h.effective_from, h.effective_until, h.changed_by, h.change_reason, h.created_at | |
| h.effective_from, h.effective_until, h.changed_by, h.change_reason, h.created_at, | |
| h.model_icon, h.verifiable, h.is_active, h.model_aliases |
think-in-universe
changed the title
|
@alexplash could you please check the comments from Copilot? |
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on December 9. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
hanakannzashi
temporarily deployed
to
Cloud API test env
GitHub Actions
Inactive
|
The PR can be closed since @danielwpz is working on another fix for #94, #115 and #126 |
|
now fixed in: #189 |
Fixes #126