-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[CHORE]: Remove next_run from attached_functions #5871
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
d730c23 to
e7dde61
Compare
65ac80f to
7bfacfe
Compare
7bfacfe to
47cb225
Compare
47cb225 to
ecd3278
Compare
e7dde61 to
72e8a81
Compare
|
Drop This PR removes the Key Changes• SQL migration Affected Areas• This summary was automatically generated by @propel-code-bot |
ecd3278 to
7de4094
Compare
72e8a81 to
efc6ae0
Compare
7de4094 to
4065355
Compare
efc6ae0 to
5b79840
Compare
5b79840 to
f655c4c
Compare
4065355 to
9eb8e4c
Compare
9eb8e4c to
ec6e7e3
Compare
f655c4c to
c0575ef
Compare
6b053a4 to
9eb8e4c
Compare
9eb8e4c to
7312e93
Compare
f655c4c to
c0575ef
Compare
| id: attached_function.id.0.to_string(), | ||
| name: attached_function.name.clone(), | ||
| function_name: attached_function.function_id.to_string(), | ||
| function_id: attached_function.function_id.to_string(), | ||
| input_collection_id: attached_function.input_collection_id.0.to_string(), | ||
| output_collection_name: attached_function.output_collection_name.clone(), | ||
| output_collection_id: attached_function | ||
| .output_collection_id | ||
| .as_ref() | ||
| .map(|id| id.0.to_string()), | ||
| params: parse_params(attached_function.params.as_deref()), | ||
| completion_offset: attached_function.completion_offset, | ||
| min_records_for_invocation: attached_function.min_records_for_invocation, | ||
| tenant_id: attached_function.tenant_id.clone(), | ||
| database_id: attached_function.database_id.clone(), | ||
| next_run_at: system_time_to_micros(attached_function.next_run), | ||
| created_at: system_time_to_micros(attached_function.created_at), | ||
| updated_at: system_time_to_micros(attached_function.updated_at), | ||
| function_id: attached_function.function_id.to_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.
[BestPractice]
The function_id field is being added at line 718 after updated_at, but this changes the field ordering compared to the original code where it appeared earlier (line 705). While Go struct initialization with named fields isn't order-dependent for correctness, this inconsistency makes the code harder to review.
The original placement (after function_name, before input_collection_id) was more logical since it groups function-related fields together. Consider maintaining the original field order for consistency:
Suggested Change
| id: attached_function.id.0.to_string(), | |
| name: attached_function.name.clone(), | |
| function_name: attached_function.function_id.to_string(), | |
| function_id: attached_function.function_id.to_string(), | |
| input_collection_id: attached_function.input_collection_id.0.to_string(), |
⚡ Committable suggestion
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Context for Agents
[**BestPractice**]
The `function_id` field is being added at line 718 after `updated_at`, but this changes the field ordering compared to the original code where it appeared earlier (line 705). While Go struct initialization with named fields isn't order-dependent for correctness, this inconsistency makes the code harder to review.
The original placement (after `function_name`, before `input_collection_id`) was more logical since it groups function-related fields together. Consider maintaining the original field order for consistency:
<details>
<summary>Suggested Change</summary>
```suggestion
id: attached_function.id.0.to_string(),
name: attached_function.name.clone(),
function_name: attached_function.function_id.to_string(),
function_id: attached_function.function_id.to_string(),
input_collection_id: attached_function.input_collection_id.0.to_string(),
```
⚡ **Committable suggestion**
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
</details>
File: rust/sysdb/src/test_sysdb.rs
Line: 718| -- Remove next_run column from attached_functions table as it's no longer needed | ||
| ALTER TABLE "public"."attached_functions" DROP COLUMN "next_run"; |
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.
[CompanyBestPractice]
This migration removes a column from the attached_functions table, which is a potentially unsafe operation. According to our SQL migration guidelines, removing a column requires careful consideration:
- Backward compatibility: Existing application code still referencing
next_runwill fail after this migration runs - Zero-downtime deployment: If using blue-green or rolling deployments, old instances will crash when the column disappears
- No rollback path: The
ALTER TABLE DROP COLUMNis irreversible without restoring from backup
Recommended approach for safer column removal:
- First deploy: Stop writing to the column, ignore reads
- Second deploy (after monitoring): Remove column references from code
- Third deploy: Run migration to drop column
Alternatively, document that this is a breaking change requiring complete system shutdown during deployment.
Context for Agents
[**CompanyBestPractice**]
This migration removes a column from the `attached_functions` table, which is a potentially unsafe operation. According to our SQL migration guidelines, removing a column requires careful consideration:
1. **Backward compatibility**: Existing application code still referencing `next_run` will fail after this migration runs
2. **Zero-downtime deployment**: If using blue-green or rolling deployments, old instances will crash when the column disappears
3. **No rollback path**: The `ALTER TABLE DROP COLUMN` is irreversible without restoring from backup
Recommended approach for safer column removal:
1. First deploy: Stop writing to the column, ignore reads
2. Second deploy (after monitoring): Remove column references from code
3. Third deploy: Run migration to drop column
Alternatively, document that this is a breaking change requiring complete system shutdown during deployment.
File: go/pkg/sysdb/metastore/db/migrations/20251114134400.sql
Line: 2
Description of changes
Summarize the changes made by this PR.
next_runcolumnTest plan
How are these changes tested?
pytestfor python,yarn testfor js,cargo testfor rustMigration plan
Are there any migrations, or any forwards/backwards compatibility changes needed in order to make sure this change deploys reliably?
Observability plan
What is the plan to instrument and monitor this change?
Documentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the _docs section?_