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

[CT-691] why isn't postgres_get_relations double-underscored? #5301

Closed
dataders opened this issue May 26, 2022 · 6 comments · Fixed by #7944
Closed

[CT-691] why isn't postgres_get_relations double-underscored? #5301

dataders opened this issue May 26, 2022 · 6 comments · Fixed by #7944
Labels
good_first_issue Straightforward + self-contained changes, good for new contributors! postgres Team:Adapters Issues designated for the adapter area of the code

Comments

@dataders
Copy link
Contributor

for dbt-greenplum, @markporoshin wants to inherit this macro without copying it. is there a reason it isn't double-underscored? Currently, the workaround is to modify this line in impl.py

# note that this isn't an adapter macro, so just a single underscore
GET_RELATIONS_MACRO_NAME = "postgres_get_relations"

to be this:

GET_RELATIONS_MACRO_NAME = "greenplum_get_relations"

but I don't know why!

{% macro postgres_get_relations () -%}

@github-actions github-actions bot changed the title why isn't postgres_get_relations double-underscored? [CT-688] why isn't postgres_get_relations double-underscored? May 26, 2022
@github-actions github-actions bot changed the title [CT-688] why isn't postgres_get_relations double-underscored? [CT-691] why isn't postgres_get_relations double-underscored? May 26, 2022
@gshank gshank added triage Team:Adapters Issues designated for the adapter area of the code labels May 27, 2022
@jtcohen6
Copy link
Contributor

@dataders I don't think there's a very good reason :) want to change it?

(We should leave the old macro name in place, in case anyone has overridden it by that name, to avoid breaking changes in upgrade.)

My best guess: for "internal" macros, which are specific to one adapter, and shouldn't accidentally interfere with real adapter dispatch, we opted for normal-ish snake case names, rather than the dunder prefix. See, for instance, dbt_snowflake_validate_get_incremental_strategy. But it makes it impossible to use actual dispatch functionality!

For what it's worth, this has caused me some slight issues when working on the "experimental package" for materialized views as well, since that requires a minor alteration to postgres_get_relations.

@jtcohen6 jtcohen6 added good_first_issue Straightforward + self-contained changes, good for new contributors! postgres and removed triage labels May 31, 2022
@github-actions
Copy link
Contributor

This issue has been marked as Stale because it has been open for 180 days with no activity. If you would like the issue to remain open, please remove the stale label or comment on the issue, or it will be closed in 7 days.

@github-actions github-actions bot added stale Issues that have gone stale and removed stale Issues that have gone stale labels Nov 28, 2022
@dbeatty10
Copy link
Contributor

@alzaar thanks for trying to find a good_first_issue to make your first contributions to this repo.

@dataders is this issue still relevant? If so, then @alzaar would like to try resolving it.

@alzaar
Copy link

alzaar commented May 23, 2023

Thanks @dbeatty10 In terms of any pre-reqs, is there anything I need to know before I get started (from the dbt docs) ? I would also like to know how can I replicate the usage locally.

@dbeatty10
Copy link
Contributor

@alzaar you can set up your machine for local development using our contributing instructions (which include a local Postgres database using Docker).

Then you can create a local dbt project via dbt init or some other means. Run dbt debug to make sure the connection is working.

Create this file:
analyses/test_macro.sql

{% set result = postgres_get_relations() %}

{% if execute %}
{% do result.print_table() %}
{% endif %}

Run it and see the results:

dbt compile -s test_macro

After you've done your implementation, then you should be able to change postgres_get_relations within analyses/test_macro.sql to either be get_relations or postgres__get_relations and it should work.

@dbeatty10 dbeatty10 removed the triage label May 23, 2023
@dbeatty10
Copy link
Contributor

@alzaar if you've never run a dbt project before, then this repo is a quick way to get a first experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good_first_issue Straightforward + self-contained changes, good for new contributors! postgres Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants