Skip to content

docs(acceleration): on_zero_results use_source fallback excludes rows removed by retention - #2348

Open
spiceemma wants to merge 4 commits into
trunkfrom
emma/docs-sync-14783
Open

spiceemma wants to merge 4 commits into
trunkfrom
emma/docs-sync-14783

Conversation

@spiceemma

@spiceemma spiceemma commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

spiceai/spiceai#14783 applies the inverse of a dataset's retention policy to the on_zero_results: use_source fallback query, so a row that retention_sql or retention_period evicted from the acceleration is no longer read back from the source. When the inverse cannot be planned against the source schema, the dataset now fails to load. Before this change, the docs listed retention policies as a reason to enable use_source without saying that the fallback would return soft-deleted and expired rows, and the "Accelerating application logs" example implied every log older than the refresh window falls back to the source.

Pages changed

  • website/docs/features/data-acceleration/data-refresh.md: new "Fallback with a Retention Policy" subsection under Behavior on Zero Results (three-valued inverse, NULL time_column kept, applies even with retention_check_enabled: false, load-time refusal and its error text, fix); a pointer sentence in the zero-results intro; item 5 of the application-logs example now notes the 7-day retention cutoff.
  • website/docs/reference/spicepod/datasets.md: acceleration.on_zero_results notes the retention inverse and the load failure, linking to the new subsection.

Versioned docs: vNext only. git tag --contains d2922159ca returns no tags, so no released version has this behavior and website/versioned_docs/ is unchanged.

Verification

  • Review round 2 (both items confirmed on trunk, code inspection only): with a scheduled retention check on a non-Cayenne engine, the fallback inverse uses the partition-AND cutoff that includes time_partition_column; Cayenne, or no scheduled check, uses time_column alone — let apply_time_column_keep = !declared_retention_runs || acceleration_settings.engine == Engine::Cayenne; at crates/runtime/src/datafusion/mod.rs:4177. In caching mode no fallback keep is built — && refresh_mode != RefreshMode::Caching at crates/runtime-table/src/accelerated/mod.rs:1318.

  • Reproduced: no release contains the change — git merge-base --is-ancestor <merge of #14783> v2.4.0-rc.1 returns false, and v2.4.0-rc.1 is the newest tag (https://github.com/spiceai/spiceai/tags), so the page names "Spice v2.4.0-rc.1 and earlier releases".

