Skip to content

refactor: Dedup tables only at the top level in FlattenTables#1107

Merged
kodiakhq[bot] merged 3 commits intocloudquery:mainfrom
erezrokah:refactor/flatten_tables_dedup_top_level_only
Sep 5, 2023
Merged

refactor: Dedup tables only at the top level in FlattenTables#1107
kodiakhq[bot] merged 3 commits intocloudquery:mainfrom
erezrokah:refactor/flatten_tables_dedup_top_level_only

Conversation

@erezrokah
Copy link
Copy Markdown
Member

@erezrokah erezrokah commented Jul 21, 2023

Summary

Noticed this while working on #1106.
Don't think we need to dedup at each level of the recursion. Also not sure why we're deduping as table names should be unique

Use the following steps to ensure your PR is ready to be reviewed

  • Read the contribution guidelines 🧑‍🎓
  • Run go fmt to format your code 🖊
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests 🧪
  • Ensure the status checks below are successful ✅

@erezrokah erezrokah requested a review from yevgenypats as a code owner July 21, 2023 09:28
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jul 21, 2023 •
edited
Loading

⏱️ Benchmark results

Comparing with d563582

  • Glob-8 ns/op: 99.75 ⬇️ 0.55% decrease vs. d563582

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 21, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: -1.30% ⚠️

Comparison is base (d563582) 48.33% compared to head (97d3652) 47.03%.

❗ Current head 97d3652 differs from pull request most recent head 454d826. Consider uploading reports for the commit 454d826 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1107      +/-   ##
==========================================
- Coverage   48.33%   47.03%   -1.30%     
==========================================
  Files          87       85       -2     
  Lines        8158     7783     -375     
==========================================
- Hits         3943     3661     -282     
+ Misses       3858     3787      -71     
+ Partials      357      335      -22
Files Changed Coverage Δ
schema/table.go 37.95% <100.00%> (-16.93%) ⬇️

... and 24 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hermanschaaf
Copy link
Copy Markdown
Contributor

hermanschaaf commented Sep 5, 2023

If memory serves, deduping was added as a safety measure to make the FlattenTables call idempotent. Back then the Relations field also wasn't set to nil during Flatten, so if you accidentally were to call FlattenTables on an already-flattened list (as did happen in a couple of cases), you'd get duplicate tables, because the child tables would get unrolled again.

It seems to not be necessary anymore, because the Relations get set to nil. If we have a test to check that FlattenTables is idempotent, then I think we can remove the dedup, but since it's a little higher on the risk scale I would probably not do that right now.

@kodiakhq kodiakhq bot merged commit 40a942f into cloudquery:main Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yevgenypats yevgenypats Awaiting requested review from yevgenypats

1 more reviewer

@hermanschaaf hermanschaaf hermanschaaf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

automerge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@erezrokah @hermanschaaf