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

External iterator API. #18

Merged
merged 9 commits into from Feb 8, 2016

Conversation

@Metalaka
Copy link
Member

commented Oct 8, 2014

Derived from: #13 (comment)

The main idea are:
We can define a fetching style with direction, startOffset(, style?).
And we can get an external iterator to fetch the results with the defined style.

Not ready to merge.

@Hywan Hywan added the enhancement label Nov 3, 2014

@Hywan Hywan self-assigned this Nov 3, 2014

@Hywan

This comment has been minimized.

Copy link
Member

commented Nov 3, 2014

State of this PR?

@Hywan

This comment has been minimized.

Copy link
Member

commented Nov 3, 2014

I have deleted a comment, my mistake.

@Metalaka Metalaka force-pushed the Metalaka:externalIterator branch from fea5970 to d754ffb Nov 4, 2014

@Metalaka

This comment has been minimized.

Copy link
Member Author

commented Nov 4, 2014

Should work for the following arguments:

$statement->setFetchingStyle(\PDO::FETCH_ORI_NEXT, 0);
$statement->setFetchingStyle(\PDO::FETCH_ORI_NEXT, 2);
$statement->setFetchingStyle(\PDO::FETCH_ORI_NEXT, 'end'); // but it's useless
$statement->setFetchingStyle(\PDO::FETCH_ORI_PRIOR, 0);
$statement->setFetchingStyle(\PDO::FETCH_ORI_PRIOR, 2);
$statement->setFetchingStyle(\PDO::FETCH_ORI_PRIOR, 'end');

Not yet tested

@Hywan

This comment has been minimized.

Copy link
Member

commented Nov 5, 2014

Next steps?

@Metalaka Metalaka force-pushed the Metalaka:externalIterator branch from d754ffb to f0fa492 Nov 12, 2014

@Hywan Hywan added the in progress label Jul 13, 2015

@Metalaka Metalaka force-pushed the Metalaka:externalIterator branch 6 times, most recently from 79248d6 to 6771dda Jul 17, 2015

@Hywan

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

ping?
I am really interested by this PR ❤️, so please, don't give up!

@Metalaka

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2015

Changes/State of the PR:

*Statement Implement IteratorAggregate, return a WrapperIterator.
The iterator is designed to be rewindable, but need to be tested...

Statement::setFetchingStyle($orientation, $offset) definition:
default value are to NEXT form FIRST.

$orientation values can be: // same value as \PDO::FETCH_ORI_*

  • DalStatement::FETCH_ORI_NEXT
  • DalStatement::FETCH_ORI_PRIOR

$offset values can be:

  • DalStatement::FETCH_OFFSET_FIRST
  • DalStatement::FETCH_OFFSET_LAST
  • Or an arbitrary offset (from the top of the result set, regardless of the orientation).

Pdo\Statement are initialized by default with cursor enabled.

I removed fetchFirst / Last / Next / Prior because it will cause unexpected behavior if used simultaneously with the iterator, thought ?
And I don't think tracking the internal result set offset is useful (with Iterator::key()) so I removed it from the PR(Metalaka@7557604)?

@Hywan

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

Sounds good for me. Some examples?

What's next?

@Metalaka

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2015

use Hoa\Database;

$dal = Database\Dal::getInstance(…);
$query = 'SELECT a, b FROM foo';

// One-query fetch all data (already exists)
foreach($dal->query($query)->fetchAll() as $row) {
    echo $row['a'], $row['b'], "\n";
}

// Lazy fetch all data
foreach($dal->query($query) as $row) {
    echo $row['a'], $row['b'], "\n";
}

// Fetch from last to first row of the result set.
$stmt = $dal->query($query);
$stmt->setFetchingStyle(
    Database\DalStatement::FETCH_ORI_PRIOR,
    Database\DalStatement::FETCH_OFFSET_LAST
);
foreach($stmt as $row) {
    echo $row['a'], $row['b'], "\n";
}

// Fetch from the fifth row of the result set to the first (offset 0).
$stmt = $dal->query($query);
$stmt->setFetchingStyle(
    Database\DalStatement::FETCH_ORI_PRIOR,
    4
);
foreach($stmt as $row) {
    echo $row['a'], $row['b'], "\n";
}

// Fetch the last row, ugly... , should be done in the query if possible.
$stmt = $dal->query($query);
$stmt->setFetchingStyle(
    null,
    Database\DalStatement::FETCH_OFFSET_LAST
);
$iterator = $stmt->getIterator();
$iterator->rewind();
$row = $iterator->current();