Trunk SHA: f23a6aa686ddff43ae5a73effa36087a3b8425f9.

  • Reproduced: load-time error text: "Failed to register dataset '{dataset_name}' ({connector}): \on_zero_results: use_source` cannot be combined with this retention configuration, so the dataset was not loaded. ..."` (crates/runtime/src/datafusion/mod.rs:289)
  • Reproduced: suggested fix text: "Remove \on_zero_results: use_source`, or use a `retention_sql` / `retention_period` predicate that can be evaluated as a filter on the source columns."` (crates/runtime/src/datafusion/mod.rs:289)
  • Reproduced: unsupported time-column cause: "time-based retention compares \{time_column}` against the cutoff, but that column cannot be used as a source filter (unsupported type)"` (crates/runtime-table/src/accelerated/fallback_retention.rs:39)
  • Reproduced: inverse is skipped in caching mode: ) && !matches!(refresh_mode, RefreshMode::Caching) (crates/runtime/src/datafusion/mod.rs:4166)
  • Reproduced: time inverse applied when no scheduled worker runs (or on Cayenne): !declared_retention_runs || acceleration_settings.engine == Engine::Cayenne; (crates/runtime/src/datafusion/mod.rs:4178)
  • Observed, not isolated: fallback hides soft-deleted and time-expired rows on DuckDB, Arrow (write-time retention_sql, worker off), and Cayenne (worker on and off), and a never-loaded recent row still falls back: source PR integration tests in crates/runtime/tests/acceleration/on_zero_results_retention.rs, fix(acceleration): keep retention-evicted rows out of the on_zero_results source fallback spiceai#14783
  • Observed, not isolated: keep_expr three-valued logic and NULL timestamps kept: source PR unit tests, fix(acceleration): keep retention-evicted rows out of the on_zero_results source fallback spiceai#14783
  • Unverified, code inspection only: the retention_sql inverse applies when retention_check_enabled is false (from_configured takes retention_delete_expr, which is parsed whenever retention_sql is set).
  • Unverified, code inspection only: the application-logs example returns no row for a log older than 7 days.
  • cd website && npm run build at 01936ca: [SUCCESS] Generated static files in "build". exit=0 (no broken links or anchors)

@spiceemma
spiceemma requested a review from a team as a code owner October 11, 2026 08:29
Copilot AI balanced review requested due to automatic review settings October 11, 2026 08:29
@spiceemma spiceemma self-assigned this Oct 11, 2026
@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

✅ Pull with Spice Passed

Passing checks:

  • ✅ Title meets minimum length requirement (10 characters)
  • ✅ Has at least one of the required labels: area/blog, area/docs, area/cookbook, dependencies
  • ✅ No banned labels detected
  • ✅ Has at least one assignee: spiceemma

@github-actions

Copy link
Copy Markdown
Contributor

🔍 Pull with Spice Failed

Passing checks:

  • ✅ Title meets minimum length requirement (10 characters)
  • ✅ Has at least one of the required labels: area/blog, area/docs, area/cookbook, dependencies
  • ✅ No banned labels detected

Failed checks:

  • ❌ At least one assignee is required for this pull request.

Please address these issues and update your pull request.

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Pull with Spice Failed

Passing checks:

  • ✅ Title meets minimum length requirement (10 characters)
  • ✅ Has at least one of the required labels: area/blog, area/docs, area/cookbook, dependencies
  • ✅ No banned labels detected

Failed checks:

  • ❌ At least one assignee is required for this pull request.

Please address these issues and update your pull request.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The application-log example misstates fallback behavior, and release applicability needs a specific version.

1 open finding
What changed in this PR

Documents retention-aware source fallback behavior for accelerated datasets.

Changes:

  • Explains retention predicate inversion and load-time failures.
  • Updates the application-log example and dataset reference.
File Description
data-refresh.md Documents fallback retention behavior and examples.
datasets.md Adds retention details to on_zero_results.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread website/docs/features/data-acceleration/data-refresh.md Outdated
@github-actions

Copy link
Copy Markdown
Contributor

🚀 deployed to https://d0e7fbd6.spiceai-org-website.pages.dev

Copilot AI balanced review requested due to automatic review settings October 11, 2026 08:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The documentation omits partition-column semantics and overstates behavior in caching mode.

0 open findings

1 resolved since last review
Previously missed (2)

In code that hasn't changed since last review

Low severity Document time_partition_column effects on retention fallback filtering

website/​docs/​features/​data-acceleration/​data-refresh.md:509

The retention_period description omits time_partition_column. When a scheduled retention worker runs on a non-Cayenne engine, the delete predicate uses both time columns, so its inverse can keep a row with an expired time_column when the partition value is recent or NULL. Cayenne and unscheduled policies use only time_column; document this distinction so readers can predict fallback results.

Low severity Scope on_zero_results behavior to supported use_source modes

website/​docs/​reference/​spicepod/​datasets.md:1024

on_zero_results is ignored in refresh_mode: caching, and the implementation skips both retention-inverse validation and fallback filtering there. This sentence currently states the behavior for any dataset with retention; scope it to use_source in the supported modes so caching users do not expect origin reads to honor this filter.

🧠 Review effort: Balanced

@github-actions

Copy link
Copy Markdown
Contributor

🚀 deployed to https://50ea7b9b.spiceai-org-website.pages.dev

Copilot AI balanced review requested due to automatic review settings October 11, 2026 08:51
@spiceemma

Copy link
Copy Markdown
Contributor Author

Addressed both items from the latest review in c660c47: the retention_period paragraph now covers time_partition_column (partition-AND inverse on non-Cayenne engines with a scheduled check; time_column alone on Cayenne or without a scheduled check, per crates/runtime/src/datafusion/mod.rs:4177), and the datasets.md note is scoped to on_zero_results: use_source and says caching mode ignores it (crates/runtime-table/src/accelerated/mod.rs:1316-1318).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The application-logs example still incorrectly states that every log older than the refresh window falls back.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Low severity Clarify fallback applies only when log is absent from acceleration

website/​docs/​features/​data-acceleration/​data-refresh.md:915

This still implies that every log older than the one-day refresh window falls back. In append mode, refresh_data_window limits what each refresh reads but does not evict rows already accelerated; a log loaded while recent can remain local until the seven-day retention cutoff. Qualify the fallback as applying only when the requested log is absent from the acceleration.

🧠 Review effort: Balanced

@github-actions

Copy link
Copy Markdown
Contributor

🚀 deployed to https://dd3793e4.spiceai-org-website.pages.dev

Copilot AI balanced review requested due to automatic review settings October 11, 2026 09:00
@spiceemma

Copy link
Copy Markdown
Contributor Author

Addressed the previously-missed item in 8bee069: item 5 now falls back only for a log that is not in the acceleration, and notes that a log loaded within the last day stays until the retention check removes it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approved

The documentation matches the implemented retention fallback behavior and includes valid cross-references.

0 open findings

🧠 Review effort: Balanced

@github-actions

Copy link
Copy Markdown
Contributor

🚀 deployed to https://e72f2860.spiceai-org-website.pages.dev

This branch was successfully deployed

1 active deployment
preview — 8bee0692 Deployed Oct 11, 2026 by github-actions[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants