Address review feedback from #4051#4646
Merged
Merged
Conversation
Replace the opaque `@type row :: list()` with a definition that documents the `[key, tags, json]` shape and element types. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016YnwhDEWjjZhPtUptmDX3L
Both are boolean fields, so use the `?` suffix convention. Renames the keys across the Decomposer producer and all consumers (DnfPlan, Querying, Shape, SubqueryIndex) plus tests. The :negated/:positive polarity atoms, the is_subquery?/1 function, and the is_subquery_shape? option are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016YnwhDEWjjZhPtUptmDX3L
The existing rescue clause already handles a missing table; lookup_element/3 also raises ArgumentError for a missing key, so it replaces the explicit case on :ets.lookup/2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016YnwhDEWjjZhPtUptmDX3L
On success the function may send a {:global_last_seen_lsn, lsn} message to
the caller, which callers are expected to handle in handle_info/2.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016YnwhDEWjjZhPtUptmDX3L
The {:init_consumer, config} continuation is never dispatched; shape
initialization now happens in the handle_info({:initialize_shape, ...})
clause, which performs the equivalent work.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016YnwhDEWjjZhPtUptmDX3L
The 400 response carries a descriptive message under the "where" field; assert against it so the expected text is documented in the test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016YnwhDEWjjZhPtUptmDX3L
Flatten each conversion into the reverse-order accumulator as it is produced, so the accumulator head is always the last emitted change. Marking it before the single Enum.reverse/1 removes the List.flatten/1 pass plus the List.pop_at(-1) + ++ rebuild that mark_last_change/1 did. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016YnwhDEWjjZhPtUptmDX3L
Replace the repeated `:crypto.hash(:md5, stack_id <> handle <> value) |> Base.encode16(case: :lower)` invocation (12 call sites) with a single value_tag/3 helper. It independently mirrors SubqueryTags.make_value_hash_raw/3 so assertions document the expected wire format rather than reusing the code under test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016YnwhDEWjjZhPtUptmDX3L
robacourt
force-pushed
the
rob/subqueries-feedback
branch
from
2a7fff5 to
dbccd71
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4646 +/- ##
==========================================
- Coverage 59.70% 59.67% -0.04%
==========================================
Files 385 385
Lines 43021 43021
Branches 12374 12374
==========================================
- Hits 25684 25671 -13
- Misses 17259 17272 +13
Partials 78 78
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Summary
Follow-up to the merged subqueries PR #4051, addressing reviewer (@alco) feedback that came in after merge. Each commit maps 1:1 to a review comment so they can be reviewed independently.
Changes
8827e20Make storagerow()type reflect actual value typefd7748eRenameposition_infoboolean fields tosubquery?/negated?91f8f17Use:ets.lookup_element/3inget_last_broadcast_lsndd8cb5cDocumentself()message sent bysubscribe_to_global_lsn_updates82ecbc2Remove dead:init_consumerhandle_continueclause1545bb3Assert mixed-polarity subquery error message in router test6aa7581Mark last converted change before reversing2a7fff5Extractvalue_tag/3helper in router testThese are internal-only changes (type docs, field renames, a dead-code removal, an ETS lookup simplification, test assertions/helpers, and a list-building cleanup) with no user-facing behavior change, so no changeset is included.
A few comments were discussed and intentionally left unchanged: the
Stream.concatsuggestion (r3131472813, the accumulated lists are tiny andreverse |> flattenis clearer) and theBETWEENquestion (r3131209403, confirmed as an intentional latent-bug fix required by this PR'sname-based DNF decomposition / SQL generation).🤖 Generated with Claude Code