Skip to content

Commit

Permalink
PR feedback, also remove an extra manifest-building from "dbt docs ge…
Browse files Browse the repository at this point in the history 
…nerate"
  • Loading branch information
Jacob Beck committed Oct 7, 2019
1 parent 5061397 commit 02feb69
Show file tree
Hide file tree
Showing 4 changed files with 134 additions and 24 deletions.
12 changes: 6 additions & 6 deletions core/dbt/task/generate.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import dbt.exceptions

from dbt.task.compile import CompileTask
from dbt.task.runnable import write_manifest


CATALOG_FILENAME = 'catalog.json'
Expand Down Expand Up @@ -178,8 +179,8 @@ def _coerce_decimal(value):

class GenerateTask(CompileTask):
def _get_manifest(self) -> Manifest:
manifest = dbt.loader.GraphLoader.load_all(self.config)
return manifest
# manifest = dbt.loader.GraphLoader.load_all(self.config)
return self.manifest

def run(self):
compile_results = None
Expand All @@ -197,10 +198,8 @@ def run(self):

adapter = get_adapter(self.config)
with adapter.connection_named('generate_catalog'):
manifest = self._get_manifest()

dbt.ui.printer.print_timestamped_line("Building catalog")
catalog_table = adapter.get_catalog(manifest)
catalog_table = adapter.get_catalog(self.manifest)

