Conversation
There was a problem hiding this comment.
Pull request overview
Updates the GitHub datasource schemads schema and SQL normalization to better align with actual pushdown behavior, adding repository filter pushdown and a new timeField table parameter to control which datetime column is used for time-range filtering.
Changes:
- Clean up schema declarations by removing advertised operators that aren’t actually supported/pushed down, and introducing
timeFieldas an optional table parameter for selected tables. - Add filter pushdown for
repositories(is_fork,is_private) and includedeploymentsin table-to-query-type mapping. - Expand test coverage for the new pushdown behavior and
timeFieldnormalization/defaulting.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/github/sql.go | Adds repositories filter pushdown; adds timeField resolution/defaulting in SQL request normalization. |
| pkg/github/schema.go | Removes explicit datetime operator advertising; adds timeField table parameter + dynamic value serving. |
| pkg/github/sql_handler_test.go | Extends normalization coverage for deployments, repositories pushdown, and timeField behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| switch queryType { | ||
| case models.QueryTypeIssues, models.QueryTypePullRequests, models.QueryTypePullRequestReviews, models.QueryTypeWorkflows: | ||
| opts, _ := normalized["options"].(map[string]interface{}) | ||
| if tfStr := strings.TrimSpace(anyToString(query.TableParameterValues["timeField"])); tfStr != "" { | ||
| if tf, ok := resolveTimeField(queryType, tfStr); ok { | ||
| opts["timeField"] = tf | ||
| } | ||
| } else { | ||
| opts["timeField"] = defaultTimeField(queryType) | ||
| } |
There was a problem hiding this comment.
If tableParameterValues.timeField is provided but resolveTimeField returns ok=false (invalid/unsupported value), the code currently leaves options.timeField unset. That will cause the downstream JSON unmarshal defaults to kick in (e.g., PRs default to PullRequestClosedAt because enum 0 != the intended default), which is surprising and can change query semantics. Consider falling back to defaultTimeField(queryType) when the provided value is non-empty but unrecognized (and optionally normalizing case with strings.ToLower).
| case models.QueryTypeIssues: | ||
| switch value { | ||
| case "created": | ||
| return int(models.IssueCreatedAt), true |
There was a problem hiding this comment.
nit: it's probably fine but something feels weird about casting to int for every return
There was a problem hiding this comment.
Updated, good nit 😊
Summary
Cleans up the
schemadsschema declarations, adds new filter pushdown andtimeFieldtable parameter support, and expands test coverage.Schema cleanup (
schema.go)timeRangeOperators— datetime columns no longer declare explicit>,>=,<,<=operators for filter pushdown. Time-based filtering is handled by Grafana's globalTimeRangemechanism, so advertising these operators was misleading.equalityOperatorsfrom Vulnerabilitiesseverity/state— these columns aren't backed by any pushdown implementation inapplyFiltersand the GitHub API doesn't support filtering them server-side.Filter pushdown for Repositories (
sql.go)is_forkandis_privatefilters on the Repositories table, translating them into GitHub search qualifiers (fork:only,is:private,is:public).QueryTypeDeploymentsto thetableToQueryTypemap.timeFieldtable parameter (schema.go,sql.go)repoScopedWithTimeFieldTableParameters— extends the standard repo-scoped parameters with an optionaltimeFieldparameter.timeFieldValuesForTablehelper providing valid values per table type (e.g.created,closed,merged,updated).SchemaandTableParameterValueshandlers to servetimeFieldvalues dynamically.resolveTimeField/defaultTimeFieldhelpers insql.goto map user-provided strings (e.g."merged") to the correct integer enum constant, with sensible defaults per query type.normalizeGrafanaSQLRequestso the time range picker filters by the user-selected datetime column.