Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tidying up materialization logic: relation names, cache loading, etc #5221

Merged
merged 3 commits into from
May 20, 2022

Conversation

jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented May 9, 2022

Inspired by:

Description

  • Rename load_relation to load_cached_relation (with backwards compat). Use this macro much more frequently, rather than adapter.get_relation (verbose + harder to read)
  • Even more consistent naming: existing_relation (never old_relation), temp_relation, intermediate_relation, backup_relation. Always X_relation, never X_identifier.
  • Use get_create_table_as_sql or get_create_view_as_sql, instead of create_table_as or create_view_as (per Reorganize global project (macros) #4154). Long-term goal: better distinguish between static macros that -> str ("returns SQL"), and "dirty" macros that -> Result (runs query by itself, a la adapter.rename_relation).
  • Adds back test case removed in Truncate relation names when appending a suffix #4921, with the understanding that before these PRs go into main, this will need to be ported over to the new test location here

Checklist

@jtcohen6 jtcohen6 requested a review from a team May 9, 2022 07:27
@jtcohen6 jtcohen6 requested review from a team as code owners May 9, 2022 07:27
@jtcohen6 jtcohen6 requested review from stu-k and McKnight-42 May 9, 2022 07:27
@cla-bot cla-bot bot added the cla:yes label May 9, 2022
@jtcohen6 jtcohen6 force-pushed the jerco/materialization-cleanup branch from 08bb9ef to a5abe20 Compare May 9, 2022 07:38
@jtcohen6
Copy link
Contributor Author

jtcohen6 commented May 9, 2022

Code quality checks are failing because of black <> click incompatibility. We resolved this on main a while ago, it's just coming up here because I branched off #4921. We'll get it fixed when rebasing against main.

@jtcohen6 jtcohen6 changed the title Lightly dust materialization logic: relation names, loading, etc Tidying up materialization logic: relation names, loading, etc May 9, 2022
@jtcohen6 jtcohen6 changed the title Tidying up materialization logic: relation names, loading, etc Tidying up materialization logic: relation names, cache loading, etc May 9, 2022
Copy link
Contributor

@McKnight-42 McKnight-42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me

Base automatically changed from dev/epapineau to main May 19, 2022 11:57
@jtcohen6 jtcohen6 force-pushed the jerco/materialization-cleanup branch from a5abe20 to 81e6a2b Compare May 19, 2022 12:01
@jtcohen6 jtcohen6 added the Team:Adapters Issues designated for the adapter area of the code label May 20, 2022
@jtcohen6
Copy link
Contributor Author

Hmm not sure why Snyk didn't run. Going to try my other special move, closing and reopening

@jtcohen6 jtcohen6 closed this May 20, 2022
@jtcohen6 jtcohen6 reopened this May 20, 2022
@jtcohen6
Copy link
Contributor Author

Thanks for reviewing @McKnight-42! I'm going to merge, since this is really just a rename-style refactor, and all the tests are passing :)

@jtcohen6 jtcohen6 merged commit aeee1c2 into main May 20, 2022
@jtcohen6 jtcohen6 deleted the jerco/materialization-cleanup branch May 20, 2022 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants