-
Notifications
You must be signed in to change notification settings - Fork 1
fix: some issues with data output to postgresql #63
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
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.
Pull request overview
This PR addresses issues with PostgreSQL data output functionality by updating the runner metadata structure, improving exception handling in the channel splitter, and adding a replace parameter for PostgreSQL operations.
Key Changes
- Updated runner metadata structure to use nested
lastRunInfoobject instead of directlastRunIdfield - Modified exception handling in
ChannelSpliterto re-raise exceptions when there's only one target - Added
replace=Falseparameter to PostgreSQL dump operation
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
cosmotech/coal/postgresql/runner.py |
Updated to access lastRunId from nested lastRunInfo object |
cosmotech/coal/store/output/postgres_channel.py |
Added replace=False parameter to dump operation |
cosmotech/coal/store/output/channel_spliter.py |
Changed list initialization style and added exception re-raising logic for single targets |
tests/unit/coal/test_postgresql/test_postgresql_runner.py |
Updated test data to match new nested metadata structure |
tests/unit/coal/test_store/test_output/test_channel_spliter.py |
Updated tests to expect exceptions to be raised for single target failures |
Comments suppressed due to low confidence (1)
cosmotech/coal/store/output/postgres_channel.py:38
- The delete method does not return a boolean value as expected by its type signature. The method should return True on success or False on failure to be consistent with the ChannelInterface contract and the calling code in ChannelSpliter that expects a boolean return value.
def delete(self):
# removing metadata will trigger cascade delete on real data
remove_runner_metadata_from_postgresql(self.configuration)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result = spliter.send() | ||
| # Assert | ||
| assert result is False | ||
|
|
||
| def test_delete_success(self): | ||
| """Test delete method when all targets succeed.""" |
Copilot
AI
Jan 7, 2026
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.
The assertion assert result is False will never be executed because it's placed after the line that raises an exception. When spliter.send() raises an exception, control immediately jumps to the exception handler, skipping this assertion. If the intent is to verify behavior when an exception is raised, this assertion should be removed.
| result = spliter.send() | |
| # Assert | |
| assert result is False | |
| def test_delete_success(self): | |
| """Test delete method when all targets succeed.""" | |
| spliter.send() | |
| def test_delete_success(self): | |
| """Test delete method when all targets succeed.""" | |
| """Test delete method when all targets succeed.""" |
| result = spliter.delete() | ||
| # Assert | ||
| assert result is False | ||
|
|
Copilot
AI
Jan 7, 2026
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.
The assertion assert result is False will never be executed because it's placed after the line that raises an exception. When spliter.delete() raises an exception, control immediately jumps to the exception handler, skipping this assertion. If the intent is to verify behavior when an exception is raised, this assertion should be removed.
| result = spliter.delete() | |
| # Assert | |
| assert result is False | |
| spliter.delete() |
| runner.get("id"), | ||
| runner.get("name"), | ||
| runner.get("lastRunId"), | ||
| runner.get("lastRunInfo").get("lastRunId"), |
Copilot
AI
Jan 7, 2026
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.
The code uses chained .get() calls on nested dictionaries without checking if lastRunInfo exists or is None. If runner.get("lastRunInfo") returns None, calling .get("lastRunId") on it will raise an AttributeError. Consider using safe navigation or adding a check to handle cases where the nested structure might be incomplete.
| conn.commit() | ||
| LOGGER.info(T("coal.services.postgresql.metadata_updated")) | ||
| return runner.get("lastRunId") | ||
| return runner.get("lastRunInfo").get("lastRunId") |
Copilot
AI
Jan 7, 2026
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.
The code uses chained .get() calls on nested dictionaries without checking if lastRunInfo exists or is None. If runner.get("lastRunInfo") returns None, calling .get("lastRunId") on it will raise an AttributeError. Consider using safe navigation or adding a check to handle cases where the nested structure might be incomplete.
| selected_tables=filter, | ||
| fk_id=run_id, | ||
| replace=False, | ||
| ) |
Copilot
AI
Jan 7, 2026
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.
The send method does not return a boolean value as expected by its type signature. The method should return True on success or False on failure to be consistent with the ChannelInterface contract and the calling code in ChannelSpliter that expects a boolean return value.
| ) | |
| ) | |
| return True |
No description provided.