chore: replace RETURNING + fetch_one w/ execute + rows_affected#621
Open
chet wants to merge 1 commit intoNVIDIA:mainfrom
Open
chore: replace RETURNING + fetch_one w/ execute + rows_affected#621chet wants to merge 1 commit intoNVIDIA:mainfrom
chet wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
…iscarded results This was an incidental finding from NVIDIA#609. Basically we have a number of cases where we are doing an `UPDATE` with `RETURNING ...`, and then chain `.fetch_one()` to make sure an update occurred, and never actually use the returned value. No need for this. Instead, we can drop `RETURNING ...` and just chain `.rows_affected()`. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-03-18 18:24:08 UTC | Commit: 3625e66 |
🛡️ Vulnerability Scan🚨 Found 72 vulnerability(ies) Severity Breakdown:
🔗 View full details in Security tab 🕐 Last updated: 2026-03-18 18:24:16 UTC | Commit: 3625e66 |
chet
changed the title
ajf
approved these changes
Mar 19, 2026
Collaborator
ajf
left a comment
ajf
left a comment
There was a problem hiding this comment.
This makes the if statement pretty much unreadable imho since the conditional is on some row a bunch of lines down and hard to read.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This was an incidental observation from #609.
We have a number of cases where we are doing an
UPDATEwithRETURNING ..., and then chain.fetch_one()to make sure an update occurred, and never actually use the returned value. I think there was some elegance to this in the sense it was less code, but the downsides are:This is more lines of code, but clearly expresses our intent, doesn't discard values, and produces a clear
NotFoundError.FWIW this is probably debatable too. Maybe we'd prefer the other approach. Part of me likes it for the elegance, but I think it's part of the elegance that can leave you guessing (which is exactly why it was brought up in a code review to begin with).
Signed-off-by: Chet Nichols III chetn@nvidia.com
Type of Change
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes