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

ORM: the query result rowset becomes invalid after a for loop #1064

Open
asmwarrior opened this issue Jul 27, 2023 · 6 comments
Open

ORM: the query result rowset becomes invalid after a for loop #1064

asmwarrior opened this issue Jul 27, 2023 · 6 comments

Comments

@asmwarrior
Copy link

I have a very strange issue, when I got a soci::rowset of the query result. I can use a for loop to print its values, but if I run the for loop again on the same rowset, the iterator got invalid.

Here is a simple test code:

            soci::rowset<person> rs = (sql.prepare << "SELECT * FROM testtable");

            // Loop through rowset using a range-based for loop
            std::cout << "first for loop" << std::endl;
            for (const auto& row : rs) {
                std::cout << row.id << std::endl;
            }
            std::cout << "second for loop" << std::endl;
            for (const auto& row : rs) {
                std::cout << row.id << std::endl;
            }

The result is: the first for loop can print the results, suppose I have two result items, and their ids will be printed. But the second for loop just did nothing, none of the items were returned.

The person class is very simple:

class person
{
public:
    int id;
    std::string name;
    int age;
};

namespace soci
{
    template<>
    struct type_conversion<person>
    {

        typedef values base_type;

        static void from_base(const values& v, indicator ind, person& row)
        {
            row.id =   v.get<int>("id", -1);
            row.name = v.get<std::string>("name", "");
            row.age =  v.get<int>("age", 0);

        }

        static void to_base(const person& row, values& v, indicator& ind)
        {
            v.set("id",   row.id);
            v.set("name", row.name);
            v.set("age",  row.age);

            ind = i_ok;
        }
    };
}

This looks very strange, that the rowset got invalid after the first loop.

The original question in-fact comes from the another issue, that I would like to convert the rowset to a std::vector<persion>, but it looks like the iterator got invalid if I run a for loop, see my test code here:

#1020 (comment)

Any one can explain this?
Thanks.

@asmwarrior asmwarrior changed the title ORM: the query result row_set becomes invalid after a for loop ORM: the query result rowset becomes invalid after a for loop Jul 27, 2023
@asmwarrior
Copy link
Author

When reading the document: soci/docs/statements.md at master · SOCI/soci

It looks like the prepare is very special?

I mean the row object(instance) is only constructed when they are accessed in the for loop? After that, they just got destroyed?

@asmwarrior
Copy link
Author

asmwarrior commented Jul 27, 2023

OK, I just debugged for a while, I have change the definition of person class like below, and try to check how many time the constructor and the destructor got called.

class person
{
public:
    person()
    {
        std::cout << "person constructor" << std::endl;
    }

    person(const person & old)
    {
        id = old.id;
        name = old.name;
        age = old.age;
        std::cout << "copy constructor" << std::endl;
    }

    ~person()
    {
        std::cout << "destroy id = " << id << std::endl;
    }
public:
    int id;
    std::string name;
    int age;
};

The result is: The constructor and the destructor only called once. And the person instance get reused in every for loop.

So, I see only one instance of person object. This is used for the performance improvement?

@zann1x
Copy link
Contributor

zann1x commented Jul 28, 2023

When using rowset, you only get a convenience wrapper around statement, with which you can access data via an iterator. It is not a container for results that you can iterate over multiple times. You could more or less rewrite it as

Person person;
soci::statement stmt = (sql.prepare << "SELECT * FROM testtable", soci::into(person));
stmt.execute();

while (stmt.fetch()) {
    std::cout << person.id << std::endl;
}

A stmt.fetch() is exactly what's happening when advancing a rowset_iterator. When fetch() returns false, i.e. no more data is available, the iterator becomes invalid. Just like can't re-use stmt after the loop, you cannot re-use rowset's iterator.

@asmwarrior
Copy link
Author

asmwarrior commented Jul 29, 2023

Hi, @zann1x thanks for the explanation. I strongly suggest that your comments should be put in the document of soci. Because this will happens for all kinds of beginners like me. What do you think?

Another issue I see is that in your comments in this ticket (ORM and rowset · Issue #1020 · SOCI/soci), you try to use the move semantic, such as

std::vector<person> ret{std::make_move_iterator(rs.begin()), std::make_move_iterator(rs.end())};

But since there is only one instance of "Person" instance, I think the 'move" semantic is not necessary. I mean the normal constructor like below is enough.

std::vector<person> ret{s.begin(), rs.end()};

What do yo think about this?

Thanks.

@zann1x
Copy link
Contributor

zann1x commented Aug 6, 2023

Hi, @zann1x thanks for the explanation. I strongly suggest that your comments should be put in the document of soci. Because this will happens for all kinds of beginners like me. What do you think?

That would surely be a great addition to the docs. Feel free to create a merge request for it!

Another issue I see is that in your comments in this ticket (ORM and rowset · Issue #1020 · SOCI/soci), you try to use the move semantic, such as

std::vector<person> ret{std::make_move_iterator(rs.begin()), std::make_move_iterator(rs.end())};

But since there is only one instance of "Person" instance, I think the 'move" semantic is not necessary. I mean the normal constructor like below is enough.

std::vector<person> ret{s.begin(), rs.end()};

What do yo think about this?

It is true that only a single instance of Person is created, but the move operation also ensures that all the individual variables of the struct aren't copied unnecessarily. For every fetched row, a value is copied into each every variable of Person in type_conversion::from_base. To get the values into the final std::vector, we would have to copy them again. Moving them instead and basically extracting them from the initial Person instance without additional memory allocations feels like the correct thing to do for me.

@asmwarrior
Copy link
Author

Hi, thanks for the explanation. Though I'm still not quite understand the move structure, I will try to learn it more.

About the document improvement, I will do it in the future. (I'm not a native English speaker, so writing some document is a bit hard for me, but I will try it later)

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

No branches or pull requests

2 participants