[FlatPostgresCollection] Implement bulkCreateOrReplaceReturnOlder + drop + Bugfix for delete(key) always returning true#286
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #286 +/- ##
============================================
+ Coverage 81.30% 81.51% +0.21%
- Complexity 1521 1531 +10
============================================
Files 241 241
Lines 7305 7334 +29
Branches 700 702 +2
============================================
+ Hits 5939 5978 +39
+ Misses 924 915 -9
+ Partials 442 441 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Test Results 120 files + 7 120 suites +7 38s ⏱️ ±0s Results for commit 8a18b2d. ± Comparison against base commit 2fe1f90. This pull request removes 27 and adds 45 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
| @Override | ||
| public void drop() { | ||
| throw new UnsupportedOperationException(WRITE_NOT_SUPPORTED); | ||
| String dropTableSQL = String.format("DROP TABLE IF EXISTS %s", tableIdentifier); |
There was a problem hiding this comment.
do we really need this? For postgres we haven't been creating table using this route, deletion should probably not happen through this route.
There was a problem hiding this comment.
I added this method for completeness sake, since we have this for Mongo. If any direct clients want to use it, they'd have an option.
There was a problem hiding this comment.
I am ok with it being there. Might be useful just for tests.
The way I am thinking about it is: if drop is exposed here, then corresponding createTable should also happen via this route. A simple create is also fine. But any other DDL on the table will be very hard. So my personal recommendation is to have all of this happen outside or we build comprehensive ddl support.
There was a problem hiding this comment.
As discussed, table management is not supported via this interface, so I have removed this impl.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Description
This PR contains three changes:
FlatPostgresCollection#bulkUpserthad a bug where it would replace missing columns in the query withnull. It now has proper merge semantics.Collection#bulkCreateOrReplaceReturnOlderand implement it forFlatPostgresCollection. This is similar toFlatPostgresCollection#bulkUpsertReturnOlderbut with replace (instead of merge) semantics.FlatPostgresCollection#delete(key)would always returntrueeven if no rows were deleted - This was divergent with Mongo's behaviour. So this has been fixed.ImplementThis is now rolled back.FlatPostgresCollection#drop.Testing
Added integration tests + compatibility tests.
Checklist: