Conversation
⏱️ Benchmark results
|
|
I removed some error validation from here but I can bring it back it just should go into |
hermanschaaf
left a comment
There was a problem hiding this comment.
Looks good, I think this should do the trick. 👍
I would just like to make sure we don't leave a gap in our validation and testing:
- we should have at least some basic test for
TablesForSpec - we should bring back some of the validation that
listAndValidateTableswas doing, and the tests for it. It's fine if we put that inValidate, but it was checking for things like invalid table names (or globs that match nothing), which I think is a pretty useful user-facing feature
erezrokah
left a comment
There was a problem hiding this comment.
I like this approach, but I think we're missing a use case
| for _, r := range t.Relations { | ||
| filteredChild := r.filterDfs(tables, skipTables) | ||
| if filteredChild != nil { | ||
| matched = true |
There was a problem hiding this comment.
Won't this always return the parent table if the child matches, for example:
{
name: "should skip parent table",
tables: []*Table{{Name: "main_table", Relations: []*Table{{Name: "sub_table", Parent: &Table{Name: "main_table"}}}}},
configurationTables: []string{"*"},
configurationSkipTables: []string{"main_table"},
want: []string{},
},The above test fails as it returns main_table and sub_table.
Maybe we should return early if the parent doesn't match is skipped
There was a problem hiding this comment.
Good catch. it depends on what we define on what has preference. if skip has preference then yes. I've added that logic.
There was a problem hiding this comment.
Ah I think https://github.com/cloudquery/plugin-sdk/pull/488/files#diff-7e84c216b71c17b23bc66ee33042fe73fc9352c152dbc4fb4cd9a94d74e2bfb4R49 is the reason
So I think we need to split Your fix works toomatched into matched and skipped if we want to support both
Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>
Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>
|
Also @yevgenypats may I suggest we do this as a
We've had a bunch of bugs related to this logic, so maybe safer to do a dedicated release |
| } | ||
| for _, skipTable := range skipTables { | ||
| if glob.Glob(skipTable, t.Name) { | ||
| return nil |
There was a problem hiding this comment.
With this I think we can do the skipTables for loop first and the tables later
| var matched bool | ||
| for _, includeTable := range tables { | ||
| if glob.Glob(includeTable, t.Name) { | ||
| matched = true | ||
| break | ||
| } | ||
| } | ||
| for _, skipTable := range skipTables { | ||
| if glob.Glob(skipTable, t.Name) { | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
| var matched bool | |
| for _, includeTable := range tables { | |
| if glob.Glob(includeTable, t.Name) { | |
| matched = true | |
| break | |
| } | |
| } | |
| for _, skipTable := range skipTables { | |
| if glob.Glob(skipTable, t.Name) { | |
| return nil | |
| } | |
| } | |
| for _, skipTable := range skipTables { | |
| if glob.Glob(skipTable, t.Name) { | |
| return nil | |
| } | |
| } | |
| var matched bool | |
| for _, includeTable := range tables { | |
| if glob.Glob(includeTable, t.Name) { | |
| matched = true | |
| break | |
| } | |
| } |
erezrokah
left a comment
There was a problem hiding this comment.
I think this looks good now, added another blocking comment as I think we can exit early on skip
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
🤖 I have created a release *beep* *boop* --- ## [1.12.1](v1.12.0...v1.12.1) (2022-12-13) ### Bug Fixes * Don't panic on empty-string for timestamp ([#489](#489)) ([83813de](83813de)) * Fix deadlock off-by-one ([#493](#493)) ([4ea9ed8](4ea9ed8)) * Reduce default concurrency ([#491](#491)) ([f995da9](f995da9)) * Refactor glob filters ([#488](#488)) ([cb5f6bb](cb5f6bb)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This simplifies our glob filtering and makes sure Tables are always stored in a tree like structured apart from when returned from
FlattenTables. This solves a lot of potential bugs and places where we need to do things likeif parent != nil.I think this also closes #475 as there is no need for another data structure.
The reasoning for this is that our tables are tree like structured and it's easier imo to always keep it like this everywhere across the code and not have multiple data structures with similar methods and so on. This required a small update to our filtering logic which just works as a DFS and in-place filtering (for that to work I had to add a copy method which I think can be useful for other things in the future).