feat(soup): forward-looking due-date buckets for grouped queries - #5633
Open
kopertop wants to merge 1 commit into
Open
feat(soup): forward-looking due-date buckets for grouped queries#5633kopertop wants to merge 1 commit into
kopertop wants to merge 1 commit into
Conversation
First step toward an Asana-style "My Tasks" view: group tasks into Today / Upcoming / Later / Backlog by reading a Date-typed property forward from the viewer's current day. Backend only — no caller requests the new field yet. Neither existing grouping mode can do this: - `GroupByField::Date` buckets `sort_ts` *backwards* (Today / Yesterday / Last week) to answer "what did I touch recently". - `GroupByField::Property` extracts values by expanding `values->'value'` with `jsonb_array_elements`, but `PropertyValue::Date` is a JSON *scalar*, so that path yields NULL and files every task under "Not Set". So `DueDateBucket` gets its own scalar-reading lateral join, and its own bucketer in `models_grouping::gtd_buckets`. Generic over the property rather than pinned to the system due date, so a custom date property groups the same way. Two decisions worth review: - **Boundaries are computed in Rust, not SQL.** `CURRENT_DATE` is the database server's date (UTC), which runs a day ahead of any viewer in the Americas during their evening and would file tomorrow morning's work under Today. The field carries an IANA `time_zone`; an unrecognized value degrades to UTC rather than failing the request. `horizon_days` is likewise a parameter, so changing the Upcoming window later is a request change, not a migration. - **The comparison is textual, not cast.** `(values->>'value')::timestamptz` is only *stable*, not immutable, so Postgres rejects an index on it. Z-suffixed RFC 3339 compares lexicographically in chronological order, keeping a plain B-tree usable. The boundary literal deliberately omits the trailing `Z`: `'.' < 'Z'`, so a value with fractional seconds would otherwise sort before a Z-suffixed boundary it should sort after. Both properties are covered by tests, including one that executes the bucketing in Postgres and asserts it agrees with the Rust implementation. The group-by expression is also now generated from a single pinned `now` (`group_select_expr_at`), since it is emitted three times per query and a request landing on midnight would otherwise partition against one set of boundaries and count against another. Both transports get the variant (REST `ApiGroupByField`, GraphQL `GraphqlGroupByField`); `static_assets/schema.graphql` regenerated.
Author
|
Opened against the wrong repository by mistake — this belongs on the Newstex fork, not upstream. Closing. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Author
|
Reopening — closing this was my mistake, not the author's intent. This is a deliberate upstream proposal. |
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
1 participant
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.
First of a planned five-PR sequence toward an Asana-style My Tasks view (buckets → date filters and due-date sort → per-user manual placement → resolution in the grouping query → the
/my-tasksview). Backend only — nothing requests the new grouping field yet, so this is inert on its own.What it adds
GroupByField::DueDateBucket— group items into Today / Upcoming / Later / Backlog by reading aDate-typed property forward from the viewer's current day. Generic over the property rather than pinned to the system due date, so a custom date property groups the same way.Why neither existing grouping mode can do this
GroupByField::Datebucketssort_tsbackwards (Today / Yesterday / Last week) to answer "what did I touch recently".GroupByField::Propertyextracts values by expandingvalues->'value'withjsonb_array_elements, butPropertyValue::Dateis a JSON scalar — that path returns NULL for every row, so grouping tasks by due date today files all of them under "Not Set".Hence a separate bucketer (
models_grouping::gtd_buckets) and a separate scalar-reading lateral join.Two decisions worth review
Boundaries are computed in Rust, not SQL.
CURRENT_DATEis the database server's date. The testboundaries_follow_the_viewer_not_the_servershows the consequence: at 22:00 EDT the server is already on tomorrow's date, and a UTC-derived "end of today" files tomorrow-morning's work under Today. The field carries an IANAtime_zone; an unrecognized value degrades to UTC rather than failing the request, since it originates from a client.horizon_daysis a parameter too, so changing the Upcoming window later is a request change rather than a migration.The comparison is textual, not cast.
(values->>'value')::timestamptzcannot be indexed — text→temporal casts are only stable, not immutable — so PR 2's partial B-tree needs the raw text. Z-suffixed RFC 3339 compares lexicographically in chronological order, which makes that work.The boundary literal deliberately omits the trailing
Z, because'.' (0x2E) < 'Z' (0x5A):Against a bare prefix, any value inside that second sorts after it, so a midnight-exact due date falls on the later side — which is what "due tomorrow" means.
text_comparison_agrees_with_instant_comparisonasserts text and instant comparison agree across those cases, anda_z_suffixed_boundary_would_misbucket_fractional_valuespins the trap itself so nobody "tidies" the format back.Also here
group_select_expr_at/group_order_expr_attake an explicitnow. The key expression is emitted three times per query (select, partition, filter); generating them from one pinned moment stops a request landing on midnight from partitioning against one set of boundaries and counting against another. The old signatures remain, delegating toUtc::now().ApiGroupByField::DueDateBucketand GraphQLDUE_DATE_BUCKET.static_assets/schema.graphqlregenerated withcargo run -p complete_graph --bin graphql_schema;without_property_optionsnow rejectstimeZone/horizonDayson modes that read no property.Verification
Passing locally:
models_grouping19 tests + 2 doctests,complete_graph27 SDL tests,cargo fmt,clippyclean on all three crates. Rebased onto currentmain(26e94d8) with no conflicts, and regenerating the schema against that base produces no diff.Not run locally: the 8 unit tests and 1 Postgres test added to
crates/soup/.../grouping/test.rs.crates/soupgatespub mod outboundbehind a cargo feature, socargo check --testsnever compiles that module; with the feature on, the test binary needs a live database because pre-existingsqlx::query!calls in test code aren't in the offline.sqlxcache. No Docker runtime on my machine. CI is the first real build of those tests. My one new insert uses the uncheckedsqlx::query()form on purpose, so this adds no.sqlxcache requirement.The Postgres test is the one that matters: it executes the generated
CASEagainst seeded rows — overdue, last instant of today, midnight with and without fractional seconds, past the horizon — and asserts Postgres and the Rust implementation reach the same bucket for each.Deliberately out of scope
entity_propertiesdue-date index lands in PR 2, alongside the filter that needs it.openapi.jsonand the generated TS client aren't regenerated: they're fetched from a running service (localhost:8086/api-doc/openapi.json, seeapps/web/scripts/services.ts), and nothing consumes the new variant until PR 5.sort_ts DESC; due-date sort is PR 2.