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-1951] [Feature] get_relations_by_pattern should move from utils to Core #6789

Open
3 tasks done
joellabes opened this issue Jan 31, 2023 · 4 comments
Open
3 tasks done
Assignees
Labels
enhancement New feature or request Refinement Maintainer input needed utils Cross-database building blocks

Comments

@joellabes
Copy link
Contributor

Is this your first time submitting a feature request?

  • I have read the expectations for open source contributors
  • I have searched the existing issues, and I could not find an existing issue for this feature
  • I am requesting a straightforward extension of existing dbt functionality, rather than a Big Idea better suited to a discussion

Describe the feature

This is prompted by dbt-labs/dbt-utils#753, which aims to add support for Redshift external tables to the above macro.

It is unclear whether/how the other adapters' implementations support external tables, and as I started looking into it I realised that since it was poking around in the information schema directly, it is probably something that would be better suited to a consistent, reliable cross-database implementation. That is to say, it would be better suited to living in the adapters!

Describe alternatives you've considered

YOLO-merging the changes and accepting that Redshift finds external tables where BQ wouldn't (NB: I have no idea whether BQ's version finds external tables, that's the whole problem). I'll be back in this same spot in 6 months when someone opens another utils issue for another adapter, and so on.

Who will this benefit?

Mostly me? Also users of smaller adapters, especially those that don't have an info schema.

I forgot to fully deprecate its older and less flexible get_relations_by_prefix sibling macro during the v1 migration. For the avoidance of doubt, you shouldn't migrate that over.

Both of these would stick around in dbt utils using the same sort of forwarding setup as {{ dbt.type_int() }} and friends had in the 0.9.x vintage.

Are you interested in contributing this feature?

If needed, but there's probably others who could do it better!

Anything else?

No response

@joellabes joellabes added enhancement New feature or request triage labels Jan 31, 2023
@github-actions github-actions bot changed the title [Feature] get_relations_by_pattern should move from utils to Core [CT-1951] [Feature] get_relations_by_pattern should move from utils to Core Jan 31, 2023
@jtcohen6 jtcohen6 added utils Cross-database building blocks Team:Adapters Issues designated for the adapter area of the code labels Jan 31, 2023
@dbeatty10 dbeatty10 self-assigned this Jan 31, 2023
@dbeatty10
Copy link
Contributor

dbeatty10 commented Jan 31, 2023

Thanks for opening this @joellabes !

Big picture

since it was poking around in the information schema directly, it is probably something that would be better suited to a consistent, reliable cross-database implementation

💯 agreed that this would benefit from a reliable cross-database implementation!

Connecting the dots

I see this as spiritually related to:

And it might resolve many or all of these:

And these contain some of the historical context as it relates to external tables:

Brass tacks

What's involved

I didn't robustly check all the details, but it looks like removing get_relations_by_pattern from dbt-utils would also take a few other macros along with it. So the listing of macros-to-remove from dbt_utils would be:

  • get_relations_by_pattern
  • get_tables_by_pattern_sql
  • get_matching_schemata
  • get_table_types_sql

These are found in the following files:

This would require double-checking -- maybe that's it, but maybe there would be more.

Considering the current implementation of get_relations_by_pattern as-is

The current implementation of get_relations_by_pattern defers the actual pattern matching bit to get_tables_by_pattern_sql (whose default of implementation relies on ilike).

Rather than just upstreaming get_relations_by_pattern, etc to dbt-core as-is, I'd like to invert the dynamic and consider:

  • What abstractions could/should we have in dbt adapters in order to "get a list of relations matching a pattern"?
  • How can dbt-utils hook utilize those abstractions as-needed to maintain backwards-compatibility for as long as it wants?

Maybe the answers to those will lead us to just lift-and-shift the current implementations. But similar to dbt_utils.get_relations_by_prefix leading to dbt_utils.get_relations_by_pattern, the exploration might yield a slightly different set of abstractions.

@dbeatty10 dbeatty10 added Refinement Maintainer input needed and removed triage labels Jan 31, 2023
@joellabes
Copy link
Contributor Author

  • What abstractions could/should we have in dbt adapters in order to "get a list of relations matching a pattern"?

I wouldn't be averse to this supporting regex for example if it could be done in a broadly compatible way.

@joellabes
Copy link
Contributor Author

This just came up again in dbt-labs/dbt-utils#779

@brendan-cook-87
Copy link

This is still outstanding 10 months later. Is it possible we get the original PR i made to resolve this merged @joellabes seeing as there appears to be no traction on a fix in dbt-core.

@mikealfare mikealfare removed the Team:Adapters Issues designated for the adapter area of the code label Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Refinement Maintainer input needed utils Cross-database building blocks
Projects
None yet
Development

No branches or pull requests

5 participants