Update import timestamp after spanner ignestion#586
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request shifts the setting of DataImportTimestamp in Spanner from the staging phase to when the ingestion status is successfully updated, and adds corresponding unit tests. The feedback suggests refactoring the SQL update query to construct it dynamically, which reduces code duplication and improves maintainability.
| if status == 'SUCCESS': | ||
| update_sql = ( | ||
| "UPDATE ImportStatus SET State = @importStatus, WorkflowId = @workflowId, " | ||
| "StatusUpdateTimestamp = PENDING_COMMIT_TIMESTAMP(), " | ||
| "DataImportTimestamp = PENDING_COMMIT_TIMESTAMP() " | ||
| "WHERE ImportName IN UNNEST(@importNames)" | ||
| ) | ||
| else: | ||
| update_sql = ( | ||
| "UPDATE ImportStatus SET State = @importStatus, WorkflowId = @workflowId, " | ||
| "StatusUpdateTimestamp = PENDING_COMMIT_TIMESTAMP() " | ||
| "WHERE ImportName IN UNNEST(@importNames)" | ||
| ) |
There was a problem hiding this comment.
The SQL update query is duplicated with only a minor difference (the addition of DataImportTimestamp when status == 'SUCCESS'). We can construct the query dynamically to avoid duplication and improve maintainability.
set_clauses = [
"State = @importStatus",
"WorkflowId = @workflowId",
"StatusUpdateTimestamp = PENDING_COMMIT_TIMESTAMP()"
]
if status == 'SUCCESS':
set_clauses.append("DataImportTimestamp = PENDING_COMMIT_TIMESTAMP()")
update_sql = f"UPDATE ImportStatus SET {', '.join(set_clauses)} WHERE ImportName IN UNNEST(@importNames)"
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| UnusedCode | 1 medium |
| ErrorProne | 1 high |
| Security | 1 medium |
🟢 Metrics 7 complexity
Metric Results Complexity 7
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
2 participants
DataImportTimestamp currently tracks the time when the data is available in STAGING state (after batch job). It may be more meaningful to use this column to reflect the time for spanner ingestion.