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

[RFC] Iterator interface #103

Closed
wants to merge 3 commits into from
Closed

Conversation

zauguin
Copy link
Collaborator

@zauguin zauguin commented Mar 28, 2017

I think we need a iterator based method to retrieve the results.
The problem is, that we need to declare the types and number of columns explicitly somewhere, which makes the syntax ugly. So I'm not sure if this is the right way forward.

This is what it looks like (with C++17 structured bindings):

for(auto &&[_age, _name, _weight] : (db << "select age,name,weight from user where age > ? ;" << 21).as<int, string, double>()) {
	if(_age != age || _name != name) 
		exit(EXIT_FAILURE);
	cout << _age << ' ' << _name << ' ' << _weight << endl;
}

Alternatively it can be implemented by free functions:

for(auto &&[_age, _name, _weight] : as<int, string, double>(db << "select age,name,weight from user where age > ? ;" << 21)) {
	if(_age != age || _name != name) 
		exit(EXIT_FAILURE);
	cout << _age << ' ' << _name << ' ' << _weight << endl;
}

@Killili
Copy link
Collaborator

Killili commented Mar 29, 2017

I think its a great idea to have that. And that it can be "bolted-on" so easily i great,too.

But it makes me wonder if its to far from the core concept the lib has till now, explicitly that we try to have everything as stream-ops.

Something like

db_itr<int,string,int> itr;
db << "SQL" >> itr;

looks more like us i guess.

@zauguin
Copy link
Collaborator Author

zauguin commented Mar 30, 2017

@Killili The current library (seems to be) inspired by the iostream interface.
There the [io]stream_iterator isn't created with stream operators either, so this wouldn't be that consistent either.

Additionally providing iterators without begin/end isn't a good idea IMO. The standard does it, but it interacts poorly with ranged for loops or ranges (should they ever become a part of the standard).
So it would be

row_view<int, string, int> rows;
db << "SQL" >> rows;
// Use rows.begin() now

I think this would be confusing, because we already support operator>> with some container types with completely different semantics.

Additionally I expect the statement to be reusable after I used operator>>, because this is how streams and all other versions of our >> behave. So it would be inconsistent. The same problem exists with the current approach, but here we have at least a different syntax to indicate that there is a difference.

@Killili
Copy link
Collaborator

Killili commented Mar 30, 2017

Maybe we have a misunderstanding here, but youre example is exactly what i meant.

And we use the op>> to indicate that we want a result now. We would just store that result in an iterator instead of "using" it then and there.
As a Ruby fanboy i am very fond of the block syntax we have now (anonymous lambda in c++). And i have only one case where our current approach fails and i have to resort to directly call sqlite functions.
This itr approach would solve that however.
The Problem is that i want to stop processing in the middle of a result set btw. so given that ´itr´

row_set<int,string,int> set;
db << "SQL" >> set;
for( auto res : set ){
  if( something ) break;
}

I just realized that looks like the stuff i had back in the VisualBasic ODBC days ...

@zauguin
Copy link
Collaborator Author

zauguin commented Mar 30, 2017

But with iterators, we don't get a result now. We get a result when we use the iterator. If we store the result in any other way, it becomes independent from the (prepared) statement, but for iterators, the statement can't be reset yet. This can be surprising and lead to bugs. But it I probably isn't be that much of a problem, because you normally don't try to execute the same statement again while still processing the results of the last run.

My bigger problem is, that

row_set<int> set;
db << "SQL" >> set;
for( auto res : set ){
  if( something ) break;
}

would be valid, reasonable code. So is

vector<int> set;
db << "SQL" >> set;
for( auto res : set ){
  if( something ) break;
}

They look very similar, but have completely different semantics.

@Killili
Copy link
Collaborator

Killili commented Mar 30, 2017

I see youre point, but i dont think thats a problem in the mind of a programmer that is used to a strongly typed language. In other languages it might be a disaster ;)

@Killili
Copy link
Collaborator

Killili commented Mar 30, 2017

So the other problem would be something like this

auto stmt = db << "SQL";
row_set<int,int> set_itr;
stmt >> set_itr;
auto res = set_itr++;
/* some more lines of code... */
row_set<int,int> new_set_itr;
stmt >> new_set_itr; // get new result set
res = set_itr++; // reuse first itr 

i guess invalidating the itr if the statment gets reused would be the sane thing to do and would be in line with the normal itr handling. Namely invalidating itr on change of the underlying object.

@zauguin
Copy link
Collaborator Author

zauguin commented Mar 30, 2017

It's not a disaster for C++ programmers, but I still think it confusing because we map two related, but distinct functionalities to the same syntax.

There is an additional problem on the implementation side. The row_view has to capture the prepared statement, if it is a rvalue, but it only references it if it is a lvalue. This is necessary to support db << "SQL" >> iter; and auto stmt = db << "SQL"; stmt >> iter;. With the original iinterface this was solved by returning two different classes depending on the value type. For operator>> the class has to be the same in all cases. This can be done (prototype in my iterator2 branch), but it's not really 'clean', because we clutter a simple view with unnecessary functionality.

@Killili
Copy link
Collaborator

Killili commented Mar 30, 2017

We have a lot of different functionality in the op>>

The ones i know of:

  • extract single value to lvalue
  • execute functor per row
  • execute lambda per row
  • extract serialized vector
  • extract single row as tuple

so i guess having one more does not hurt.

@zauguin
Copy link
Collaborator Author

zauguin commented Feb 7, 2018

Replaced by #126.

@zauguin zauguin closed this Feb 7, 2018
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