feat: Re-ingest local files on updates #5656
Conversation
k-anshul
changed the title
k-anshul
force-pushed
the
local_duckdb_me
branch
from
7fa86aa
to
66a426d
Compare
| root, err := p.Repo.Root(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Update parser's localDataToResourcepath map to track which resources depend on the local file | ||
| for _, entry := range entries { | ||
| localPaths = append(localPaths, filepath.Join(root, entry.Path)) |
There was a problem hiding this comment.
The same parser is only used for one repo, so could it just store the project-relative path? That would reduce dependency on the local filesystem.
I suppose there's a case on local where you can reference a path outside of the project. I would actually prefer we do not hash or auto-refresh for files outside the project because a) it's a workflow we should discourage, and b) the watcher doesn't (and shouldn't) watch them.
| if !ok { | ||
| resources = make(map[string]any) |
There was a problem hiding this comment.
The nesting is getting very deep here, consider pulling the inputConnector == "local_file" handling into a separate function to support early return: https://github.com/uber-go/guide/blob/master/style.md#reduce-nesting
| file, err := os.Open(path) | ||
| if err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
This again becomes the only place with a hard dependency on a real local file system in the rillv1 package – any chance we could put this under an interface? I realize using RepoStore.Get won't do, but perhaps this could be solved by adding a buffered GetReader(ctx, path) (io.Reader, error) function to RepoStore?
| // Mapping of local data files to resource paths that depend on them | ||
| localDataToResourcepath map[string]map[string]any | ||
|
|
||
| // Internal state | ||
| resourcesForPath map[string][]*Resource // Reverse index of Resource.Paths | ||
| resourcesForUnspecifiedRef map[string][]*Resource // Reverse index of Resource.rawRefs where kind=ResourceKindUnspecified |
There was a problem hiding this comment.
Since the parser already keeps state of the paths for a particular ResourceName (through Parser.Resources[name]), could it just store the resource names that use a local data file?
Also, can it put it under the // Internal state section? Ordering of fields indicate their importance and relationship, and this is for example a less central piece of state than resourcesForPath.
For example:
| // Mapping of local data files to resource paths that depend on them | |
| localDataToResourcepath map[string]map[string]any | |
| // Internal state | |
| resourcesForPath map[string][]*Resource // Reverse index of Resource.Paths | |
| resourcesForUnspecifiedRef map[string][]*Resource // Reverse index of Resource.rawRefs where kind=ResourceKindUnspecified | |
| // Internal state | |
| resourcesForPath map[string][]*Resource // Reverse index of Resource.Paths | |
| resourcesForUnspecifiedRef map[string][]*Resource // Reverse index of Resource.rawRefs where kind=ResourceKindUnspecified | |
| resourceNamesForDataPaths map[string][]ResourceName // Index of local data files to resources that depend on them |
closes #5479
Sample model file: