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

add support of bulk operations for ORM in ORACLE and SQLite backends #1053

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vyskubov
Copy link

@vyskubov vyskubov commented May 29, 2023

Hello everyone,

I am excited to submit this pull request, which (re)introduces support for bulk operations in the SOCI library's ORM (Object-Relational Mapping) functionality. This work is based on the initial groundwork laid by @abc100m, and I have built upon their contributions to enhance the capabilities of the library.

I would appreciate it if the community could review and provide feedback on the changes. Your insights and suggestions are valuable in ensuring the quality and effectiveness of this new feature.

Thank you for your attention and consideration. I look forward to collaborating with you all to further enhance the SOCI library.

Best regards,
Aleksey

Copy link
Member

@vadz vadz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR but I have a few questions just after the first very quick look at it:

  1. Could you please try to minimize the number of unrelated changes? For examples, in several places at(pos) is replaced with [pos] which is not obviously right but even if it is, it would be much better to make it in a separate PR. There are also a lot of whitespace-only changes that really should be avoided.
  2. Why are the new tests different for Oracle and SQLite? Is there some actual difference between the backends functionality and, if so, what is it? And if not, could we please have these tests in the common code?
  3. And, of course, there are the the CI failures, could you please look at them?

I'll try to have a more detailed look at this later, but fixing (1) would be really helpful.

TIA!

@vyskubov vyskubov force-pushed the orm_bulk_support branch 3 times, most recently from 18657fa to 15d6f36 Compare June 2, 2023 15:43
@vyskubov
Copy link
Author

vyskubov commented Jun 2, 2023

Thanks for reviewing!

  1. I have reverted all the whitespace-only changes. I apologize for mistakenly committing them. Regarding the replacement of at(pos) with [pos], these changes were already present in the original PR, so I simply included them as-is. However, I have now reverted these changes for the time being. It's worth mentioning that the usage of at(pos) and [pos] in this file is inconsistent, which could be addressed in a separate PR.
  2. and 3. I will address these points later.

Thank you again.

@vadz
Copy link
Member

vadz commented Jun 2, 2023

1. I have reverted all the whitespace-only changes.

Thanks!

Regarding the replacement of at(pos) with [pos], these changes were already present in the original PR, so I simply included them as-is.

Sorry for not realizing this.

However, I have now reverted these changes for the time being. It's worth mentioning that the usage of at(pos) and [pos] in this file is inconsistent, which could be addressed in a separate PR.

I agree that inconsistencies should be fixed but it's definitely better to do them in a separate PR. Thanks for fixing this up too!

@asmwarrior
Copy link

What is the status of this PR, I think the bulk adding is a very important feature for soci.

I have a similar discussion issue discussion here: ORM: What is the correct way to bulk add a lot of rows? · Issue #1066 · SOCI/soci, in the issue, I have to use a for loop to add many rows, but I do need a simple way to do that.

Thanks.

@vyskubov
Copy link
Author

vyskubov commented Aug 23, 2023

I apologize for for temporarily abandoning this PR. I would like to complete the work on it; however, I currently have more urgent tasks to attend to.

Furthermore, I do not have a readily available development environment on Windows and FreeBSD. Any assistance would be greatly appreciated.

Thank you.

Copy link
Member

@vadz vadz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it doesn't seem like it can be merged in the current state, there are just too many unclear/unrelated things and commented out parts.

I also don't know if we really want to unconditionally replace scalar values with vectors, i.e. use vector of size 1 instead of simple values, it just looks like such a strange idea when we already have both scalar and vector versions.

Of course, maybe I just misunderstand things, but it's really hard to guess what is the intention/rationale here without any comments anywhere in sight.

{
std::vector<std::string> vec_name { "John", "James" };
sql << "insert into soci_test(name) values(:name)", use(vec_name);
assert(vec_name.size() == 2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these asserts really need to be changed to CHECK()s.

std::vector<PhonebookEntry> vec_orm(10);
sql << "select * from soci_test", into(vec_orm);

assert(vec_orm.size() == 2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too.

Comment on lines +720 to +722
for (std::vector<PhonebookEntry>::iterator it = vec_orm.begin();
it != vec_orm.end();
++it)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use range-for loop here...

@@ -40,7 +46,7 @@ class SOCI_DECL column_properties
class SOCI_DECL row
{
public:
row();
row(std::size_t bulk_size = 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
row(std::size_t bulk_size = 1);
explicit row(std::size_t bulk_size = 1);

@@ -132,8 +169,16 @@ void sqlite3_vector_into_type_backend::post_fetch(bool gotData, indicator * ind)
int const endRow = static_cast<int>(statement_.dataCache_.size());
for (int i = 0; i < endRow; ++i)
{
sqlite3_column &col = statement_.dataCache_[i][position_-1];

std::shared_ptr<sqlite3_column> col1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we use a shared pointer here? It doesn't seem to be shared with anybody...

@@ -243,7 +246,7 @@ class SOCI_DECL values
std::vector<details::standard_use_type *> uses_;
std::map<details::use_type_base *, indicator *> unused_;
std::vector<indicator *> indicators_;
std::map<std::string, std::size_t> index_;
std::map<std::string, std::size_t, CaseInsensitiveComparator> index_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this need to be changed to be case-insensitive?

Comment on lines +301 to +305
}
//create table T(t number)--> select t+0 from T; we get: scale=precision=0, size=22
else if (dbscale == 0 && dbprec == 0 && dbsize == 22)
{
type = dt_double;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated to the topic of this PR.

Comment on lines -22 to -27
// This is used instead of tolower() just to avoid warnings about int to char
// casts inside MSVS std::transform() implementation.
char toLowerCh(char c) {
return static_cast<char>( std::tolower(c) );
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this and inlining it looks a completely gratuitous change, was there really some reason for doing it?

else
return load_one();

return (1 == number) ? load_one() : load_rowset(number);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like hasVectorIntoElements_ is not used at all now and should be removed?


namespace soci
{

namespace details
{

// this class is for pass test [commontests.h, test12()],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems completely unrelated to the subject of this PR...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants