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

Write run results to disk (#829) #904

Merged
merged 7 commits into from
Aug 10, 2018
Merged

Write run results to disk (#829) #904

merged 7 commits into from
Aug 10, 2018

Conversation

beckjake
Copy link
Contributor

@beckjake beckjake commented Aug 6, 2018

Per discussion, we'll be writing the run results to a separate file from the manifest itself. The run results include the compiled node, so the compiled sql is in there.

The actual code changes are small, the tests are just very verbose.

@cmcarthur cmcarthur self-requested a review August 8, 2018 14:48
Copy link
Member

@cmcarthur cmcarthur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super happy with the structure here.

there is one comment here that i think is a bug in dbt/compat.py. the rest is non blocking

def default(self, obj):
if isinstance(obj, Decimal):
return float(obj)
return super(JSONEncoder, self).default(obj)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like the way Python handles JSON encoding. This looks like a good start in handling this better in dbt.

fail = named_property('fail', 'True if this was a test and it failed')
status = named_property('status', 'The status of the model execution')
execution_time = named_property('execution_time',
'The time in seconds to execute the model')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love this

dbt/compat.py Outdated

def write_json(path, data, **kwargs):
with _open(path, 'w') as fp:
json.dump(data, fp, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't love this as part of the public API of dbt.compat, but i see the need for it. is there an advantage to doing it this way, vs calling write_file with json.dumps(...) from dbt.clients.system?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance is different. json.dump() uses less memory because it streams, but can take longer to run because each 'chunk' of json becomes a file.write() call and it bypasses the "one shot" encoder in C. I don't know which is faster in any given situation, I'll just change it to use json.dumps() and hope we never run out of memory, we'll probably gain a lot from using the good encoder anyway.

@@ -4,6 +4,14 @@

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is going to have some rough conflicts w #883

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The merge conflicts will look horrible for sure, but looking at that PR they'll be pretty easy to resolve manually if I rebase on top of that after merge - I really only have to add a couple fields here and there.

dbt/compat.py Outdated
else:
with open(path, 'w') as f:
return f.write(to_string(s))
return open(path, 'w')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you want return open(path, mode)

@beckjake beckjake force-pushed the write-run-results branch from 2a8929c to d05173d Compare August 8, 2018 16:51
@beckjake beckjake changed the base branch from dev/isaac-asimov to development August 9, 2018 15:17
@beckjake beckjake force-pushed the write-run-results branch from d05173d to 5ccaf5b Compare August 9, 2018 15:32
@beckjake beckjake merged commit 02799e5 into development Aug 10, 2018
@beckjake beckjake deleted the write-run-results branch August 10, 2018 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants