Repository navigation
Spanner: StreamedResult.rows() is confusing and error prone #4414
Description
One of Cloud Spanner customers had a bug where they were iterating on StreamedResultSet.rows and were not getting any data back:
result = transaction.execute_sql(query)
# here result.rows is empty
for row in result.rows:
print(row) # prints nothing
This is kind of working as intended because result.rows will only contain data when consume_next or consume_all is called. But it is still a very confusing API and easy to get wrong. It is also not clear to me at any point in time what rows will be present in this list.
I think we should make the rows private and not expose it to the user. We should do it before marking the library as GA as this is a breaking change.
Activity
danoscarmike commentedon Nov 17, 2017
@vkedia is this only an issue for Python? Do other languages implement it a better way?
vkedia commentedon Nov 17, 2017
I believe so. Definitely
JavaandGodon't do it like this. We will need to check other languages.[-]StreamedResult.rows() is confusing and error prone[/-][+]Spanner: StreamedResult.rows() is confusing and error prone[/+]vkedia commentedon Nov 28, 2017
Another user running into the same issue:
https://stackoverflow.com/questions/47311934/read-with-keysets-ranges
chemelnucfin commentedon Nov 28, 2017
@vkedia I have checked the code and identified a possible problem where the lists are checked for [] and None when it should be checked only for None. However, I'm getting ResourceExhasuted errors and I can't further test the issue.
vkedia commentedon Nov 28, 2017
That usually means that either you are out of disk or you are sending too much load to your spanner instance indicated by CPU usage. You should be able to look at these details in cloud console
vkedia commentedon Nov 28, 2017
BTW in this issue I am more concerned with the API itself rather than any bug in its implementation. I don't think we should expose StreamedResultSet.rows at all since users can just iterator over StreamedResultSet directly.
lukesneeringer commentedon Nov 29, 2017
@chemelnucfin I think all @vkedia is asking for is, "Change the
rowsattribute to be spelled_rows."vkedia commentedon Nov 29, 2017
We might want to remove consume_all, consume_next methods as well.
chemelnucfin commentedon Dec 15, 2017
Closed via #4492 #4583