Support for Unnest on Top-Level Array Fields in Flat Postgres Collection #239
Conversation
suddendust
requested review from
avinashkolluru,
kotharironak,
skjindal93 and
suresh-prakash
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #239 +/- ##
=============================================
- Coverage 79.69% 57.17% -22.52%
Complexity 1097 1097
=============================================
Files 212 212
Lines 5328 5364 +36
Branches 450 463 +13
=============================================
- Hits 4246 3067 -1179
- Misses 762 2014 +1252
+ Partials 320 283 -37
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:
|
suddendust
changed the title
| // From looking at the data: | ||
| // - "hygiene" appears in docs 1, 5, 8 = 3 times | ||
| // - "personal-care" appears in docs 1, 3 = 2 times | ||
| // - "grooming" appears in docs 6, 7 = 2 times |
There was a problem hiding this comment.
This is nice. Thanks a lot for these inline comments. 🙂
| return String.format( | ||
| "EXISTS (SELECT 1 FROM jsonb_array_elements(COALESCE(%s, '[]'::jsonb)) AS \"%s\" WHERE %s)", | ||
| parsedLhs, alias, parsedFilter); | ||
| if (isFlatCollection) { |
There was a problem hiding this comment.
Apologies if this comment is coming pretty late.
The more and more of these conditions make me feel that it would be better to separate the "FlatCollection Visitors" and "NestedCollection Visitors" in different classes backed by some factory, avoiding a lot of these checks. More than the checks, it'll be much cleaner to maintain/debug.
Having said that, I leave it upto you if the distinction really makes sense.
There was a problem hiding this comment.
Sure, this makes sense. Let me see how much of this can be refactored. Thanks!
There was a problem hiding this comment.
Apologies if this comment is coming pretty late. The more and more of these conditions make me feel that it would be better to separate the "FlatCollection Visitors" and "NestedCollection Visitors" in different classes backed by some factory, avoiding a lot of these checks. More than the checks, it'll be much cleaner to maintain/debug.
Having said that, I leave it upto you if the distinction really makes sense.
+1
There was a problem hiding this comment.
@puneet-traceable @suresh-prakash I did a bit of refactoring and the changes are substantial. How about we merge this PR and take that up in a separate one? To keep things cleaner.
| postgresQueryParser.getQuery().getFilter(), postgresQueryParser); | ||
|
|
||
| if (StringUtils.isNotEmpty(unnestFilters) && mainFilter.isPresent()) { | ||
| return Optional.of(unnestFilters + " AND " + mainFilter.get()); |
There was a problem hiding this comment.
Nit: Prefer StringBuilder, StringBuffer or String.format()
3 participants
Description
This PR adds support for un-nesting top-level array fields in flat PG collection. Currently, it assumes that the column defined in
UnnestExpressionis an array. Un-nesting JSON arrays in currently not support in flat collections.Testing
Have added
DocStoreQueryV1Test#testFlatPostgresCollectionUnnestTagsto test the behaviour.Checklist: