dbt-core icon indicating copy to clipboard operation
dbt-core copied to clipboard

[CT-1951] [Feature] get_relations_by_pattern should move from utils to Core

Open joellabes opened this issue 2 years ago • 4 comments

Is this your first time submitting a feature request?

  • [X] I have read the expectations for open source contributors
  • [X] I have searched the existing issues, and I could not find an existing issue for this feature
  • [X] 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 https://github.com/dbt-labs/dbt-utils/pull/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 avatar Jan 31 '23 05:01 joellabes

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:

  • https://github.com/dbt-labs/dbt-core/issues/6449

And it might resolve many or all of these:

  • https://github.com/dbt-labs/dbt-utils/pull/698
  • https://github.com/dbt-labs/dbt-utils/pull/563
  • https://github.com/dbt-labs/dbt-utils/issues/546
  • https://github.com/dbt-labs/dbt-utils/issues/752
  • https://github.com/dbt-labs/dbt-utils/issues/648
  • https://github.com/dbt-labs/dbt-utils/issues/269

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

  • https://github.com/dbt-labs/dbt-utils/pull/476
  • https://github.com/dbt-labs/dbt-utils/pull/351

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:

  • https://github.com/dbt-labs/dbt-utils/blob/1.0.0/macros/sql/get_relations_by_pattern.sql
  • https://github.com/dbt-labs/dbt-utils/blob/1.0.0/macros/sql/get_tables_by_pattern_sql.sql
  • https://github.com/dbt-labs/dbt-utils/blob/1.0.0/macros/sql/get_table_types_sql.sql

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 avatar Jan 31 '23 15:01 dbeatty10

  • 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 avatar Feb 08 '23 02:02 joellabes

This just came up again in https://github.com/dbt-labs/dbt-utils/pull/779

joellabes avatar Apr 19 '23 23:04 joellabes

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.

brendan-cook-87 avatar Oct 13 '23 05:10 brendan-cook-87