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

Test the default implementation against all supported adapters #622

Closed
wants to merge 11 commits into from

Conversation

dbeatty10
Copy link
Contributor

@dbeatty10 dbeatty10 commented Jul 16, 2022

Related to #621

This is a:

  • documentation update
  • bug fix with no breaking changes
  • new functionality
  • a breaking change

All pull requests from community contributors should target the main branch (default).

Description & motivation

This pull request demonstrates that the current default implementation will not work in call cases because natural joins are not null safe.

Suggested path forward

My recommendation would be the following:

  1. open up a separate pull request
  2. utilize the changes to the integration tests demonstrated here
  3. add integration tests that cover both CTE and relations separately
  4. remove the natural join out of the implementation
  5. choose one of the following (or some other alternative):
    • reinstate the original default implementation
    • for the default implementation throw a "not supported error" if the relation parameter is a CTE rather than a true Relation (ref or source)

Checklist

  • I have verified that these changes work locally on the following warehouses (Note: it's okay if you do not have access to all warehouses, this helps us understand what has been covered)
    • BigQuery
    • Postgres
    • Redshift
    • Snowflake
  • I followed guidelines to ensure that my changes will work on "non-core" adapters by:
    • dispatching any new macro(s) so non-core adapters can also use them (e.g. the star() source)
    • using the limit_zero() macro in place of the literal string: limit 0
    • using dbt_utils.type_* macros instead of explicit datatypes (e.g. dbt_utils.type_timestamp() instead of TIMESTAMP
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)
  • I have added an entry to CHANGELOG.md

@github-actions
Copy link

This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jul 28, 2023
@github-actions
Copy link

github-actions bot commented Aug 4, 2023

Although we are closing this PR as stale, it can still be reopened to continue development. Just add a comment to notify the maintainers.

@github-actions github-actions bot closed this Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant