Skip to content

Add support for temp_table_name override #146

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

Merged
merged 2 commits into from
May 25, 2022

Conversation

bcwaldon
Copy link
Contributor

This enables clients that run more than one bulk import
operation concurrently to avoid table naming conflicts.

This enables clients that run more than one bulk import
operation concurrently to avoid table naming conflicts.
@bcwaldon bcwaldon requested a review from palewire as a code owner May 16, 2022 20:05
@bcwaldon
Copy link
Contributor Author

@palewire Hi there! I have been evaluating this library for some of my work and came across this little design issue. The content of this PR is the minimum viable solution, and there are many different ways to address the problem. Please let me know if 1) I am missing something about how these temporary tables act such that this is not actually an issue, and 2) how you would like this change to be made and tested. Thanks!

@palewire
Copy link
Owner

This seems reasonable to me. I haven't fully thought it thru, but I suppose an alternative would be having the library append some random string to the end of the table name, or something like that.

I'm not sure I have an opinion of whether that kind of approach would be better or worse, though I guess this new option might be nice even if we did it.

Unless you have further thoughts I think I'm okay with merging this. But I would like to see the option added to our documentation. Could.you please include that as well?

@bcwaldon
Copy link
Contributor Author

This seems reasonable to me. I haven't fully thought it thru, but I suppose an alternative would be having the library append some random string to the end of the table name, or something like that.

I'm not sure I have an opinion of whether that kind of approach would be better or worse, though I guess this new option might be nice even if we did it.

I had that thought as well, but decided to go this direction first since it enables a user to choose to generate a random name themselves, or set a specific name if that is the intent.

Unless you have further thoughts I think I'm okay with merging this. But I would like to see the option added to our documentation. Could.you please include that as well?

Absolutely!

@bcwaldon
Copy link
Contributor Author

@palewire I just pushed up what I believe is the doc change you requested. I have not been able to run tests locally, so please advise if that is something I need to do.

@bcwaldon
Copy link
Contributor Author

Hiya @palewire - is there anything else needed here?

@palewire
Copy link
Owner

I'd like to run the tests more thoroughly yet. Otherwise it looks good. I'm on vacation out of the country for another week so I haven't been at my computer. Do you need the change urgently? If so I can find my laptop and shove it out.

@bcwaldon
Copy link
Contributor Author

I'd like to run the tests more thoroughly yet. Otherwise it looks good. I'm on vacation out of the country for another week so I haven't been at my computer. Do you need the change urgently? If so I can find my laptop and shove it out.

No worries! It would be awesome to get this released properly within the next two weeks. Enjoy your time away :)

@palewire palewire merged commit b78fe51 into palewire:master May 25, 2022
@palewire
Copy link
Owner

This has been merged and released. Thank you for your contribution.

@bcwaldon bcwaldon deleted the temp_table_name branch May 25, 2022 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants