BDMS-520-1-1-Cleanup-2.0#445
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors database operations in the well transfer process to use bulk inserts instead of bulk_save_objects, improving performance and code organization. The changes also add a new integer coercion utility function for sensor transfer data validation.
Changes:
- Refactored
_after_hook_chunkto return dictionaries grouped by model type instead of flat object lists - Replaced
bulk_save_objectswith SQLAlchemyinsert()statements for better performance - Added
_coerce_wi_intutility function and applied it to sensor deployment fields
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| transfers/well_transfer.py | Refactored to use bulk insert operations with dict payloads; added _model_to_dict helper function |
| transfers/sensor_transfer.py | Added _coerce_wi_int function and applied it to WI fields; simplified _coerce_wi_mic_gain |
| tests/test_sensor_transfer.py | Added comprehensive tests for _coerce_wi_int function |
Comments suppressed due to low confidence (1)
transfers/sensor_transfer.py:1
- Import statements should follow the standard order: standard library imports first, then third-party imports, then local application imports. Move the local import from line 1 to after the third-party imports (lines 2-3).
# ===============================================================================
| except TypeError: | ||
| pass | ||
| try: | ||
| return bool(int(float(value))) |
There was a problem hiding this comment.
The _coerce_wi_mic_gain function now removes the string-based boolean parsing (e.g., 'true', 'false', 'yes', 'no') and only handles numeric values. If the source data contains string boolean representations, this change could cause those values to be coerced to None instead of being properly converted. Verify that the source data only contains numeric boolean representations.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fbd2bbf30
ℹ️ 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".
| for column in mapper.columns: | ||
| key = column.key | ||
| if column.primary_key and column.autoincrement: | ||
| continue | ||
| data[key] = getattr(obj, key) |
There was a problem hiding this comment.
The new _model_to_dict includes every mapped column value via getattr, which will be None for audit fields like created_at (defined with server_default and nullable=False in db/base.py::AuditMixin). When those dicts are passed to session.execute(insert(model), rows), SQL inserts explicit NULLs, bypassing server defaults and likely violating NOT NULL constraints or losing automatic timestamps. This didn’t happen with ORM bulk saves because unset attributes are omitted; you’ll need to drop unset/defaulted fields from the dict (or otherwise let server defaults apply) before bulk inserting.
Useful? React with 👍 / 👎.
Why
This PR addresses the following problem / context:
How
Implementation summary - the following was changed / added / removed:
Notes
Any special considerations, workarounds, or follow-up work to note?