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

[CT-3216] [Regression] Calls to get_catalog are not all wrapped in the feature flag #8846

Closed
2 tasks done
mikealfare opened this issue Oct 12, 2023 · 3 comments · Fixed by #8856
Closed
2 tasks done
Assignees
Labels
bug Something isn't working Impact: Adapters pre-regression Regression not yet in a stable release

Comments

@mikealfare
Copy link
Contributor

mikealfare commented Oct 12, 2023

Is this a regression in a recent version of dbt-core?

  • I believe this is a regression in dbt-core functionality
  • I have searched the existing issues, and I could not find an existing issue for this regression

Current Behavior

Calls to the new get_catalog method fail due to providing too many arguments (the instance plus two arguments). This line causes the error:

catalog_table, exceptions = adapter.get_catalog(self.manifest, selected_nodes)

Expected/Previous Behavior

Calls to get_catalog work so that docs generate runs properly.

Steps To Reproduce

Try to run docs generate to trigger a call to get_catalog.

Relevant log output

https://github.com/databricks/dbt-databricks/actions/runs/6499031208/job/17651537811

Environment

- OS:
- Python:
- dbt (working version): dbt-core/dbt-databricks 1.7.0rc1
- dbt (regression version): dbt-core/dbt-databricks 1.6

Which database adapter are you using with dbt?

other (mention it in "Additional Context")

Additional Context

dbt-databricks 1.7.0rc1

@mikealfare mikealfare added bug Something isn't working triage regression labels Oct 12, 2023
@github-actions github-actions bot changed the title [Regression] Calls to get_catalog are not all wrapped in the feature flag [CT-3216] [Regression] Calls to get_catalog are not all wrapped in the feature flag Oct 12, 2023
@dbeatty10 dbeatty10 added the pre-regression Regression not yet in a stable release label Oct 12, 2023
@dbeatty10
Copy link
Contributor

@mikealfare do you think the root cause for the particular error you observed is that dbt-databricks overrides get_catalog here?

Either way, do we have a proposed fix yet?

@mikealfare
Copy link
Contributor Author

@mikealfare do you think the root cause for the particular error you observed is that dbt-databricks overrides get_catalog here?

Either way, do we have a proposed fix yet?

Yes, the fact that it's overridden is a factor. We added an optional argument to get_catalog and then called get_catalog somewhere else using the new argument. If an adapter does not override get_catalog, it will work fine. But if they do, then we will provide too many arguments to get_catalog. On the surface, the solution appears to be wrapping the call to get_catalog in the new feature flag framework (Capability) since the reason we added the argument is also covered by that framework. We could also try wrapping it in a simple try/catch with the exception path omitting the extra argument. These would have the same effect. That being said, we still need to discuss it to make sure we're making the right decision.

@peterallenwebb
Copy link
Contributor

See #8856 for a potential fix.

After discussions with @gshank and @mikealfare I found that a solid fix was going to be more complicated than we anticipated at first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Impact: Adapters pre-regression Regression not yet in a stable release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants