Make internal macros use macro dispatch to be overridable in child adapters #320

ueshin · 2022-04-04T20:10:51Z

resolves #319

Description

Makes internal macros use macro dispatch to be overridable in child adapters.

The target macros are:

file_format_clause
location_clause
options_clause
comment_clause
partition_cols
clustered_cols
create_temporary_view

Checklist

I have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
I have updated the CHANGELOG.md and added information about my change to the "dbt-spark next" section.

…apters.

ueshin · 2022-04-05T00:00:47Z

@jtcohen6 Do you think this will be backported to 1.0.latest? If not, we might need to apply the original approach to the 1.0.latest branch in dbt-databricks. Thanks.

jtcohen6

One requested change, then this is good to go. Thanks for the quick work!

@jtcohen6 Do you think this will be backported to 1.0.latest? If not, we might need to apply the original approach to the 1.0.latest branch in dbt-databricks. Thanks.

@leahwicz What do you think? I'd be willing to backport this to 1.0.latest, given that:

it's a total non-change for dbt-spark end users (overriding location_clause() by name will still work just as before)
it will enable dbt-databricks to patch this behavior for folks migrating from dbt-spark v1.0.latest to dbt-databricks v1.0.latest

jtcohen6 · 2022-04-05T10:47:56Z

dbt/include/spark/macros/adapters.sql

@@ -1,19 +1,33 @@
 {% macro file_format_clause() %}
+  {{ return(adapter.dispatch('file_format_clause')()) }}


For this and the other macros below, could you provide the second argument to dispatch (macro_namespace) as 'dbt'?

Suggested change

{{ return(adapter.dispatch('file_format_clause')()) }}

{{ return(adapter.dispatch('file_format_clause', 'dbt')()) }}

This enables users to install packages with overrides for "global" (core/plugin) macros, and choose to prefer the package's version over the global version by specifying a project config like:

# dbt_project.yml dispatch: - macro_namespace: dbt search_order: ['my_project', 'package_with_custom_overrides', 'dbt']

Docs: https://docs.getdbt.com/reference/dbt-jinja-functions/dispatch#overriding-global-macros