catalog_data: List[PrimitiveDict] = [
dict(zip(catalog_table.column_names, map(_coerce_decimal, row)))
Expand All @@ -209,13 +208,14 @@ def run(self):

catalog = Catalog(catalog_data)
results = self.get_catalog_results(
nodes=catalog.make_unique_id_map(manifest),
nodes=catalog.make_unique_id_map(self.manifest),
generated_at=datetime.utcnow(),
compile_results=compile_results,
)

path = os.path.join(self.config.target_path, CATALOG_FILENAME)
results.write(path)
write_manifest(self.manifest, self.config)

dbt.ui.printer.print_timestamped_line(
'Catalog written to {}'.format(os.path.abspath(path))
Expand Down
8 changes: 6 additions & 2 deletions core/dbt/task/runnable.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@
MANIFEST_FILE_NAME = 'manifest.json'


def write_manifest(manifest, config):
if dbt.flags.WRITE_JSON:
manifest.write(os.path.join(config.target_path, MANIFEST_FILE_NAME))


def load_manifest(config):
# performance trick: if the adapter has a manifest loaded, use that to
# avoid parsing internal macros twice. Also, when loading the adapter's
Expand All @@ -31,8 +36,7 @@ def load_manifest(config):
internal = adapter.load_internal_manifest()
manifest = GraphLoader.load_all(config, internal_manifest=internal)

if dbt.flags.WRITE_JSON:
manifest.write(os.path.join(config.target_path, MANIFEST_FILE_NAME))
write_manifest(manifest, config)
return manifest


Expand Down
131 changes: 115 additions & 16 deletions test/integration/029_docs_generate_tests/test_docs_generate.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -821,7 +821,7 @@ def expected_seeded_manifest(self, model_database=None):
return {
'nodes': {
'model.test.model': {
'build_path': None,
'build_path': 'target/compiled/test/model.sql',
'name': 'model',
'root_path': self.test_root_dir,
'resource_type': 'model',
Expand Down Expand Up @@ -869,9 +869,17 @@ def expected_seeded_manifest(self, model_database=None):
},
'patch_path': schema_yml_path,
'docrefs': [],
'compiled': True,
'compiled_sql': ANY,
'extra_ctes_injected': True,
'extra_ctes': [],
'injected_sql': ANY,
'wrapped_sql': 'None',
},
'seed.test.seed': {
'build_path': None,
'compiled': True,
'compiled_sql': '',
'config': {
'enabled': True,
'materialized': 'seed',
Expand Down Expand Up @@ -905,10 +913,16 @@ def expected_seeded_manifest(self, model_database=None):
'description': '',
'columns': {},
'docrefs': [],
'compiled': True,
'compiled_sql': '',
'extra_ctes_injected': True,
'extra_ctes': [],
'injected_sql': '',
'wrapped_sql': 'None',
},
'test.test.not_null_model_id': {
'alias': 'not_null_model_id',
'build_path': None,
'build_path': 'target/compiled/test/schema_test/not_null_model_id.sql',
'column_name': 'id',
'columns': {},
'config': {
Expand Down Expand Up @@ -941,10 +955,16 @@ def expected_seeded_manifest(self, model_database=None):
'tags': ['schema'],
'unique_id': 'test.test.not_null_model_id',
'docrefs': [],
'compiled': True,
'compiled_sql': AnyStringWith('count(*)'),
'extra_ctes_injected': True,
'extra_ctes': [],
'injected_sql': AnyStringWith('count(*)'),
'wrapped_sql': AnyStringWith('count(*)'),
},
'test.test.test_nothing_model_': {
'alias': 'test_nothing_model_',
'build_path': None,
'build_path': 'target/compiled/test/schema_test/test_nothing_model_.sql',
'column_name': None,
'columns': {},
'config': {
Expand Down Expand Up @@ -977,10 +997,16 @@ def expected_seeded_manifest(self, model_database=None):
'tags': ['schema'],
'unique_id': 'test.test.test_nothing_model_',
'docrefs': [],
'compiled': True,
'compiled_sql': AnyStringWith('select 0'),
'extra_ctes_injected': True,
'extra_ctes': [],
'injected_sql': AnyStringWith('select 0'),
'wrapped_sql': AnyStringWith('select 0'),
},
'test.test.unique_model_id': {
'alias': 'unique_model_id',
'build_path': None,
'build_path': 'target/compiled/test/schema_test/unique_model_id.sql',
'column_name': 'id',
'columns': {},
'config': {
Expand Down Expand Up @@ -1013,6 +1039,12 @@ def expected_seeded_manifest(self, model_database=None):
'tags': ['schema'],
'unique_id': 'test.test.unique_model_id',
'docrefs': [],
'compiled': True,
'compiled_sql': AnyStringWith('count(*)'),
'extra_ctes_injected': True,
'extra_ctes': [],
'injected_sql': AnyStringWith('count(*)'),
'wrapped_sql': AnyStringWith('count(*)'),
},
},
'parent_map': {
Expand Down Expand Up @@ -1123,7 +1155,7 @@ def expected_postgres_references_manifest(self, model_database=None):
'nodes': {
'model.test.ephemeral_copy': {
'alias': 'ephemeral_copy',
'build_path': None,
'build_path': 'target/compiled/test/ephemeral_copy.sql',
'columns': {},
'config': {
'column_types': {},
Expand Down Expand Up @@ -1160,10 +1192,16 @@ def expected_postgres_references_manifest(self, model_database=None):
'database': self.default_database,
'tags': [],
'unique_id': 'model.test.ephemeral_copy',
'compiled': True,
'compiled_sql': ANY,
'extra_ctes_injected': True,
'extra_ctes': [],
'injected_sql': ANY,
'wrapped_sql': 'None',
},
'model.test.ephemeral_summary': {
'alias': 'ephemeral_summary',
'build_path': None,
'build_path': 'target/compiled/test/ephemeral_summary.sql',
'columns': {
'first_name': {
'description': 'The first name being summarized',
Expand Down Expand Up @@ -1228,10 +1266,17 @@ def expected_postgres_references_manifest(self, model_database=None):
'schema': my_schema_name,
'database': self.default_database,
'tags': [],
'unique_id': 'model.test.ephemeral_summary'},
'unique_id': 'model.test.ephemeral_summary',
'compiled': True,
'compiled_sql': ANY,
'extra_ctes_injected': True,
'extra_ctes': [ANY],
'injected_sql': ANY,
'wrapped_sql': 'None',
},
'model.test.view_summary': {
'alias': 'view_summary',
'build_path': None,
'build_path': 'target/compiled/test/view_summary.sql',
'columns': {
'first_name': {
'description': 'The first name being summarized',
Expand Down Expand Up @@ -1295,7 +1340,13 @@ def expected_postgres_references_manifest(self, model_database=None):
'schema': my_schema_name,
'sources': [],
'tags': [],
'unique_id': 'model.test.view_summary'
'unique_id': 'model.test.view_summary',
'compiled': True,
'compiled_sql': ANY,
'extra_ctes_injected': True,
'extra_ctes': [],
'injected_sql': ANY,
'wrapped_sql': 'None',
},
'seed.test.seed': {
'alias': 'seed',
Expand Down Expand Up @@ -1330,7 +1381,13 @@ def expected_postgres_references_manifest(self, model_database=None):
'schema': my_schema_name,
'database': self.default_database,
'tags': [],
'unique_id': 'seed.test.seed'
'unique_id': 'seed.test.seed',
'compiled': True,
'compiled_sql': '',
'extra_ctes_injected': True,
'extra_ctes': [],
'injected_sql': '',
'wrapped_sql': 'None',
},
'source.test.my_source.my_table': {
'columns': {
Expand Down Expand Up @@ -1365,7 +1422,7 @@ def expected_postgres_references_manifest(self, model_database=None):
},
],
'external': {
'file_format': None, 'location': None, 'partitions': None,
'file_format': None, 'location': None, 'partitions': None,
'row_format': None, 'tbl_properties': None
},
'freshness': {'error_after': None, 'warn_after': None, 'filter': None},
Expand Down Expand Up @@ -1589,7 +1646,7 @@ def expected_bigquery_complex_manifest(self):
'sources': [],
'depends_on': {'macros': [], 'nodes': ['seed.test.seed']},
'fqn': ['test', 'clustered'],
'build_path': None,
'build_path': 'target/compiled/test/clustered.sql',
'name': 'clustered',
'original_file_path': clustered_sql_path,
'package_name': 'test',
Expand Down Expand Up @@ -1632,10 +1689,16 @@ def expected_bigquery_complex_manifest(self):
'description': 'A clustered and partitioned copy of the test model',
'patch_path': self.dir('bq_models/schema.yml'),
'docrefs': [],
'compiled': True,
'compiled_sql': ANY,
'extra_ctes_injected': True,
'extra_ctes': [],
'injected_sql': ANY,
'wrapped_sql': 'None',
},
'model.test.multi_clustered': {
'alias': 'multi_clustered',
'build_path': None,
'build_path': 'target/compiled/test/multi_clustered.sql',
'config': {
'cluster_by': ['first_name', 'email'],
'column_types': {},
Expand Down Expand Up @@ -1694,10 +1757,16 @@ def expected_bigquery_complex_manifest(self):
'description': 'A clustered and partitioned copy of the test model, clustered on multiple columns',
'patch_path': self.dir('bq_models/schema.yml'),
'docrefs': [],
'compiled': True,
'compiled_sql': ANY,
'extra_ctes_injected': True,
'extra_ctes': [],
'injected_sql': ANY,
'wrapped_sql': 'None',
},
'model.test.nested_view': {
'alias': 'nested_view',
'build_path': None,
'build_path': 'target/compiled/test/nested_view.sql',
'config': {
'column_types': {},
'enabled': True,
Expand Down Expand Up @@ -1757,10 +1826,16 @@ def expected_bigquery_complex_manifest(self):
'description': 'The test model',
'patch_path': self.dir('bq_models/schema.yml'),
'docrefs': [],
'compiled': True,
'compiled_sql': ANY,
'extra_ctes_injected': True,
'extra_ctes': [],
'injected_sql': ANY,
'wrapped_sql': 'None',
},
'model.test.nested_table': {
'alias': 'nested_table',
'build_path': None,
'build_path': 'target/compiled/test/nested_table.sql',
'config': {
'column_types': {},
'enabled': True,
Expand Down Expand Up @@ -1794,6 +1869,12 @@ def expected_bigquery_complex_manifest(self):
'columns': {},
'description': '',
'docrefs': [],
'compiled': True,
'compiled_sql': ANY,
'extra_ctes_injected': True,
'extra_ctes': [],
'injected_sql': ANY,
'wrapped_sql': 'None',
},
'seed.test.seed': {
'build_path': None,
Expand Down Expand Up @@ -1832,6 +1913,12 @@ def expected_bigquery_complex_manifest(self):
'columns': {},
'description': '',
'docrefs': [],
'compiled': True,
'compiled_sql': '',
'extra_ctes_injected': True,
'extra_ctes': [],
'injected_sql': '',
'wrapped_sql': 'None',
},
},
'child_map': {
Expand Down Expand Up @@ -1960,7 +2047,7 @@ def expected_redshift_incremental_view_manifest(self):
return {
'nodes': {
'model.test.model': {
'build_path': None,
'build_path': 'target/compiled/test/model.sql',
'name': 'model',
'root_path': self.test_root_dir,
'resource_type': 'model',
Expand Down Expand Up @@ -2022,6 +2109,12 @@ def expected_redshift_incremental_view_manifest(self):
},
'patch_path': self.dir('rs_models/schema.yml'),
'docrefs': [],
'compiled': True,
'compiled_sql': ANY,
'extra_ctes_injected': True,
'extra_ctes': [],
'injected_sql': ANY,
'wrapped_sql': 'None',
},
'seed.test.seed': {
'build_path': None,
Expand Down Expand Up @@ -2060,6 +2153,12 @@ def expected_redshift_incremental_view_manifest(self):
'columns': {},
'description': '',
'docrefs': [],
'compiled': True,
'compiled_sql': ANY,
'extra_ctes_injected': True,
'extra_ctes': [],
'injected_sql': ANY,
'wrapped_sql': 'None',
},
},
'parent_map': {
Expand Down
7 changes: 7 additions & 0 deletions test/integration/048_rpc_test/test_rpc.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -927,6 +927,8 @@ def test_docs_generate_postgres(self):
self.run_dbt_with_vars(['seed'])
self.run_dbt_with_vars(['run'])
self.assertFalse(os.path.exists('target/catalog.json'))
if os.path.exists('target/manifest.json'):
os.remove('target/manifest.json')
result = self.async_query('docs.generate').json()
dct = self.assertIsResult(result)
self.assertTrue(os.path.exists('target/catalog.json'))
Expand All @@ -949,6 +951,11 @@ def test_docs_generate_postgres(self):
}
for uid in expected:
self.assertIn(uid, nodes)
self.assertTrue(os.path.exists('target/manifest.json'))
with open('target/manifest.json') as fp:
manifest = json.load(fp)
self.assertIn('nodes', manifest)
self.assertEqual(len(manifest['nodes']), 17)


class FailedServerProcess(ServerProcess):
Expand Down

0 comments on commit 02feb69

Please sign in to comment.