-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add a row iterator #24
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not packaged this to test on a sandbox yet but if I'm reading this correctly think how we are discarding array items is reversed.
Overall this is a great idea! Let's get it tested and merged so we can start using it. :)
src/ThriftSQL/ThriftStream.php
Outdated
*/ | ||
public function next() { | ||
$this->location ++; | ||
array_pop( $this->buffer ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be array_shift()
so that we grab and discard the first element in the buffer? Otherwise if we get a resultset of 6 rows in two chunks like so:
---
1
2
3
---
4
5
6
---
Iterating through the set would end up producing (I think):
1
1
1
4
4
4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in f818232
src/ThriftSQL/ThriftStream.php
Outdated
} | ||
|
||
try { | ||
$this->buffer = array_merge( $this->buffer, $this->stream->fetch( 100 ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is array_merge()
needed here seeing as we already know $this->buffer
is empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I guess maybe we can fetch even less rows at a time given that we are going to iterate over the results to reduce memory consumption even more. But 100 is not very many so maybe not worth it to try and tune it any further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 703371f.
src/ThriftSQL.php
Outdated
* | ||
* @param string $queryStr | ||
* | ||
* @return \ThriftSQL\ThriftStream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super minor but seeing as this is already namespaced within \ThriftSQL
why name the iterator \ThriftSQL\ThriftStream
and not something more obvious like simply \ThriftSQL\Iterator
or \ThriftSQL\ResultIterator
? After all it's a straight up implementation of \Iterator
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to \ThriftSQL\Utils\Iterator
in 519e794
README.md
Outdated
|
||
// Try out a Hive query | ||
$hiveIterator = new \ThriftSQL\Hive( 'hive.host.local', 10000, 'user', 'pass' ); | ||
$hiveTables = $hive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this should be $hiveIterator
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 47d3eac
src/ThriftSQL/ThriftStream.php
Outdated
* @since 5.0.0 | ||
* @throws Exception | ||
*/ | ||
public function rewind() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have some sort of first run flag and throw a warning if the same query is run twice? What's a good way to alert the dev that doing something like the following is actually going to run the query twice which is
- Slow
- May produce different results in each loop
$res = $thriftsql->getIterator('SELECT ...');
foreach ( $res as $row ) {
// Do something with $row
}
...
foreach ( $res as $row ) {
// Do something else with $row
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a feature to disable rewind from happening more then once which prevents queries from being rerun unless \ThriftSQL\ThriftStream::allowRerun(true)
is called in 71fc266.
Rebased / merged with latest tagged release to get packagist.org updates working again. |
FYI, might be a good idea to merge with #25 first if you are trying to build the phar to test. |
We don't need to use array merge because we only fill the buffer once it's empty already. We add a class const to set how many rows to keep in the in-memory buffer of the iterator.
Change private var names to match thrift types that they contain so that it's clearer what they are and we avoid having things that say: ``` $this->query->query( $this->queryStr ) ``` Which can be confusing. Also add in strong typing for constructor.
Make the name more obvious by renaming it from: ``` \ThriftSQL\ThriftStream() ``` to: ``` \ThriftSQL\Utils\Iterator() ```
Use an abstract class instead of just an interface so that we don't have to repeate the `getIterator()` method.
@withinboredom can you take a look at the latest diff when you have a chance? |
Update docs to explain different methods to use ThriftSQL and specify the iterator object as the recommended method to fetch results.
- Don't eat any exceptions, let's bubble them up so that the caller / runner can handle them instead of pretending that the query simply returned no results or partial results. - Validity after fetching rows is simply a matter of is there are records in the buffer so just return that.
Allows efficiently iterating over a result set.