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

Turn on performance improvements by default #3502

Closed
jtcohen6 opened this issue Jun 28, 2021 · 7 comments
Closed

Turn on performance improvements by default #3502

jtcohen6 opened this issue Jun 28, 2021 · 7 comments
Assignees
Labels
1.0.0 Issues related to the 1.0.0 release of dbt enhancement New feature or request performance

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented Jun 28, 2021

Namely:

  • partial parsing
  • experimental parser

This will require accounting for all known "fast-and-incorrect" edge cases and either (or both) of:

  • fixing them
  • documenting them, and what to do when they happen

This does not require accounting for all known "slow-but-correct" edge cases. While strictly better, those can come later, and they won't yield incorrect results in the meantime.

@jtcohen6 jtcohen6 added enhancement New feature or request performance 1.0.0 Issues related to the 1.0.0 release of dbt labels Jun 28, 2021
@leahwicz
Copy link
Contributor

leahwicz commented Jul 8, 2021

  • Can we expand the experimental parsing scope?
  • Depends on the results we get back from metrics
  • Would have to update the tests as well

@leahwicz
Copy link
Contributor

leahwicz commented Jul 8, 2021

Where is the flag for this? In the profile?

@leahwicz
Copy link
Contributor

leahwicz commented Jul 8, 2021

Let's split this into 2 different tickets:

  • Experimental parsing -> would be a 1
  • Partial parsing -> would be a 4-8 with test updates and some additional code changes

@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Jul 9, 2021

Where is the flag for this? In the profile?

Yes! I think CLI flag + profile + env var (+ other configuration mechanism?). I'm looking at our discussion in #2990 re: coordinating configs.

Can we expand the experimental parsing scope?

One thing I had discussed with @nathaniel-may last week: rather than simply setting use_experimental_parser: True for everyone, I think we'll actually want to:

  • Move proven parts of the experimental parser into the default parser (and delete old default code) — it's no longer experimental, everyone gets to have it
  • Keep around the use_experimental_parser flag/config/var, still False by default, as a way for users to always opt into the next phase of not-yet-proven experimentation

Noting this here because it's different from the way we'd turn on partial parsing by default—that we'd more straightforwardly accomplish by setting the default value of partial_parse to True.

@leahwicz
Copy link
Contributor

The experimental parser remaining work is in this ticket: #3377

@jtcohen6 jtcohen6 changed the title Turn on performance improvements by default? Turn on performance improvements by default Aug 11, 2021
@jtcohen6
Copy link
Contributor Author

jtcohen6 commented Sep 1, 2021

Remaining must-haves before we turn these on by default. We can fix/handle (ideal), or document limitations with path to turn off (less desirable).

As far as actually turning on:

  • Partial parsing: set to true by default. Let's also move all partial parsing logging from info-level to debug-level.
  • Experimental parser: bake current use into default model parser; keep around flag/config (false by default) to use with iterative improvements + sampling in the future

@leahwicz
Copy link
Contributor

leahwicz commented Sep 9, 2021

@jtcohen6 jtcohen6 closed this as completed Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0.0 Issues related to the 1.0.0 release of dbt enhancement New feature or request performance
Projects
None yet
Development

No branches or pull requests

4 participants