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

vyskubov · 2023-05-29T11:25:09Z

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

vadz

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

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.
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?
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 · 2023-06-02T16:06:29Z

Thanks for reviewing!

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.
and 3. I will address these points later.

Thank you again.

vadz · 2023-06-02T16:29:08Z

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 · 2023-08-22T06:33:57Z

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 · 2023-08-23T20:09:09Z

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.

vadz

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.

vadz · 2024-01-02T19:09:39Z

tests/sqlite3/test-sqlite3.cpp

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


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

vadz · 2024-01-02T19:10:08Z

tests/sqlite3/test-sqlite3.cpp

+    std::vector<PhonebookEntry> vec_orm(10);
+    sql << "select * from soci_test", into(vec_orm);
+
+    assert(vec_orm.size() == 2);


vadz · 2024-01-02T19:10:30Z

tests/sqlite3/test-sqlite3.cpp

+    for (std::vector<PhonebookEntry>::iterator it = vec_orm.begin();
+         it != vec_orm.end();
+         ++it)


Could use range-for loop here...

vadz · 2024-01-02T19:11:17Z

include/soci/row.h

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


Suggested change

row(std::size_t bulk_size = 1);

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

vadz · 2024-01-02T19:14:36Z

src/backends/sqlite3/vector-into-type.cpp

@@ -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;


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

vadz · 2024-01-02T19:19:43Z

include/soci/values.h

@@ -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_;


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

vadz · 2024-01-02T19:20:07Z

src/backends/oracle/statement.cpp

+        }        
+        //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;


This seems unrelated to the topic of this PR.

vadz · 2024-01-02T19:21:15Z

src/backends/sqlite3/statement.cpp

-// 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) );
-}
-


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

vadz · 2024-01-02T19:23:04Z

src/backends/sqlite3/statement.cpp

-    else
-        return load_one();
-
+    return (1 == number) ? load_one() : load_rowset(number);


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

vadz · 2024-01-02T19:23:57Z

include/soci/type-holder.h


 namespace soci
 {

 namespace details
 {

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


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

vadz reviewed May 31, 2023

View reviewed changes

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

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

329515e

vyskubov force-pushed the orm_bulk_support branch from 15d6f36 to 329515e Compare June 2, 2023 22:52

asmwarrior mentioned this pull request Aug 22, 2023

ORM: What is the correct way to bulk add a lot of rows? #1066

Open

Merge branch 'master' into orm_bulk_support

5036e5d

vadz requested changes Jan 2, 2024

View reviewed changes

vadz added the Status/Work-In-Progress label Jan 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vyskubov commented May 29, 2023 •

edited

vadz left a comment

vyskubov commented Jun 2, 2023

vadz commented Jun 2, 2023

asmwarrior commented Aug 22, 2023

vyskubov commented Aug 23, 2023 •

edited

vadz left a comment

vadz Jan 2, 2024

vadz Jan 2, 2024

vadz Jan 2, 2024

vadz Jan 2, 2024

vadz Jan 2, 2024

vadz Jan 2, 2024

vadz Jan 2, 2024

vadz Jan 2, 2024

vadz Jan 2, 2024

vadz Jan 2, 2024

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

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

Are you sure you want to change the base?

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

Conversation

vyskubov commented May 29, 2023 • edited

vadz left a comment

Choose a reason for hiding this comment

vyskubov commented Jun 2, 2023

vadz commented Jun 2, 2023

asmwarrior commented Aug 22, 2023

vyskubov commented Aug 23, 2023 • edited

vadz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyskubov commented May 29, 2023 •

edited

vyskubov commented Aug 23, 2023 •

edited