Repository navigation
Support file selectors in pipelines run - #7184
Open
chenyuem-db wants to merge 4 commits into
Open
chenyuem-db wants to merge 4 commits into
chenyuem-db wants to merge 4 commits into
Conversation
Collaborator
✅ Integration test reportCommit: d2509d2
Top 9 slowest tests (at least 2 minutes):
|
chenyuem-db
force-pushed
the
chenyuem-db/pipeline-refresh-by-file
branch
2 times, most recently
from
October 28, 2026 17:02
0824056 to
652cd9e
Compare
Contributor
|
Commit: Regression test reportSelected 2 changed tests (limit=20, 0 not selected) 3 tests: regression=3 unreleased=0 coverage=0 failing=0
TestAccept/bundle/help/bundle-run/DMS=: branch ✅ | main (33bcce3) ❌ | latest (v1.24.0) ❌main (33bcce3): latest (v1.24.0): same output as main (33bcce3) TestAccept/bundle/help/bundle-run/DMS=true: branch ✅ | main (33bcce3) ❌ | latest (v1.24.0) ❌main (33bcce3): latest (v1.24.0): same output as main (33bcce3) TestAccept/pipelines/run/refresh-flags: branch ✅ | main (33bcce3) ❌ | latest (v1.24.0) ❌main (33bcce3): latest (v1.24.0): |
chenyuem-db
force-pushed
the
chenyuem-db/pipeline-refresh-by-file
branch
from
October 28, 2026 22:30
ae8800b to
9fd46d5
Compare
chenyuem-db
marked this pull request as ready for review
October 29, 2026 22:08
chenyuem-db
requested review from
a team,
jefferycheng1,
kanterov and
lennartkats-db
as code owners
October 29, 2026 22:08
chenyuem-db
force-pushed
the
chenyuem-db/pipeline-refresh-by-file
branch
from
November 1, 2026 22:44
9fd46d5 to
e092a9c
Compare
chenyuem-db
force-pushed
the
chenyuem-db/pipeline-refresh-by-file
branch
from
November 1, 2026 22:53
e092a9c to
9d78b59
Compare
andrewnester
reviewed
Nov 6, 2026
| trace print_requests.py //pipelines | ||
|
|
||
| title "Refreshing tables defined in a source file" | ||
| trace $CLI pipelines run --refresh pipeline_file.py |
Contributor
There was a problem hiding this comment.
Just to double check: did you run this against a real workspace? The description says no live refresh was performed and the fake server only echoes the request back, so nothing here confirms the API accepts paths in this form (/Workspace/... prefix, notebook extension stripped). Could you try --refresh pipeline_file.py and a notebook manually and share the result, or make this test Cloud = true?
Contributor
Author
There was a problem hiding this comment.
I ran this on staging with both a Python file and a SQL notebook, and both worked. I also enabled Cloud = true for this test.
This comment was generated with GitHub MCP.
| return fmt.Errorf("cannot use --refresh or --full-refresh together with %s, these flags are mutually exclusive", strings.Join(mutuallyExclusiveFlags, "")) | ||
| } | ||
|
|
||
| // The API ignores file selections when table names are supplied for the same refresh mode. |
Contributor
There was a problem hiding this comment.
Is it documented somewhere that the API ignores the by-path selection when table names are present? The SDK docs for refresh_selection_by_path don't mention it. If it's a real API limitation let's link it here, otherwise I'd rather not restrict mixing in the CLI.
Contributor
Author
There was a problem hiding this comment.
I checked the backend, and it uses the table selection when both are set for the same refresh mode. I kept the check so we don’t silently ignore the files.
This comment was generated with GitHub MCP.
| // List of tables to update. | ||
| Refresh []string | ||
|
|
||
| RefreshByPath []string |
Contributor
There was a problem hiding this comment.
These fields are only ever set from cmd/pipelines/run.go; bundle run --refresh (see Define below) still only takes table names. Do we want bundle run to support files too? The header of cmd/pipelines/run.go asks to keep both in sync.
Contributor
Author
There was a problem hiding this comment.
Yes, bundle run should support files too. Both commands now use the same resolver in bundle/run.
This comment was generated with GitHub MCP.
| trace print_requests.py //pipelines | ||
|
|
||
| title "Fully refreshing tables defined in a source file" | ||
| trace $CLI pipelines run --full-refresh pipeline_file.py |
Contributor
There was a problem hiding this comment.
Could you also cover a notebook (extension stripped), an absolute /Workspace/ path and running from a subdirectory here? Only pipeline_file.py from the bundle root is exercised end-to-end, the rest is unit tests only.
Contributor
Author
There was a problem hiding this comment.
Added all three cases to this test: a notebook, an absolute /Workspace/ path, and running from a subdirectory.
This comment was generated with GitHub MCP.
| if err != nil { | ||
| return fmt.Errorf("invalid --full-refresh: %w", err) | ||
| } | ||
| if err := runOptions.Pipeline.Validate(nil); err != nil { |
Contributor
There was a problem hiding this comment.
toPayload already calls Validate, so this runs twice; I assume it's here so --restart fails before stopping the pipeline. Worth a comment, or move it into Restart so bundle run --restart gets the same behaviour?
Contributor
Author
There was a problem hiding this comment.
Yes, this is to catch invalid input before stopping the pipeline. I added a short comment and made sure bundle run --restart does the same.
This comment was generated with GitHub MCP.
| remoteRoot = filepath.ToSlash(b.SyncRootPath) | ||
| } | ||
| if strings.Contains(remoteRoot, "${") { | ||
| return nil, nil, fmt.Errorf("cannot resolve the deployed path for %q; use its absolute /Workspace/ path", selector) |
Contributor
There was a problem hiding this comment.
In immutable mode this means every local file selector fails and the user has to find the snapshot path by hand, and we don't print it anywhere. The snapshot's full_path is in the deployment state, can we resolve it from there instead of erroring? Or is this fine for the first version?
Contributor
Author
There was a problem hiding this comment.
This now reads the deployed snapshot path from state using the existing ResolveConfigAgainstState helper. I added a test for both refresh flags.
This comment was generated with GitHub MCP.
| } | ||
| return nil, nil, fmt.Errorf("source file %q is excluded from bundle sync; include it and run 'databricks pipelines deploy' first", selector) | ||
| } | ||
| remoteRoot := b.Config.Workspace.FilePath |
Contributor
There was a problem hiding this comment.
This is the third copy of the "which remote root" rule: applyTranslations in bundle/config/mutator/translate_paths.go:323-340 and lookup in bundle/config/mutator/resolve_variable_references.go:114-119 do the same switch. Can we extract a helper on *bundle.Bundle and use it in all three places?
Contributor
Author
There was a problem hiding this comment.
Moved this into Bundle.RemoteFilePath() and updated all three call sites to use it.
This comment was generated with GitHub MCP.
| } | ||
|
|
||
| // refreshSourcePaths uses the sync mapping so notebook names and sync exclusions match deployment. | ||
| func refreshSourcePaths(ctx context.Context, b *bundle.Bundle) (map[string]string, error) { |
Contributor
There was a problem hiding this comment.
listSyncableRelPaths in bundle/config/mutator/resolve_job_run_file_triggers.go:80 builds the same file list from the same inputs. Could you reuse it (or share one helper) instead of a second copy?
Contributor
Author
There was a problem hiding this comment.
Both now use Bundle.GetSyncFiles() to build the file list.
This comment was generated with GitHub MCP.
| func resolveRefreshSelectors(ctx context.Context, b *bundle.Bundle, selectors []string) (tables, paths []string, err error) { | ||
| var remoteNames map[string]string | ||
| for _, selector := range selectors { | ||
| if strings.HasPrefix(selector, "/Workspace/") { |
Contributor
There was a problem hiding this comment.
We have libraries.IsWorkspacePath (bundle/libraries/path.go:24) which also accepts /Users/ and /Shared/. Is accepting only /Workspace/ here intentional (because /Users/... is a real local path on macOS)? If so, worth a comment.
Contributor
Author
There was a problem hiding this comment.
Yes, that’s intentional because /Users/... can be a local path on macOS. I added a comment here.
This comment was generated with GitHub MCP.
| return nil, nil, fmt.Errorf("source file %q is outside the bundle sync root", selector) | ||
| } | ||
| if remoteNames == nil { | ||
| remoteNames, err = refreshSourcePaths(ctx, b) |
Contributor
There was a problem hiding this comment.
nit: resolveRefreshSelectors is called once per flag, so with files in both --refresh and --full-refresh we walk the sync tree twice. Could the mapping be computed once and passed in?
Contributor
Author
There was a problem hiding this comment.
The two flags now reuse the same mapping, so we only scan the files once.
This comment was generated with GitHub MCP.
This branch has not been deployed
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.
Changes
Support source files in
pipelines run --refreshand--full-refresh, so users can refresh the tables defined in a file without listing their names; pipelinebundle runuses the same resolver.Local paths are relative to the current directory and follow bundle sync rules, including notebook names and exclusions. Immutable deployments resolve the deployed snapshot path from state. Absolute
/Workspace/paths are also accepted.Each flag accepts either table names or files because the API gives table selections precedence within a refresh mode. The two flags can use different selector types. Existing local files take precedence over bare names; backticks select ambiguous table identifiers. Invalid selectors are rejected before a restart stops the active update.
Completion summaries fetch paginated events and filter progress events locally because the public events API rejects
event_typefilters.Why
Implements file-based selective refresh from the SDP Turbo CLI PRD, bringing the UI “Run file” capability to the CLI. Local changes use the existing deploy-then-run workflow. Output shows selected source paths; displaying resolved table names remains a follow-up.
Tests
Cloud = trueon the existing refresh-flags test and passed it against a real staging workspace, covering table and file refreshes, notebooks, absolute paths, subdirectory runs, andbundle run; invalid restart inputs sent no pipeline mutations../task fmt,./task checks, and./task lint.