Maybe we should keep Statement::fetchFirst/Last for one row use case ?

Things like fetch the ten last row (first to last) isn't possible with this API.

What's next?

Find a compatible environment to test scrollable cursors.

* The statement instance.
*
* @var \Hoa\Database\IDal\WrapperStatement
*/
protected $statement = null;
protected $statement = null;

This comment has been minimized.

Copy link
@Hywan

Hywan Aug 5, 2015

Member

We don't align attributes with constants' equal sign 😉.

*/
public function fetchLast()
public function setFetchingStyle($orientation = self::FETCH_ORI_NEXT,
$offset = self::FETCH_OFFSET_FIRST)

This comment has been minimized.

Copy link
@Hywan

Hywan Aug 5, 2015

Member

CS:

public function setFetchingStyle(
    $orientation = self…,
    $offset      = self
) {

}
*/
public function fetchPrior();
public function setFetchingStyle($orientation = DalStatement::FETCH_ORI_NEXT,
$offset = DalStatement::FETCH_OFFSET_FIRST);

This comment has been minimized.

Copy link
@Hywan

Hywan Aug 5, 2015

Member

CS:

public function setFetchingStyle(
    $orientation = self…,
    $offset      = self
);
public function next()
{
$this->_row = $this->getStatement()->fetch(
\PDO::FETCH_ASSOC,

This comment has been minimized.

Copy link
@Hywan

Hywan Aug 5, 2015

Member

Are we sure? Could be great to still have array or object results nop?

*/
public function key()
{
return null;

This comment has been minimized.

Copy link
@Hywan

Hywan Aug 5, 2015

Member

Is an integer make sense? Not sure, because if we iterate backward, we don't really know what to do… So, ok for null, just wondering if you thought about this.

}
/**
* Rewind the Iterator to the first element.

This comment has been minimized.

Copy link
@Hywan

Hywan Aug 5, 2015

Member
- Iterator
+ iterator
\PDO::FETCH_ORI_ABS,
$this->_offset
);
}

This comment has been minimized.

Copy link
@Hywan

Hywan Aug 5, 2015

Member

So we can re-iterate over the same statement as expected?

This comment has been minimized.

Copy link
@Hywan

Hywan Aug 5, 2015

Member

Should we use closeCursor?

This comment has been minimized.

Copy link
@Metalaka

Metalaka Aug 5, 2015

Author Member

If the driver and the database support cursors, it should.
I don't know if closeCursor is required, maybe depending of databases ?

This comment has been minimized.

Copy link
@Hywan

Hywan Aug 5, 2015

Member

If I understood correctly, closeCursor will allow to reexecute the query, and this is not what we want. However, a new iterator should be re-generate, but this is part of the user to deal with it I guess.

*/
public function fetchLast()
public function setFetchingStyle($orientation = DalStatement::FETCH_ORI_NEXT,
$offset = DalStatement::FETCH_OFFSET_START)

This comment has been minimized.

Copy link
@Hywan

Hywan Aug 5, 2015

Member

CS.

@Hywan

This comment has been minimized.

Copy link
Member

commented Aug 5, 2015

So the final API of a statement is the following:

  • fetchAll,
  • fetchColumn,
  • getIterator.

Why keeping fetchColumn? Not sure it is useful here.

Next? Details first 😉. I am not very happy with the FETCH_* constant names. Their values are smart and correct, but I don't like the name:

  • FETCH_ORI_NEXT, what about FETCH_FORWARD?
  • FETCH_ORI_PRIOR, what about FETCH_BACKWARD?
  • FETCH_OFFSET_FIRST, very correct,
  • FETCH_OFFSET_LAST, very correct.

I understand you tried to mimic PDO naming, but… PDO naming is… It does not fit into Hoa's naming (be neutral, be neutral…).

Next next? Scrollable cursors are not supported by SQLite? According to https://bugs.php.net/39310, nop… I can ask on Twitter, thoughts?

@Hywan

This comment has been minimized.

Copy link
Member

commented Aug 5, 2015

@Metalaka Also, is it possible to support the Countable interface?
Can be addressed in another PR.

@Hywan

This comment has been minimized.

Copy link
Member

commented Sep 9, 2015

Now about the naming (because this is the last thing). We are very close to a consensus.

Offset

  • FROM_START,
  • FROM_END.

Direction

  • FORWARD,
  • BACKWARD.

Fetching style

  • AS_MAP,
  • AS_SET,
  • AS_OBJECT, we expect a stdClass to be generate (maybe AS_STRUCTURE would be better but not in the PHP culture, so let's keep AS_OBJECT),
  • 🔴 AS_CLASS, while I agree with the term, how do we specify the classname?
  • AS_REUSABLE_OBJECT, what about choosing the best behavior for the user between this one and AS_OBJECT? Reductio ad absurdum: If AS_OBJECT generates one new instance of a stdClass class per fetch (per row), we understand AS_REUSABLE_OBJECT will reuse the same instance. It can generate some troubles in case objects are stored when looping over the rows because of the references. So we cannot choose for the user because we cannot guess the expected behavior. So that's why this is a check mark,
  • 🔴 AS_DEBUG_MAP, I was thinking about AS_MULTIMAP (cf. multi-set), but like multi-set, multi-map is something very connotated in the literature and this is not exactly what we do here. Actually this is what we do but this is a specific result: This does not reflect our motivations when writing AS_MULTIMAP. AS_DEBUG_MAP seems good then. This is intended to be used when debugging only and it reflects our issues: We don't know exactly what data are fetched with the query in each row, using an object for the fetching is difficult, a set is not worth envisaging neither, so a map is pretty straightforward and adapted to the situation since it provides an immediate feedback, and this is exactly what we look for when debugging. However, is “as debug map” correct regarding the English language? I am not sure. This is the last point.

Conclusion

  1. We keep AS_CLASS if this is easy to specify the classname (maybe as the next argument of the fetching style method),
  2. AS_DEBUG_MAP can be wrong regarding the English language, I am not sure. Thoughts?

Thanks for your work!

@Hywan

This comment has been minimized.

Copy link
Member

commented Sep 9, 2015

Note the order: Offset, direction and fetching style. This is different from your proposal: Direction, offset and fetching style. I guess this is more intuitive in this order. “Where are we?” (offset), “Where to go?” (direction), “How do we go?” (fetching style). What do you think?

Edit: Would this order be “logically different” in another culture?

@Metalaka Metalaka force-pushed the Metalaka:externalIterator branch from 38fc08f to 17a4ecb Sep 9, 2015

@Metalaka

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2015

Setting the class name was here, but wasn't documented, anyway I just changed the signature.
I packed fetching options into an array since there are optional parameters (class name, constructor argument, object to use/reuse).

I added two arguments to setFetchStyle (see PDOStatement::setFetchMode, I don't really know if the second can be useful).
And, I reordered the arguments according to your logic.

@Hywan

This comment has been minimized.

Copy link
Member

commented Sep 10, 2015

As always, you did a great job.

@Hywan

This comment has been minimized.

Copy link
Member

commented Sep 10, 2015

Sorry to comment the patches instead of the diffs, once again…

@Metalaka Metalaka force-pushed the Metalaka:externalIterator branch from 17a4ecb to 3eb953a Sep 10, 2015

@Metalaka

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2015

Done.

@Hywan

This comment has been minimized.

Copy link
Member

commented Sep 10, 2015

I will review the PR soon, thanks!

@Hywan

This comment has been minimized.

Copy link
Member

commented Nov 9, 2015

Damn, I thought the PR was merged… I am putting this issue on the top of the column in the board!

@Hywan

This comment has been minimized.

Copy link
Member

commented Jan 26, 2016

I still thought it was merged! Damn…… Going to merge it as soon as possible.

@Hywan

This comment has been minimized.

Copy link
Member

commented Jan 26, 2016

@Metalaka I made a PR on your fork for the rebase: Metalaka#1. This really a good job! Now we have to write tests.

@hoaproject/hoackers Any volunteer to give it a try quickly?

@Metalaka Metalaka force-pushed the Metalaka:externalIterator branch from 3ef3f5a to 7813f26 Jan 28, 2016

Remove type hint of WrapperIterator constructor.
Since there is no base between supported underlying layer, the type must be `object`.
@Metalaka

This comment has been minimized.

Copy link
Member Author

commented Jan 28, 2016

Metalaka#2 merged.

@Hywan

This comment has been minimized.

Copy link
Member

commented Jan 29, 2016

So we are good?

@Hywan

This comment has been minimized.

Copy link
Member

commented Jan 29, 2016

Any idea how to provide a test suite for that?

@Bhoat Bhoat merged commit 91d1d6c into hoaproject:master Feb 8, 2016

@Metalaka Metalaka deleted the Metalaka:externalIterator branch Apr 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.