Feature/support sql dot jinja

[Luttik]

resolves #3484

Description

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 next" section.

[Luttik]

@jtcohen6 Can you help me with some questions during this process? First one: I couldn't find dbt next in the changelog. So I prepended the file with this section is that the correct behavior or should I move this?

[Luttik]

@jtcohen6 Yet another question. I see that a unittest is failing (seems like the same unittest in 5 different scenarios) but I fail to see how this is affected by my changes. Could you point me in the right direction to get me to use your test flow with breakpoints? Simply running pytest will not work in the debt scenario :/

[jtcohen6]

@Luttik Thanks for the contribution!

As far as debugging test failures: You can insert breakpoint() / idpb.set_trace() and then run with the -s flag (shortcut for --capture=no, though I remember it as short for "show").

In this case, it's a pretty thorny unit test that failed, since it "magically mocks" a python dictionary into a set of model .sql files. In so doing, it takes a couple clever shortcuts, with the upshot that dbt (given the changes in this PR) was reading those model files twice. Hence, the duplication error: dbt found two resources with the name "model_one".

I think I found a straightforward fix, by changing this line:
https://github.com/fishtown-analytics/dbt/blob/4d246567b937f9707ed748992912972c4698e068/test/unit/test_graph.py#L78
To be more prescriptive:

if iter_self.extension != '.sql':

Voila:

$ python3 -m pytest test/unit/test_graph.py
======================================================= test session starts =======================================================
platform darwin -- Python 3.9.5, pytest-6.2.4, py-1.10.0, pluggy-0.13.1
rootdir: /Users/jerco/dev/product/dbt, configfile: tox.ini
plugins: logbook-1.2.0, flaky-3.7.0, xdist-2.3.0, dotenv-0.5.2, forked-1.3.0
collected 6 items

test/unit/test_graph.py ......                                                                                              [100%]

======================================================== 6 passed in 2.46s ========================================================

[Luttik]

Ok so the tests succeed. The remaining steps are:

• Adding new tests.
• Manual testing in a real project.

I am doing this as a hobby but I expect to be able to work on this in the coming weekend.

[Luttik]

So the good thing is that the test suite now reflects the issues that I was experiencing in an actual project. The bad aspect is that I need to unravel the monster method that is dbt.parser.manifest.ManifestLoader.load. Which is the issue that I mentioned above... @jtcohen6 You've been really helpful so far. Any pointers?

[jtcohen6]

@Luttik Sorry for the delay on my end here. The failing tests you're running into are pretty thorny. I can try to take a look when I have a chance to put on my serious debugging hat.

[Luttik]

@jtcohen6 Yeah, I thought the same. It looked to me that it might even be beneficial to start with refactoring the ManifestLoader (especially the load function) a bit. I think that it could pay huge dividends in terms of overall code quality and interpretability and would make debugging issues like these much easier.

Would you guys be open to refactors like that?

[Luttik]

@jtcohen6 I think this would still be a great addition. Could you spare some resources to help me implement this?

[jtcohen6]

There's definitely a lot of code that's changed since this PR was opened. I'd also say that, insofar as this one got bogged down in tricky unit tests, which mock manifests and the like — we're actively looking into refactoring our approaches to unit/integration testing, to prioritize usability.

@dbt-labs/core-language I leave this one up to you — some of the changes that @Luttik is proposing may yield dividends, by introducing the kinds of abstractions / separations (i.e. between file loading vs. manifest construction) that we've been discussing over the past few weeks.

[gshank]

Do you still want help with this? The code has changed so you'd probably need to freshen this up.

[Luttik]

Hé @gshank,

I think that this would be a very relevant change. You'd expect something like this to be a fairly small change too... I can definitely freshen this up, but that only seems relevant if https://github.com/orgs/dbt-labs/teams/core-language has some resources to spend on this too as @jtcohen6 indicated.

[github-actions]

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