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

[#3867] Turn on partial parsing by default #3989

Merged
merged 1 commit into from
Oct 1, 2021
Merged

Conversation

gshank
Copy link
Contributor

@gshank gshank commented Oct 1, 2021

resolves #3867

Description

Turn on partial parsing by default.

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.

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Looks like the timestamp format switch threw off a couple integration tests. Assuming those are simple fixes, this LGTM

@@ -41,7 +41,7 @@
"STATIC_PARSER": True,
"WARN_ERROR": False,
"WRITE_JSON": True,
"PARTIAL_PARSE": False,
"PARTIAL_PARSE": True,
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Some parts of the code want the Z ending, and some parts don't, apparently. I moved the kludging of 'generated_at' into a post serialization method in BaseArtifactMetadata.

Comment on lines +561 to +567
# We don't want to have stale generated_at dates
manifest.metadata.generated_at = datetime.utcnow()
# or invocation_ids
if dbt.tracking.active_user:
manifest.metadata.invocation_id = dbt.tracking.active_user.invocation_id
else:
manifest.metadata.invocation_id = None
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@gshank gshank force-pushed the default_partial_parsing branch 2 times, most recently from 24eac63 to f393481 Compare October 1, 2021 18:42
@gshank
Copy link
Contributor Author

gshank commented Oct 1, 2021

The remaining test failure is because we're not handling 'vars' in partial parsing. To fix it the right way we'd have to implement one of the 'vars' solutions we've discussed. (For the case of this test, it's vars passed in as cli args in the rpc server.) Or we could turn off partial parsing for the test.

@gshank
Copy link
Contributor Author

gshank commented Oct 1, 2021

Have modified the tests to use the --no-partial-parse flag. The issues with vars and env_vars are well known and on the roadmap.

Copy link
Contributor

@nathaniel-may nathaniel-may left a comment

Choose a reason for hiding this comment

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

🎉

@gshank gshank merged commit 16b02f4 into develop Oct 1, 2021
@gshank gshank deleted the default_partial_parsing branch October 1, 2021 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Turn on partial parser by default
3 participants