(Meta comment: The syntax here has developed organically over time, so it can be a bit clunky. Some thoughts about how we could make it more ergonomic in the future: dbt-labs/dbt-core#4646)

Sure, thanks for letting me know! Updated.

jtcohen6 · 2022-04-05T10:48:34Z

tests/unit/test_macros.py

+        def dispatch(macro_name):
+            return getattr(template.module, f'spark__{macro_name}')
+        self.default_context['adapter'].dispatch = dispatch


leahwicz · 2022-04-05T15:44:20Z

@jtcohen6 if this is just a minimal internal change, the yes we can backport it to 1.0.latest to get into the RC.

@ueshin let me know what your timeline is for getting this change in so I can plan accordingly for the RC release. I was hoping to get to it in the next day or 2 if possible

ueshin · 2022-04-05T17:23:01Z

@leahwicz Thanks for taking care of this issue.

let me know what your timeline is for getting this change in so I can plan accordingly for the RC release. I was hoping to get to it in the next day or 2 if possible

As I've addressed @jtcohen6's comment, this must be good to go unless there are any other comments.

jtcohen6

LGTM, thanks @ueshin!

One integration test failed for a reason that appears unrelated to the changes here. Rerunning now. If it fails again I'll try taking a deeper look.

jtcohen6 · 2022-04-05T20:30:36Z

Rerunning now. If it fails again I'll try taking a deeper look.

The same test failed again, only when running against a Databricks cluster, only via ODBC connection: test_delta_comments_databricks_cluster

Trying to dig in to figure out why.

jtcohen6 · 2022-04-05T21:19:34Z

I haven't been able to reproduce locally, or by running the exact same SQL against a cluster in our Databricks workspace.

Looks like this one has failed flakily for us before. Here's a run from 10 days ago, against main, same exact failure.

It also sounds like other users have also seen the same error message crop up intermittently: #293

Spark adapter: ('42000', '[42000] [Simba][Hardy] (80) Syntax or semantic analysis error thrown in server while executing query. Error message from server: org.apache.hive.service.cli.HiveSQLException: Error running query: org.apache.spark.sql.AnalysisException: org.apache.hadoop.hive.ql.metadata.HiveException: at least one column must be specified for the table\n\tat org.apache.spark.sql.hive.thriftserver.Spa (80) (SQLExecDirectW)')

We could enable query retries (retry_all) for our integration tests?

I'm trying one last rerun, but just about willing to say this is a flaky failure, and unrelated to any substantive changes in this PR.

…apters (#320) * Make internal macros use macro dispatch to be overridable in child adapters. * changelog * Address a comment. * Fix. * Fix.

…apters (#320) (#322) * Make internal macros use macro dispatch to be overridable in child adapters. * changelog * Address a comment. * Fix. * Fix. Co-authored-by: Takuya UESHIN <[email protected]>

ueshin · 2022-04-06T17:43:51Z

@jtcohen6 Thanks for merging and backporting this!

Make internal macros use macro dispatch to be overridable in child ad…

a096481

…apters.

cla-bot bot added the cla:yes label Apr 4, 2022

changelog

d4c1498

jtcohen6 self-requested a review April 4, 2022 20:56

ueshin mentioned this pull request Apr 4, 2022

Make internal macros use macro dispatch pattern databricks/dbt-databricks#72

Merged

jtcohen6 requested changes Apr 5, 2022

View reviewed changes

ueshin added 3 commits April 5, 2022 10:04

Address a comment.

dcc82db

Fix.

e750d0e

Fix.

643f917

jtcohen6 approved these changes Apr 5, 2022

View reviewed changes

jtcohen6 merged commit fe77af7 into dbt-labs:main Apr 6, 2022

ueshin deleted the macro_dispatch branch April 6, 2022 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make internal macros use macro dispatch to be overridable in child adapters #320

Make internal macros use macro dispatch to be overridable in child adapters #320

ueshin commented Apr 4, 2022 •

edited

Loading

ueshin commented Apr 5, 2022 •

edited

Loading

jtcohen6 left a comment

jtcohen6 Apr 5, 2022

ueshin Apr 5, 2022

jtcohen6 Apr 5, 2022

leahwicz commented Apr 5, 2022

ueshin commented Apr 5, 2022

jtcohen6 left a comment

jtcohen6 commented Apr 5, 2022 •

edited

Loading

jtcohen6 commented Apr 5, 2022 •

edited

Loading

ueshin commented Apr 6, 2022

		@@ -1,19 +1,33 @@
		{% macro file_format_clause() %}
		{{ return(adapter.dispatch('file_format_clause')()) }}

Make internal macros use macro dispatch to be overridable in child adapters #320

Make internal macros use macro dispatch to be overridable in child adapters #320

Conversation

ueshin commented Apr 4, 2022 • edited Loading

Description

Checklist

ueshin commented Apr 5, 2022 • edited Loading

jtcohen6 left a comment

Choose a reason for hiding this comment

jtcohen6 Apr 5, 2022

Choose a reason for hiding this comment

ueshin Apr 5, 2022

Choose a reason for hiding this comment

jtcohen6 Apr 5, 2022

Choose a reason for hiding this comment

leahwicz commented Apr 5, 2022

ueshin commented Apr 5, 2022

jtcohen6 left a comment

Choose a reason for hiding this comment

jtcohen6 commented Apr 5, 2022 • edited Loading

jtcohen6 commented Apr 5, 2022 • edited Loading

ueshin commented Apr 6, 2022

ueshin commented Apr 4, 2022 •

edited

Loading

ueshin commented Apr 5, 2022 •

edited

Loading

jtcohen6 commented Apr 5, 2022 •

edited

Loading

jtcohen6 commented Apr 5, 2022 •

edited

Loading