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.

Sign up for GitHub

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

Jump to bottom

[CORE-15] SQLQueryStatus event expects get_response to return AdapterResponse object only #4433

Closed
jtcohen6 opened this issue Dec 3, 2021 · 2 comments · Fixed by #4463
Closed

[CORE-15] SQLQueryStatus event expects get_response to return AdapterResponse object only #4433

jtcohen6 opened this issue Dec 3, 2021 · 2 comments · Fixed by #4463
Assignees
@nathaniel-may @McKnight-42
Labels
adapter_plugins Issues relating to third-party adapter plugins bug Something isn't working Team:Adapters Issues designated for the adapter area of the code
Milestone
v1.0.1

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented Dec 3, 2021 •
edited
Loading

See dbt-labs/dbt-spark#265 for context

My proposal would be:

  • v1.0.1: Check the type of the value returned by get_response, then either use its _message attribute (if AdapterResponse) or itself (if str)
  • v1.1.0: Enforce that the get_response abstract method must return an AdapterResponse — not a bad change, and it's effectively true, but we should wait for a minor version release, with clearer communication ahead of time
@jtcohen6 jtcohen6 added bug Something isn't working adapter_plugins Issues relating to third-party adapter plugins Team:Adapters Issues designated for the adapter area of the code labels Dec 3, 2021
@jtcohen6 jtcohen6 mentioned this issue Dec 7, 2021
Closed
@McKnight-42 McKnight-42 mentioned this issue Dec 9, 2021
Merged
4 tasks
@jtcohen6 jtcohen6 added this to the v1.0.1 milestone Dec 10, 2021
@leahwicz leahwicz changed the title SQLQueryStatus event expects get_response to return AdapterResponse object only [CORE-15] SQLQueryStatus event expects get_response to return AdapterResponse object only Dec 13, 2021
@nathaniel-may
Copy link
Contributor

re-opening since #4463 only solved the first bullet.

@nathaniel-may nathaniel-may reopened this Dec 14, 2021
@jtcohen6 jtcohen6 mentioned this issue Dec 16, 2021
Closed
@jtcohen6
Copy link
Contributor Author

Opened #4499 to reflect future work. Closing this one as resolved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nathaniel-may nathaniel-may

@McKnight-42 McKnight-42

Labels
adapter_plugins Issues relating to third-party adapter plugins bug Something isn't working Team:Adapters Issues designated for the adapter area of the code
Projects
None yet
Milestone
v1.0.1
Development

Successfully merging a pull request may close this issue.

made change to test of str dbt-labs/dbt-core
3 participants
@nathaniel-may @jtcohen6 @McKnight-42