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

Remove get_catalog_for_single_relation from sql/impl.py #241

Merged
merged 2 commits into from
Jun 20, 2024
Merged

Conversation

aranke
Copy link
Member

@aranke aranke commented Jun 20, 2024

Make consistent with calculate_freshness_from_metadata

@cla-bot cla-bot bot added the cla:yes label Jun 20, 2024
Copy link

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@aranke aranke marked this pull request as ready for review June 20, 2024 12:46
@aranke aranke requested a review from a team as a code owner June 20, 2024 12:46
mikealfare
mikealfare previously approved these changes Jun 20, 2024
@mikealfare mikealfare dismissed their stale review June 20, 2024 13:08

I misread the PR

@mikealfare
Copy link
Contributor

I think there's a decent number of adapters that would be able to just define the macro and go on from there, without needing to overwrite the method. In a sense, this is the equivalent of an abstract method for a SQLAdapter-based adapter. Is there a specific reason to remove this and fallback to the abstract method in BaseAdapter?

@aranke
Copy link
Member Author

aranke commented Jun 20, 2024

@mikealfare Is there an example of such an adapter?

@aranke
Copy link
Member Author

aranke commented Jun 20, 2024

@mikealfare Synced with @jtcohen6 offline, this macro needs to do some post-processing to return a CatalogTable object; I don't think a macro can directly return this.

Given this fact, I'd prefer to remove this stub here to imply that there's more work required for an adapter maintainer.

@mikealfare
Copy link
Contributor

@mikealfare Synced with @jtcohen6 offline, this macro needs to do some post-processing to return a CatalogTable object; I don't think a macro can directly return this.

Given this fact, I'd prefer to remove this stub here to imply that there's more work required for an adapter maintainer.

Ah, yes. I just looked at the dbt-snowflake implementation in your PR and there's quite a bit more logic there than the stub suggests. In that case I agree for two reasons. The stub will never be used and the presence of the stub is misleading.

In that case, can we also remove the related module variable for the macro name? I don't think it's used anywhere else.

@aranke aranke requested a review from mikealfare June 20, 2024 15:49
@aranke
Copy link
Member Author

aranke commented Jun 20, 2024

@mikealfare good call-out, done!

@aranke aranke merged commit ecf3e1d into main Jun 20, 2024
15 checks passed
@aranke aranke deleted the gcfsr_cleanup branch June 20, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants