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

make calc_batch_size compatible with default__load_csv_rows #179

Closed
dataders opened this issue Nov 11, 2021 · 2 comments · Fixed by #211
Closed

make calc_batch_size compatible with default__load_csv_rows #179

dataders opened this issue Nov 11, 2021 · 2 comments · Fixed by #211
Assignees
Milestone

Comments

@dataders
Copy link
Collaborator

dbt-labs/dbt-core#3623 (released as part of dbt-core v0.21.0) allowed adapters like dbt-sqlserver to not have to re-implement default__load_csv_rows() because the only difference between dbt-sqlserver's and core's global_project was the binding parameter character: ?, and %s respectively.

What I had forgotten about was @jacobm001's calc_batch_size which doesn't really work with default__load_csv_rows.

It'd be nice to not have to reimplement default__load_csv_rows! This might require a PR to dbt-core directly...

@dataders dataders added the help wanted Extra attention is needed label Nov 11, 2021
@jacobm001
Copy link
Contributor

Should be a pretty easy thing to migrate into the typical dbt macros. It'll probably be a couple days before I can get to it, but I can get that done.

@sdebruyn
Copy link
Member

I worked a bit on #211 and made sure it followed the core implementation more closely. The macro to get the batch size in dbt-core doesn't have the model or its columns as argument and we need that to calculate our batch size based on the amount of columns the csv contains. Changing the macro in dbt-core would probably cause incompatibility with adapters which haven't updated yet.

I'd be glad to contribute to dbt-core, but I'd need some advice on how to change the get_batch_size() in dbt-core so that it takes an argument. And which argument would that be? The model, the columns or the amount of columns?

@dataders @jtcohen6 ideas?

@sdebruyn sdebruyn added this to the v1.4.1 milestone May 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants