Fix ai report docs and allow inheriting watermark for time range resolution - #9860
Open
pjain1 wants to merge 2 commits into
Open
Fix ai report docs and allow inheriting watermark for time range resolution#9860pjain1 wants to merge 2 commits into
pjain1 wants to merge 2 commits into
Conversation
…ples The AI resolver has always declared `explore`, `dimensions` and `measures` at the top level of the `ai` block, as decided during review of #8673, but the JSON schema and the docs generated from it still nested them under `context`. Since unknown resolver properties only produce a warning, the documented example silently dropped the explore. Also replaces the `iso_duration`/`iso_offset` examples, which the resolver rejects, with Rill time expressions, and documents the `time_zone` and `where` properties. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness risks around backwards compatibility for the previously documented ai.context.* shape and around cache invalidation for explore-derived watermarks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates AI report documentation/schema to reflect the actual resolver properties (notably explore as a top-level field) and extends watermark inheritance so time range expressions can resolve against the latest data when an AI report targets an Explore.
Changes:
- Add
ResourceWatermarksupport forexploreresources by delegating to the underlying metrics view’s watermark. - Update parser behavior/tests to record an Explore ref for AI reports when
data.ai.exploreis set. - Align JSON schema + docs examples/fields for AI report configuration (remove deprecated ISO fields, move
exploreout ofcontext, documenttime_zone, etc).
File summaries
| File | Description |
|---|---|
| runtime/queries/resource_watermark.go | Add explore watermark resolution via underlying metrics view |
| runtime/queries/resource_watermark_test.go | Test explore watermark inherits from its metrics view |
| runtime/parser/schema/project.schema.yaml | Fix AI report schema/docs example: explore at top-level; update time_range/comparison_time_range fields |
| runtime/parser/parser_test.go | Add parser coverage asserting report refs include Explore when data.ai.explore is set |
| runtime/parser/parse_partial_data.go | Add Explore as a dependency ref for AI resolver configs |
| docs/docs/reference/project-files/reports.md | Update AI report docs to match actual resolver props (no context.explore) |
| docs/docs/reference/project-files/models.md | Sync AI config docs to the updated schema/field set |
| docs/docs/reference/project-files/alerts.md | Sync AI config docs to the updated schema/field set |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Approval assessments differ, and the AI report example has an unresolved watermark inconsistency.
Review details
Suppressed comments (1)
runtime/parser/schema/project.schema.yaml:2884
- The corrected AI report example still omits
watermark: inherit, so itslatestexpressions remain anchored to the scheduled trigger time rather than the explore's data watermark. The report always passes an execution time to the AI resolver, andExecutor.ResolveTimeRangereplaces the metrics-view watermark with that value; only inherited watermark makes that value data-based. Addwatermark: inheritto this example and regenerate the report reference so the example demonstrates the behavior described by this PR.
explore: my_explore
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
pjain1
force-pushed
the
parag/ai-report-schema-and-watermark
branch
from
September 4, 2026 11:33
a56cde6 to
9b3ad28
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Reconciler-level watermark inheritance coverage is required, and the schema example should be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
runtime/parser/schema/project.schema.yaml:2884
- This example still omits
watermark: inherit. Without it,ReportReconcilerkeeps the trigger time as the execution watermark (runtime/reconcilers/report.go:421-431), and the AI resolver then uses that value forlatest; merely settingexploredoes not make these ranges data-relative. Addwatermark: inheritto the example (and regenerate the reference docs) so it demonstrates the behavior described forexplore.
explore: my_explore
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
nishantmonu51
added
Type:Bug
…rk: inherit` Reports already support `watermark: inherit`, which uses the lowest watermark of the report's refs as the execution time so that `latest` in time range expressions means the latest data rather than the trigger time. AI reports could not use it because they had no refs, so the reconciler silently fell back to the trigger time. - Add the `explore` of an AI report as a ref. This also makes the report wait for the explore to reconcile before running. - In the report reconciler, resolve explore refs to their metrics view before querying the watermark. This keeps the `ResourceWatermark` query keyed on the metrics view, whose cache key tracks the underlying data, rather than on the explore, which does not change when data refreshes. - Document the behavior on the `explore` property of the AI resolver. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
pjain1
force-pushed
the
parag/ai-report-schema-and-watermark
branch
from
September 4, 2026 17:28
9b3ad28 to
2b18a63
Compare
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The schema example still omits the inheritance setting needed to demonstrate the documented behavior.
Review details
Suppressed comments (1)
runtime/parser/schema/project.schema.yaml:2884
- This example still omits the report-level
watermark: inherit, so itslatest/Dexpressions continue to use the trigger time rather than the explore's data watermark. Reports default totrigger_time(runtime/parser/parse_report.go:88-99), and metrics-view resolution replaces its timestamp bounds with the report execution time (runtime/metricsview/executor/executor_rewrite_time.go:79-84). Add inheritance here so the corrected example demonstrates the data-based behavior described below.
explore: my_explore
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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
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.
Fixes #9849
Checklist: