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-3020] More defensive code around fetchmany in SQLConnectionManager.get_result_from_cursor #8471

Closed
1 task done
jtcohen6 opened this issue Aug 23, 2023 · 3 comments
Closed
1 task done
Labels
adapter_plugins Issues relating to third-party adapter plugins

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented Aug 23, 2023

Housekeeping

  • I am a maintainer of dbt-core

Short description

In #7545, we changed dbt show --limit to apply the limit while fetching the result set from the database cursor, instead of after the full result set was already fetched and loaded into memory.

if limit:
rows = cursor.fetchmany(limit)

However, multiple adapter maintainers weren't aware that they needed to add a fetchmany method, leading to issues in some externally maintained adapters upon upgrading to v1.6:

Instead of raising an ugly error ('DatabricksSQLCursorWrapper' object has no attribute 'fetchmany'), we should:

  • Add some defensive code, to first verify the existence of a fetchmany method
  • If no such method is supported for this adapter/platform's cursor, raise a warning, and apply the limit after fetching the result instead (the previous behavior from v1.5)

Acceptance criteria

  • It is possible to run dbt show --limit 10 on an adapter that inherits from the "SQL adapter," and does not support a fetchmany method. The final result set should still be limited to 10. (I imagine we'd want a unit test to mock this scenario.)
  • We have added a test case to dbt-tests-adapter for dbt show --limit. Adapter maintainers know to inherit & use this test case when validating their upgrade to v1.6. We have reflected the need for a fetchmany method in the v1.6 adapter maintainer guide: Adapter Maintainers: Upgrading to dbt-core `1.6.0` #7958.

Impact to Other Teams

Adapters

Will backports be required?

1.6.latest

Context

No response

@jtcohen6 jtcohen6 added bug Something isn't working adapter_plugins Issues relating to third-party adapter plugins backport 1.6.latest Impact: Adapters labels Aug 23, 2023
@github-actions github-actions bot changed the title More defensive code around fetchmany in SQLConnectionManager.get_result_from_cursor [CT-3020] More defensive code around fetchmany in SQLConnectionManager.get_result_from_cursor Aug 23, 2023
@martynydbt
Copy link

martynydbt commented Aug 28, 2023

Related: #8496 if we don't continue with fetchmany we shouldn't do this ticket.

@jtcohen6
Copy link
Contributor Author

There may be some benefit to continuing to use fetchmany, in addition to templating the --limit into the query. That's an implementation detail for whomever picks up #8496.

If we continue to call fetchmany at all, we need more defensive code wrapping it, for adapters that have not implemented the method (this ticket).

If we stop calling fetchmany entirely, then this issue can be closed as "not planned."

@martynydbt
Copy link

@MichelleArk is this ticket still needed?

@martynydbt martynydbt closed this as not planned Won't fix, can't repro, duplicate, stale Sep 26, 2023
@martynydbt martynydbt removed bug Something isn't working backport 1.6.latest Impact: Adapters High Severity bug with significant impact that should be resolved in a reasonable timeframe labels Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapter_plugins Issues relating to third-party adapter plugins
Projects
None yet
Development

No branches or pull requests

3 participants