Add getting column spec for RowIterator #337
Conversation
Exposing column spec through iterator allows us to get column names and to stay consistent with QueryResult's api.
Tests for simple queries and using prepared statement ones are slightly amended to check column_spec getter for RowIterator in query_iter() and execute_iter().
scylla/src/transport/session_test.rs
Outdated
| for (spec, name) in query_result.get_column_specs().iter().zip(["a", "b", "c"]) { | ||
| assert_eq!(spec.name, name); // Check column name. | ||
| assert_eq!(spec.table_spec.ks_name, "ks"); | ||
| } |
There was a problem hiding this comment.
This test doesn't detect the case when the get_column_specs is empty. The zip method takes two iterators and produces a new one which stops after the shorter of them finishes (doc link) - so if the columns specs slice is empty, the loop will immediately stop.
Besides, it's precisely what happens now. The way that the RowIterator is implemented now is that it caches a single page of rows and lazily updates it after the page is exhausted and the next method is called - but the iterator starts with a dummy, empty page (see e.g. here) which has empty metadata. So, until you call next() on the iterator for the first time, you will get empty metadata.
To solve this problem, let's change the Session::execute_iter and Session::query_iter so that they return an iterator already initialized with the first page. Most of the changes needed to implement this should happen in RowIterator::new_for_query and RowIterator::new_for_prepared_statement.
iterator: changed creating RowIterator for queries and prepared queries, so they do load first page of the result. Changed new_for_query and new_for_prepared_statement into async functions as the consequence. session: changed invoking new_for_query and new_for_prepared_statement in compliance with above changes. Before these changes we were allowing for constructing an iterator in an invalid state - i.e. we could get empty metadata unless we called next method at least once.
…ere passing for empty metadata.
| @@ -146,16 +146,24 @@ impl RowIterator { | |||
|
|
|||
| tokio::task::spawn(worker_task); | |||
|
|
|||
| RowIterator { | |||
| let pages_received = receiver.recv().await.unwrap()?; | |||
There was a problem hiding this comment.
It's not really obvious if the worker always pushes at least one value before the channel is closed. If not, the unwrap() here will trigger a panic which will crash the application - and that is something we would like to avoid.
Let's be defensive and use unwrap_with_default() instead - it will return a Default::default() page if the channel does not produce any page.
Additionally, the variable holds only one page, so the page in the variable name should be singular (page_received).
The same comments apply to the logic in new_for_prepared_statement.
There was a problem hiding this comment.
After our conversation in private I was convinced that unwrapping here is OK. It's a bug if there are no pages returned.
|
Merged. The commits needed some cleaning up, so in order not to delay merging this PR further I reorganized them a little before merging. Some tips for the future:
|
Fixes: #336
We add a getter for
ColumnSpecfor*_iterquery methods.Enables users to learn about column names when they are using
*_itermethods.I took implementation from cvybhu@6bf7182 and made tests for it.
Pre-review checklist
Fixes:annotations to PR description.