-
Notifications
You must be signed in to change notification settings - Fork 295
feat: add support for subqueries behind a feature flag #3005
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
✅ Deploy Preview for electric-next ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
09e78d3 to
ac089ee
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3005 +/- ##
==========================================
- Coverage 78.70% 77.48% -1.22%
==========================================
Files 163 167 +4
Lines 8476 8725 +249
Branches 282 283 +1
==========================================
+ Hits 6671 6761 +90
- Misses 1803 1962 +159
Partials 2 2
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:
|
msfstef
left a comment
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.
Nice work, love that it's fairly contained!
I've left some comments mostly about 1) some lookups that I think might be avoidable and 2) the Shape.new code becoming very hard to follow and understand.
Other limitations are known, I did want to highlight the blocking of ShapeCache for materializing subqueries which feels like it should be noted even for the feature flag.
packages/sync-service/lib/electric/replication/shape_log_collector.ex
Outdated
Show resolved
Hide resolved
| for layer <- DependencyLayers.get_for_handles(state.dependency_layers, affected_shapes) do | ||
| # Each publish is synchronous, so layers will be processed in order | ||
| layer | ||
| |> Enum.map(&Map.fetch!(state.pids_by_shape_handle, &1)) |
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.
Do we need pids_by_shape_handle? Would it not be enough to just use Consumer.name(stack_id, shape_handle) instead and avoid maintaining another lookup?
10cbf57 to
03bb0d1
Compare
|
ChatGPT Pro's review https://chatgpt.com/share/68a4839c-5294-800c-9d71-b79d0e231ae3 |
|
benchmark this |
This PR allows initial, under-optimised implementation of where clauses in the form of
column IN (SELECT id FROM parent WHERE ...)It has some limitations:
This PR is mergable because it doesn't change current behaviour, but adds a feature flag as ENV variable that allows creations of these shapes, to allow us to iterate in main codebase and make this reviewable piece by piece