Fix incorrect workflow_id type for workflow_run table - #558
Conversation
a93ac88 to
afc789d
Compare
cbruno10
left a comment
There was a problem hiding this comment.
@WallyGuzman Can you please see review comments? Thanks!
| if createdAt := d.Quals["created_at"]; createdAt != nil { | ||
| for _, q := range createdAt.Quals { | ||
| givenTime := q.Value.GetTimestampValue().AsTime() | ||
| var createdTime string | ||
|
|
||
| op := q.Operator | ||
| if op == "=" { | ||
| op = "" | ||
| createdTime = givenTime.Format(time.DateOnly) | ||
| } else { | ||
| createdTime = givenTime.Format(time.RFC3339) | ||
| } | ||
|
|
||
| if opts.Created == "" { | ||
| opts.Created = op + createdTime | ||
| } else { | ||
| opts.Created = strings.TrimLeft(opts.Created, "<=>") + ".." + createdTime | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This range building is now order dependent. The quals arrive in query order, so where created_at <= '2026-02-01' and created_at >= '2026-01-01' builds 2026-02-01T00:00:00Z..2026-01-01T00:00:00Z, a reversed range that returns no rows.
Since the filter is pushed down to the API, postgres never gets the rows back to recheck, so they're silently dropped. Three or more created_at quals would also produce a malformed string like A..B..C.
Can you collect the bounds first, then build the string once? Something like:
if createdAt := d.Quals["created_at"]; createdAt != nil {
var lower, upper string
for _, q := range createdAt.Quals {
t := q.Value.GetTimestampValue().AsTime()
switch q.Operator {
case "=":
opts.Created = t.Format(time.DateOnly)
case ">", ">=":
lower = t.Format(time.RFC3339)
case "<", "<=":
upper = t.Format(time.RFC3339)
}
}
if opts.Created == "" {
switch {
case lower != "" && upper != "":
opts.Created = lower + ".." + upper
case lower != "":
opts.Created = ">=" + lower
case upper != "":
opts.Created = "<=" + upper
}
}
}One thing worth a code comment either way: .. and >=/<= are inclusive, so strict >/< quals can return boundary rows from the API. That's fine because Steampipe rechecks quals on the returned rows, but it looks lossy at first glance.
There was a problem hiding this comment.
This range building is now order dependent. The quals arrive in query order, so
where created_at <= '2026-02-01' and created_at >= '2026-01-01'builds2026-02-01T00:00:00Z..2026-01-01T00:00:00Z, a reversed range that returns no rows. Since the filter is pushed down to the API, postgres never gets the rows back to recheck, so they're silently dropped.
I don't think the order of the quals actually makes a difference and changing it does work in the bare API and in this PR:
Shell example with gh api:
$ gh api repos/turbot/steampipe/actions/runs -X GET -f created=2026-02-01T00:00:00Z..2026-01-01T00:00:00Z -F per_page=1
{
"total_count": 44,
"workflow_runs": [
{
"id": 21541410334,
"name": "30 - Admin: Stale Issues and PRs",
....
}
$ gh api repos/turbot/steampipe/actions/runs -X GET -f created=2026-01-01T00:00:00Z..2026-02-01T00:00:00Z -F per_page=1
{
"total_count": 44,
"workflow_runs": [
{
"id": 21541410334,
"name": "30 - Admin: Stale Issues and PRs",
...
}
SQL example with these changes:
select
*
from
github_actions_repository_workflow_run
where
repository_full_name = 'turbot/steampipe'
and created_at <= '2026-02-01T00:00:00Z'
and created_at >= '2026-01-01T00:00:00Z'
limit 1;
+----------------------+----------------------+----------------+----------+-------------+----------------------------+------------+-----------+------------+--->
| login_id | repository_full_name | id | event | workflow_id | node_id | conclusion | status | run_number | ar>
+----------------------+----------------------+----------------+----------+-------------+----------------------------+------------+-----------+------------+--->
| MDQ6VXNlcjM2MjM0MjY= | turbot/steampipe | 21,541,410,334 | schedule | 172,982,979 | WFR_kwLOE7GVQM8AAAAFA_fWHg | success | completed | 209 | ht>
| | | | | | | | | | >
| | | | | | | | | | >
| | | | | | | | | | >
+----------------------+----------------------+----------------+----------+-------------+----------------------------+------------+-----------+------------+--->
(END)Three or more
created_atquals would also produce a malformed string likeA..B..C. Can you collect the bounds first, then build the string once? Something like...
Maybe a dumb question, but would we ever use three or more clauses on created_at on the same repository_full_name in a single call? I'm not sure the REST API would support more than two created_at conditions [1] solely with something like the suggestion. Unless you are also referring to changing the below code to make multiple ListWorkflowRuns* calls based on the number of created_at values? In that case I think it probably makes more sense to keep all of the values and build multiple strings so that the individual calls can profit from the opts.Created pushdown.
To be clear, I'm not arguing against this suggestion and I'm happy to incorporate it, but I'm more asking about whether this will work as we expect and push down the opts.Created filter to the underlying call. Otherwise, I think we might need to make a series of calls with the appropriate filters.
One thing worth a code comment either way: .. and >=/<= are inclusive, so strict >/< quals can return boundary rows from the API. That's fine because Steampipe rechecks quals on the returned rows, but it looks lossy at first glance.
This is a good point. I hadn't considered the boundary row condition, but I think the approach of adding/subtracting a second (similar to here [2]) might work, if you agree?
[2] https://github.com/turbot/steampipe-plugin-github/blob/main/github/table_github_commit.go#L77
There was a problem hiding this comment.
You're right on the reversed range, thanks for testing that. I reproduced it, both orders return the same 44 results, so GitHub normalizes it (undocumented, but it works).
Looking at the 3 quals issue, I had done some testing and found it also fires with just two. The current code assumes the second qual is always the opposite bound of the first, so two same-direction quals break it. For instance, this where clause:
where created_at >= '2026-01-01' and created_at > '2026-01-15'Builds 2026-01-01T00:00:00Z..2026-01-15T00:00:00Z, which turns the second lower bound into an upper bound. The API returns runs from Jan 1-15, the recheck requires after Jan 15, and everything gets dropped.
The query silently returns zero rows. Same story with two upper bounds, or an = qual followed by a range qual (2026-01-01..2026-02-01T00:00:00Z, which changes the = semantics).
This shape shows up without anyone typing it, as a view or CTE with a baked-in window (created_at >= now() - interval '30 days') queried with the user's own date filter on top. Postgres doesn't collapse redundant range predicates, so both quals reach the plugin.
(Using 3+ quals also has the same issue today)
For the pushdown question, there's no need for multiple API calls. We can collapse all the quals to a single lower and upper bound, build one range string, and then make one call. The API filter just needs to be a superset of what the quals allow, and the qual recheck drops anything extra.
On the boundary rows, I'd skip the +/- 1 second adjustment from the commit table. It has an edge case for created_at > '2026-01-01T00:00:00.5Z', bumping the bound to 00:00:01.5 excludes a row at 00:00:01 that satisfies the qual, and since that happens API-side the recheck never sees the row. Just use the qual value as an inclusive bound for > and < and let the recheck drop the boundary rows. Small cost of a few extra rows fetched at most and adding a short comment could prevent future confusion.
There was a problem hiding this comment.
You're right on the reversed range, thanks for testing that. I reproduced it, both orders return the same 44 results, so GitHub normalizes it (undocumented, but it works).
...
(Using 3+ quals also has the same issue today)
First of all, thank you so much for the feedback! Extremely helpful since I'm not that familiar with Postgres.
Second, this makes *complete sense. I'll make these changes, but probably add a bit of logic to make sure we account for (possibly) redundant filters and reduce rows client-side.
For the pushdown question, there's no need for multiple API calls. We can collapse all the quals to a single lower and upper bound, build one range string, and then make one call. The API filter just needs to be a superset of what the quals allow, and the qual recheck drops anything extra.
Got it. Thanks for clarifying!
I think I did not explain what I meant correctly:
What happens in the case of where ((created_at >= '2020-01-01' and created_at <= '2020-02-01') or (created_at >= '2026-01-01' and created_at <= '2026-02-01'))? From trying this locally, it seems like this completely fails at pushing down the opts.Created filters even though it should be possible? I might be missing something here.
On the boundary rows, I'd skip the +/- 1 second adjustment from the commit table. It has an edge case for created_at > '2026-01-01T00:00:00.5Z', bumping the bound to 00:00:01.5 excludes a row at 00:00:01 that satisfies the qual, and since that happens API-side the recheck never sees the row. Just use the qual value as an inclusive bound for > and < and let the recheck drop the boundary rows. Small cost of a few extra rows fetched at most and adding a short comment could prevent future confusion.
You got it.
- Remove unecessary strconv to match other workflow-related tables - Add optional quals for headSha, actorLogin, and createdAt - Fix minor bug in workflow_job to handle missing CompletedAt fields - Also minor typo fix in docs and new query example with dates
afc789d to
6f7653e
Compare
|
Thanks @WallyGuzman for the fixes! |
Note: I'm not really sure why this was coming in as a
string. Both versions of the REST API [1,2] showworkflow_idas an integer and the go-github library sets it toint64[3]. Either way, this PR represents a breaking change for any customers who rely onworkflow_idbeing of typestring.Please let me know if there's a specific way to structure this change or otherwise handle versioning/communication for this.
Edit: Also bundled minor bug fix for
github_actions_repository_workflow_job.Edit2: Updated in response to feedback and rebased onto main.
[1] https://docs.github.com/en/rest/actions/workflow-runs?apiVersion=2022-11-28#list-workflow-runs-for-a-repository
[2] https://docs.github.com/en/rest/actions/workflow-runs?apiVersion=2026-03-10#list-workflow-runs-for-a-repository
[3] https://pkg.go.dev/github.com/google/go-github/v55@v55.0.0/github#WorkflowRun
Example query results
Results
Before
After
Other queries
Also some database logs during the query above:
*Double-check the
created_atfilters: