Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Akiban driver #188

Closed
wants to merge 91 commits into from

4 participants

@posulliv

An initial implementation of a driver for the Akiban Server. You will notice in the diff there are some features Akiban does not yet support in its latest release but are planned for future releases.

I wasn't sure if pull request was the best way to start a dialog to get this work accepted or whether the mailing list is more appropriate. Please let me know and I will do as suggested.

I am an engineer at Akiban and am very willing to maintain/improve this driver.

posulliv added some commits
@posulliv posulliv Add akibansrv to list of available drivers in DriverManager class. 16e0692
@posulliv posulliv Add simple implementation of Driver interface for Akiban Server. aba8430
@posulliv posulliv Simple implementation of Connection interface for Akiban Server. Not …
…all methods are implemented yet.
2fe3944
@posulliv posulliv Add simple exception wrapper for AkibanSrv exceptions. 2851f2c
@posulliv posulliv Add skeleton for AkibanSrvStatement class that implements Statement i…
…nterface. Nearly all methods still need to be implemented.
6ce2ae9
@posulliv posulliv Add skeleton for Akiban Server schema manager. c5e757f
@posulliv posulliv Skeleton for AkibanServerPlatform implementation. Most methods need t…
…o be implemented.
c750b4e
@posulliv posulliv Update akiban driver implementation to use correct names for platform…
… and schema manager implementations.
b5e6946
@posulliv posulliv Update function for getting next sequence value. 4ad4453
@posulliv posulliv Implement function for generating SQL to create sequences. fa487b6
@posulliv posulliv Akiban does not need to implement function for returning now expressi…
…on. This is due to Akiban having a NOW() function that is compatible with MySQL.
025fd93
@posulliv posulliv Akiban does not support regular expressions at this time. ad5cd9f
@posulliv posulliv Implement getLocateExpression function. 74ff267
@posulliv posulliv Implement getDateDiffExpression function. 9e5af6b
@posulliv posulliv Implement getDateAddDaysExpression function. 227f159
@posulliv posulliv Implement getDateSubDaysExpression function. 4f25bd2
@posulliv posulliv Implement getDateAddMonthExpression function. b2141df
@posulliv posulliv Implement getDateSubMonthExpression function. 6c97818
@posulliv posulliv Implement getListTablesSQL function. 2fcc4c9
@posulliv posulliv Implement getListViewsSQL function. db4e494
@posulliv posulliv Implement getCreateViewSQL function. ed04fa2
@posulliv posulliv Implement getListDatabasesSQL function. 87c0974
@posulliv posulliv Add cascade to drop schema command. This ensures a schema is dropped …
…even if it contains tables.
755a2d4
@posulliv posulliv Akiban does not support ALTER SEQUENCE yet so throw an unsupported ex…
…ception when that function is called.
85e5f1c
@posulliv posulliv Implement getSchemaNames function. 1e96fff
@posulliv posulliv Implement dropDatabase function. cc50f53
@posulliv posulliv Implement createDatabase function. 4c8020b
@posulliv posulliv Add parameter checking to dropDatabase function. d4166e9
@posulliv posulliv Use functions from abstract class for trigger related functionality. d4003ab
@posulliv posulliv Copy implementation of _getPortableViewDefintion from PostgreSQL impl…
…ementation.
47b70ae
@posulliv posulliv Use PostgreSQL implementation of _getPortableUserDefinition function. 010cb79
@posulliv posulliv Implement _getPortableTableDefinition table. 8b547df
@posulliv posulliv Implement _getPortableDatabaseDefinition function. 240afe1
@posulliv posulliv Implement _getPortableSequenceDefintion function. d05326b
@posulliv posulliv Use copy of PostgreSQL keywords for Akiban Server keywords for now. 8259c93
@posulliv posulliv Akiban Server will not implement _getPortableTableForeignKeyDefinitio…
…n for now.
660dee4
@posulliv posulliv Implement getSubstringExpression function. 52c882f
@posulliv posulliv Simple implementation of getListTableIndexesSQL function. Not sure if…
… this is sufficient.
8703c4c
@posulliv posulliv Implement initializeDoctrineTypeMappings function. This function has …
…the current types Akiban Server supports. As Akiban Server supports more types, this function will need to be updated.
8fde2b7
@posulliv posulliv Implemented the getTimeTypeDeclarationSQL function. 81123e0
@posulliv posulliv Implement getDateTimeTypeDeclaration function. Taken from MySQL imple…
…mentation for now.
6d58a7a
@posulliv posulliv Implement getBooleanTypeDeclarationSQL function. 63889e3
@posulliv posulliv Implement getListSequencesSQL function. 269f118
@posulliv posulliv Throw unsupported operation exception if an attempt is made to change…
… transaction isolation level for a session.
32961fa
@posulliv posulliv No need for akiban server to implement convertBooleans function. 682e207
@posulliv posulliv Implement getAlterTableSQL function. 1ed0377
@posulliv posulliv Akbian Server does not have date or time data types that support time…
…zones (same as mysql).
05fed4d
@posulliv posulliv Implement initial version of _getCreateTableSQL function. b40d8bb
@posulliv posulliv use default implementation of getDropForeignKeySQL for Akiban. 541fb29
@posulliv posulliv Use default implementation for getAdvancedForeignKeyOptionsSQL function. 4c7c94a
@posulliv posulliv Akiban Server does not need getTableWhereClause function. 1df2d9a
@posulliv posulliv Implemenet getListTableColumnsSQL function. 1736d59
@posulliv posulliv Implemnet getListTableConstraintsSQL function. 3277021
@posulliv posulliv Throw un-supported operation exception for getListTableForeignKeysSQL…
… function in Akiban for now.
556273f
@posulliv posulliv Unit tests can now run against Akiban with most of them obviously fai…
…ling right now since we have not implemented the majority of functionality.
a9513da
@posulliv posulliv Explicitly specify Akiban does not support savepoints. Flesh out a fe…
…w methods in Connection and Statement classes so queries can be sent to Akiban now. This allows basic unit tests to pass. Will likely modify this implementation in the near future.
e8d0f49
@posulliv posulliv Implement transaction related methods in AkibanSrvConnection class. 68dc30d
@posulliv posulliv Implement columnCount function in AkibanSrvStatement class. 44bbb0f
@posulliv posulliv Specify how Akiban does bit and/or operationsin platform class. 180666e
@posulliv posulliv Modification to unit test that takes Akiban behavior into account. 31b9f20
@posulliv posulliv Implement quote method in AkibanSrvConnection class. Implementation i…
…s taken from OCI8 driver.
f18ef46
@posulliv posulliv Implement remaining methods in AkibanSrvStatement class. 490f1f4
@posulliv posulliv Corrections to test around fetch with classes. 499ba2d
@posulliv posulliv Got more unit tests to pass. Failing tests are now related to serial …
…data type not being supported at the moment. AkibanSrvStatement class needs to be cleaned up a little bit.
ccc65fb
@posulliv posulliv Use identity columns for auto-increment for now until support for ser…
…ial is added.
578ce0a
@posulliv posulliv Akiban does not support the FOR UPDATE clause at the moment. a48684c
@posulliv posulliv Akiban does not support nested transactions at the moment so only beg…
…in a new transaction if not already in a transaction.
3d4668f
@posulliv posulliv If running unit tests against Akiban, do not run tests related to tem…
…porary tables since Akiban does not support those.
200ebd8
@posulliv posulliv Get all required information about a sequence from the information_sc…
…hema.
15e3204
@posulliv posulliv Re-enable failing test. f737f40
@posulliv posulliv Implement lastInsertId function in AkibanSrvConnection now that currv…
…al support has been added to Akiban.
366ad07
@posulliv posulliv Use serial instead of identity columns since akiban now supports it. 40fe291
@posulliv posulliv Clear up exception message. ef8ba00
@posulliv posulliv Make some formatting of comments more consistent. 50e8ce0
@posulliv posulliv Remove un-needed variable in AkibanSrvStatement class. af2cc93
@posulliv posulliv Simplify code in AkibanSrvStatement class. 0eef622
@posulliv posulliv Clean up code in AkibanSrvStatement class around fetching of results. 1aa8278
@posulliv posulliv Be more consistent with string quoting. f276c65
@posulliv posulliv Implement getUniqueConstraintDeclarationSQL function. 4cf1dee
@posulliv posulliv Akiban does not support foreign key constraints. Explicitly specify t…
…hat in platform implementation.
2044afb
@posulliv posulliv Implement some of the remaining functions in Platform and Schema mana…
…ger.
a0cea52
@posulliv posulliv Add test for AkibanSchemaManager. 8931cf1
@posulliv posulliv Add tests for AkibanServerPlatform class. 390d5a9
@posulliv posulliv Expand on comments so it is clear what akiban does/does not support. ab0a456
@posulliv posulliv Correct unit tests around akiban platform class. f7b0275
@posulliv posulliv Revert unit tests back to way they used to be. 316ee6d
@posulliv posulliv Changed a unit test by accident. 116640e
@posulliv posulliv Akiban does not support savepoints so make sure test reflects that. f1dab8e
@travisbot

This pull request passes (merged f1dab8e into e25c774).

@travisbot

This pull request passes (merged f1dab8e into e25c774).

...octrine/DBAL/Driver/AkibanSrv/AkibanSrvConnection.php
@@ -0,0 +1,179 @@
+<?php
+/*
+ * $Id$
@stof
stof added a note

Please remove this. We have some files which still have it, but it is a left-over from the SVN area

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...octrine/DBAL/Driver/AkibanSrv/AkibanSrvConnection.php
((15 lines not shown))
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * This software consists of voluntary contributions made by many individuals
+ * and is licensed under the MIT license. For more information, see
+ * <http://www.doctrine-project.org>.
+ */
+
+namespace Doctrine\DBAL\Driver\AkibanSrv;
+
+use Doctrine\DBAL\Platforms\AkibanServerPlatform;
+
+/**
+ * Akiban Server implementation of the Connection interface.
+ *
+ * @author Padraig O'Sullivan <osullivan.padraig@gmail.com>
+ * @since 2.3
@stof
stof added a note

this should be 2.4 as 2.3 is already in RC and so new features cannot be added in it anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...octrine/DBAL/Driver/AkibanSrv/AkibanSrvConnection.php
((22 lines not shown))
+namespace Doctrine\DBAL\Driver\AkibanSrv;
+
+use Doctrine\DBAL\Platforms\AkibanServerPlatform;
+
+/**
+ * Akiban Server implementation of the Connection interface.
+ *
+ * @author Padraig O'Sullivan <osullivan.padraig@gmail.com>
+ * @since 2.3
+ */
+class AkibanSrvConnection implements \Doctrine\DBAL\Driver\Connection
+{
+ /**
+ * @var resource
+ */
+ protected $_dbh;
@stof
stof added a note

please remove the underscore. This naming convention comes from the PHP 4 time when there was no visibility in PHP (everything was puoblic), and Doctrine does not use it anymore for new code (and older code using it is renamed little by little)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...octrine/DBAL/Driver/AkibanSrv/AkibanSrvConnection.php
((30 lines not shown))
+ * @since 2.3
+ */
+class AkibanSrvConnection implements \Doctrine\DBAL\Driver\Connection
+{
+ /**
+ * @var resource
+ */
+ protected $_dbh;
+
+ /**
+ * Create a Connection to an Akiban Server Database using
+ * the native PostgreSQL PHP driver.
+ *
+ * @param string $username
+ * @param string $password
+ * @param string $schema
@stof
stof added a note

this looks out of sync :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...octrine/DBAL/Driver/AkibanSrv/AkibanSrvConnection.php
((73 lines not shown))
+ {
+ $args = func_get_args();
+ $sql = $args[0];
+ $stmt = $this->prepare($sql);
+ $stmt->execute();
+ return $stmt;
+ }
+
+ /**
+ * Quote input value.
+ *
+ * @param mixed $input
+ * @param int $type PDO::PARAM*
+ * @return mixed
+ */
+ public function quote($value, $type=\PDO::PARAM_STR)
@stof
stof added a note

you should add spaces around the = in the signature (same for all methods)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...octrine/DBAL/Driver/AkibanSrv/AkibanSrvConnection.php
((117 lines not shown))
+
+ $sql = "SELECT CURRENT VALUE FOR " . $name;
+ $stmt = $this->query($sql);
+ $result = $stmt->fetchColumn(0);
+
+ if ($result === false) {
+ throw new AkibanSrvException("lastInsertId failed due to current value not being returned for a sequence.");
+ }
+ return (int) $result[0];
+ }
+
+ /**
+ * Start a transactiom
+ *
+ * @return bool
+ */
@stof
stof added a note

please use {@inheritDoc} for all methods already documented in the parent class or the interface (eventually adding something if there is a need for an explanation about the way it is implemented). It makes it easier to maintain than duplicating the phpdoc (we don't need to fix typos in 10 different places)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...octrine/DBAL/Driver/AkibanSrv/AkibanSrvConnection.php
((122 lines not shown))
+ if ($result === false) {
+ throw new AkibanSrvException("lastInsertId failed due to current value not being returned for a sequence.");
+ }
+ return (int) $result[0];
+ }
+
+ /**
+ * Start a transactiom
+ *
+ * @return bool
+ */
+ public function beginTransaction()
+ {
+ $trxStatus = pg_transaction_status($this->_dbh);
+ if (! $trxStatus == PGSQL_TRANSACTION_INTRANS) {
+ if (! pg_query($this->_dbh, "BEGIN")) {
@stof
stof added a note

you should use && here instead of nesting 2 if

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Doctrine/DBAL/Driver/AkibanSrv/AkibanSrvStatement.php
((59 lines not shown))
+
+ /**
+ * An array of the parameters for this statement.
+ */
+ private $_parameters = array();
+
+ /**
+ * The fetch mode for this statement.
+ */
+ private $_defaultFetchMode = PDO::FETCH_BOTH;
+
+ private $_className;
+ private $_ctorArgs;
+
+ private static $fetchModeMap =
+ array(
@stof
stof added a note

array( should be on the previous line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Doctrine/DBAL/Driver/AkibanSrv/AkibanSrvStatement.php
((56 lines not shown))
+ * @var resource
+ */
+ private $_conn;
+
+ /**
+ * An array of the parameters for this statement.
+ */
+ private $_parameters = array();
+
+ /**
+ * The fetch mode for this statement.
+ */
+ private $_defaultFetchMode = PDO::FETCH_BOTH;
+
+ private $_className;
+ private $_ctorArgs;
@stof
stof added a note

Please don't abbreviate the name. It will be more readable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Doctrine/DBAL/Driver/AkibanSrv/AkibanSrvStatement.php
((188 lines not shown))
+ $this->_results = pg_query($this->_dbh, $this->_statement);
+ } else if (empty($this->_parameters) && ! is_null($params)) {
+ $this->_results = pg_query_params($this->_dbh, $this->_statement, $params);
+ } else {
+ $this->_results = pg_query_params($this->_dbh, $this->_statement, $this->_parameters);
+ }
+ return $this->_results;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setFetchMode($fetchMode, $arg2 = null, $arg3 = null)
+ {
+ $this->_defaultFetchMode = $fetchMode;
+ if ($fetchMode == PDO::FETCH_OBJ || $fetchMode == PDO::FETCH_CLASS) {
@stof
stof added a note

Please use a strict comparison whenever possible. And You should use && instead of a nested if

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Doctrine/DBAL/Driver/AkibanSrv/AkibanSrvStatement.php
((203 lines not shown))
+ if ($fetchMode == PDO::FETCH_OBJ || $fetchMode == PDO::FETCH_CLASS) {
+ if (func_num_args() >= 2) {
+ $args = func_get_args();
+ $this->_className = $args[1];
+ $this->_ctorArgs = (isset($args[2])) ? $args[2] : array();
+ }
+ }
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getIterator()
+ {
+ $data = $this->fetchAll();
+ return new \ArrayIterator($data);
@stof
stof added a note

you don't need $data IMO. It could be inlined

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Doctrine/DBAL/Driver/AkibanSrv/AkibanSrvStatement.php
((221 lines not shown))
+ /**
+ * {@inheritdoc}
+ */
+ public function fetch($fetchMode = null, $rowPos = NULL)
+ {
+ $fetchMode = $fetchMode ? : $this->_defaultFetchMode;
+
+ if (! isset(self::$fetchModeMap[$fetchMode])) {
+ throw new \InvalidArgumentException("Invalid fetch style: " . $fetchMode);
+ }
+
+ if ($fetchMode == PDO::FETCH_OBJ || $fetchMode == PDO::FETCH_CLASS) {
+ if ($this->_results && $this->_className) {
+ if (empty($this->_ctorArgs)) {
+ return pg_fetch_object($this->_results, $rowPos, $this->_className);
+ } else {
@stof
stof added a note

no need to use else as the if returns:

<?php

if ($this->_results && $this->_className) {
    return;
}

// some code executed only when the condition is false
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Doctrine/DBAL/Driver/AkibanSrv/AkibanSrvStatement.php
((249 lines not shown))
+ /**
+ * {@inheritdoc}
+ */
+ public function fetchAll($fetchMode = null)
+ {
+ $fetchMode = $fetchMode ? : $this->_defaultFetchMode;
+
+ if (! isset(self::$fetchModeMap[$fetchMode])) {
+ throw new \InvalidArgumentException("Invalid fetch mode: " . $fetchMode);
+ }
+
+ $result = array();
+
+ switch ($fetchMode) {
+ case PDO::FETCH_OBJ:
+ case PDO::FETCH_CLASS:
@stof
stof added a note

missing one indentation level here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/DBAL/Driver/AkibanSrv/Driver.php
((41 lines not shown))
+ if (isset($params['dbname'])) {
+ $connString .= 'dbname=' . $params['dbname'] . ' ';
+ }
+ if (isset($username)) {
+ $connString .= 'user=' . $username . ' ';
+ }
+ if (isset($password)) {
+ $connString .= 'user=' . $username . ' ';
+ }
+
+ return $connString;
+ }
+
+ public function getDatabasePlatform()
+ {
+ return new \Doctrine\DBAL\Platforms\AkibanServerPlatform();
@stof
stof added a note

you should add use statements for these classes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/DBAL/Platforms/AkibanServerPlatform.php
((46 lines not shown))
+ {
+ if ($len === null) {
+ return "SUBSTR(" . $value . ", " . $from . ")";
+ } else {
+ return "SUBSTR(" . $value . ", " . $from . ", " . $len . ")";
+ }
+ }
+
+ /**
+ * returns the position of the first occurrence of substring $substr in string $str
+ *
+ * @param string $substr literal string to find
+ * @param string $str literal string
+ * @param int $pos position to start at, beginning of string by default
+ * @return integer
+ */
@stof
stof added a note

Please use inheritDoc whenever possible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/DBAL/Platforms/AkibanServerPlatform.php
((34 lines not shown))
+ /**
+ * Returns part of a string.
+ *
+ * Note: Not SQL92, but common functionality.
+ *
+ * @param string $value the target $value the string or the string column.
+ * @param int $from extract from this character.
+ * @param int $len extract this amount of characters.
+ * @return string sql that extracts part of a string.
+ * @override
+ */
+ public function getSubstringExpression($value, $from, $len = null)
+ {
+ if ($len === null) {
+ return "SUBSTR(" . $value . ", " . $from . ")";
+ } else {
@stof
stof added a note

please don't use else when it is not needed (see the other platforms as I fixed their coding standards 2 weeks ago)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/DBAL/Platforms/AkibanServerPlatform.php
((242 lines not shown))
+ public function getListTableConstraintsSQL($table)
+ {
+ // TODO - do we only want unique and primary key indexes here?
+ return "SELECT index_name " .
+ "FROM information_schema.indexes " .
+ "WHERE schema_name != 'information_schema' AND " .
+ "table_name = '" . $table . "'";
+ }
+
+ /**
+ * @param string $table
+ * @return string
+ */
+ public function getListTableIndexesSQL($table, $currentDatabase = null)
+ {
+ if (! is_null($currentDatabase)) {
@stof
stof added a note

please use null !== to compare the value with null, to be consistent with our coing standards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/DBAL/Schema/AkibanServerSchemaManager.php
((94 lines not shown))
+ }
+
+ protected function _getPortableTableDefinition($table)
+ {
+ return $table['table_name'];
+ }
+
+ /**
+ * @param array $tableIndexes
+ * @param string $tableName
+ * @return array
+ */
+ protected function _getPortableTableIndexesList($tableIndexes, $tableName=null)
+ {
+ $indexBuffer = array();
+ if (! empty($tableIndexes)) {
@stof
stof added a note

is it really needed ? Looping over an empty array is possible without any issue (I simply haven't checked the code to be sire that the array is always passed)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Doctrine/DBAL/Driver/AkibanSrv/AkibanSrvStatement.php
((275 lines not shown))
+ case PDO::FETCH_NUM:
+ $result = $this->fetchRows($fetchMode);
+ break;
+ case PDO::FETCH_COLUMN:
+ for ($i = 0; $i < pg_num_rows($this->_results); $i++) {
+ for ($col = 0; $col < $this->columnCount(); $col++) {
+ $result[] = $this->fetchColumn($col, $i);
+ }
+ }
+ break;
+ default:
+ $result = pg_fetch_all($this->_results);
+ break;
+ }
+
+ return empty($result) ? false : $result;
@stof
stof added a note

this does not respect the interface. It mandates to return an array in fetchAll. Why replacing an empty array by false ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/Doctrine/Tests/DBAL/Functional/DataAccessTest.php
@@ -338,7 +338,7 @@ public function testQuoteSQLInjection()
$sql = "SELECT * FROM fetch_table WHERE test_string = " . $this->_conn->quote("bar' OR '1'='1");
$rows = $this->_conn->fetchAll($sql);
- $this->assertEquals(0, count($rows), "no result should be returned, otherwise SQL injection is possible");
+ $this->assertEquals(0, ($rows == false) ? 0 : count($rows), "no result should be returned, otherwise SQL injection is possible");
@stof
stof added a note

please revert it. It should not be necessary if fetchAll was implemented as expected by the interface

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...DBAL/Functional/Schema/AkibanSrvSchemaManagerTest.php
((14 lines not shown))
+ {
+ parent::tearDown();
+
+ if (!$this->_conn) {
+ return;
+ }
+
+ $this->_conn->getConfiguration()->setFilterSchemaAssetsExpression(null);
+ }
+
+ public function testGetSchemaNames()
+ {
+ $names = $this->_sm->getSchemaNames();
+
+ $this->assertInternalType('array', $names);
+ $this->assertTrue(count($names) > 0);
@stof
stof added a note

you should use assertGreaterThan so that the message is better in case of failure (currently, it would say Failed asserting false is true)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/Doctrine/Tests/DBAL/Functional/WriteTest.php
@@ -122,7 +122,8 @@ public function testDelete()
$this->assertEquals(1, count($this->_conn->fetchAll('SELECT * FROM write_table')));
$this->assertEquals(1, $this->_conn->delete('write_table', array('test_int' => 1)));
- $this->assertEquals(0, count($this->_conn->fetchAll('SELECT * FROM write_table')));
+ $result = $this->_conn->fetchAll('SELECT * FROM write_table');
+ $this->assertEquals(0, ($result == false) ? 0 : count($result));
@stof
stof added a note

same here than above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof

Hi,

If you are willing to continue maintaining this driver, there is definitely a good chance to have it merged once it is ready (my comments are about letting you achieve this goal).
And a pull request is the best way to do it as you already have some code for it. It is the best way to let us review it.

@beberlei
Owner

Hi,

thank you for this contribution. We will consider including it as a third-party maintained driver, but we have to put the full maintenance and testing of it into your hands. That means we mention the driver on the documentation and link to you for support. Tickets can go through our jira and we can make you a developer on Jira.

@posulliv

First off, a huge thank you for the review and very helpful comments. I will go through those today and apply your suggestions.

One question I did have: is there an official coding standards doc somewhere I can use for reference?

As for maintenance, I am very willing to maintain, test, support, and continue to improve this driver.

@stof

The official coding standards are mostly the PSR-2 standard. And then on top of that, @guilhermeblanco enforces some rules coming from the Object Calisthenics (but he checks the ORM more than DBAL IIRC)

@travisbot

This pull request passes (merged abb7b55 into e25c774).

@travisbot

This pull request passes (merged 6003fae into e25c774).

@posulliv

Ok, this branch is ready for another review.

I've updated the code to adhere to the coding standards and applied the suggestions you specified.

...Doctrine/DBAL/Driver/AkibanSrv/AkibanSrvStatement.php
((41 lines not shown))
+ /**
+ * SQL statement to execute
+ *
+ * @var string
+ */
+ private $statement;
+
+ /**
+ * query results
+ */
+ private $results;
+
+ /**
+ * Akiban Server connection object.
+ *
+ * @var resource
@stof
stof added a note

wrong phpdoc. This is not a resource

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Doctrine/DBAL/Driver/AkibanSrv/AkibanSrvStatement.php
((43 lines not shown))
+ *
+ * @var string
+ */
+ private $statement;
+
+ /**
+ * query results
+ */
+ private $results;
+
+ /**
+ * Akiban Server connection object.
+ *
+ * @var resource
+ */
+ private $connection;
@stof
stof added a note

Btw, I asked to avoid to avoid abbreviating constructorArguments to cstorArgs to keep it readable, but conn is used in the whole Doctrine codebase each time a connection is stored in a variable (both DBAL and ORM) so I would vote for keeping it in this cases (same for params, which may even be used in method names IIRC)

Makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff
...Doctrine/DBAL/Driver/AkibanSrv/AkibanSrvStatement.php
((98 lines not shown))
+ * this method converts all positional parameters into numbered parameters.
+ */
+ private function convertPositionalToNumberedParameters($statement)
+ {
+ $count = 1;
+ $inLiteral = false;
+ $stmtLen = strlen($statement);
+ for ($i = 0; $i < $stmtLen; $i++) {
+ if ($statement[$i] === '?' && ! $inLiteral) {
+ $param = "$" . $count;
+ $len = strlen($param);
+ $statement = substr_replace($statement, $param, $i, 1);
+ $i += $len - 1;
+ $stmtLen = strlen($statement);
+ ++$count;
+ } else if ($statement[$i] === "'" || $statement[$i] === '"') {
@stof
stof added a note

This breaks if you have the string "Foo'Bar" in the query, as the single quote in the literal will trigger a switch whereas only the double quote should end the literal. You need some more complex logic

Indeed, right you are. Another benefit of modifying our implementation to using the PostgreSQL PDO driver would be we could get rid of the need for this method entirely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Doctrine/DBAL/Driver/AkibanSrv/AkibanSrvStatement.php
((177 lines not shown))
+
+ /**
+ * {@inheritDoc}
+ */
+ public function errorInfo()
+ {
+ return pg_last_error($this->connectionHandle);
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function execute($params = null)
+ {
+ $noExistingParameters = empty($this->parameters) ? true : false;
+ $parametersPassed = null !== $params ? true : false;
@stof
stof added a note

no need for these ternary operators: empty returns a boolean, and so does the comparison.

thus, you can get rid of these variable entirely:

<?php
if ( ! empty($this->parameters)) {
    return $this->executeParameterizedQuery($this->connectionHandle, $this->statement, $this->parameters);
}

if (null !== $params) {
    return $this->executeParameterizedQuery($this->connectionHandle, $this->statement, $params);
}

return $this->executeQuery($this->connectionHandle, $this->statement);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Doctrine/DBAL/Driver/AkibanSrv/AkibanSrvStatement.php
((189 lines not shown))
+ public function execute($params = null)
+ {
+ $noExistingParameters = empty($this->parameters) ? true : false;
+ $parametersPassed = null !== $params ? true : false;
+ if ($noExistingParameters && ! $parametersPassed) {
+ return $this->executeQuery($this->connectionHandle, $this->statement);
+ }
+ if ($noExistingParameters && $parametersPassed) {
+ return $this->executeParameterizedQuery($this->connectionHandle, $this->statement, $params);
+ }
+ return $this->executeParameterizedQuery($this->connectionHandle, $this->statement, $this->parameters);
+ }
+
+ private function executeParameterizedQuery($connectionHandle, $statement, $parameters)
+ {
+ $this->results = pg_query_params($connectionHandle, $statement, $parameters);
@stof
stof added a note

you could simplify the signature by accessing $this->connectionHandle here directly instead of passing it as an argument from the calling code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Doctrine/DBAL/Driver/AkibanSrv/AkibanSrvStatement.php
((227 lines not shown))
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getIterator()
+ {
+ return new \ArrayIterator($this->fetchAll());
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function fetch($fetchMode = null, $rowPos = null)
+ {
+ $fetchMode = $fetchMode ? : $this->defaultFetchMode;
@stof
stof added a note

the space should be removed when using the short version of the ternary operator: ?:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff
...Doctrine/DBAL/Driver/AkibanSrv/AkibanSrvStatement.php
((290 lines not shown))
+ $result[] = $this->fetchColumn($col, $i);
+ }
+ }
+ break;
+ default:
+ $result = pg_fetch_all($this->results);
+ break;
+ }
+
+ /*
+ * The native PostgreSQL client returns false if no
+ * rows are returned. Since the fetchAll interface
+ * specifies an array must be returned, make sure
+ * an empty array is returned if in fact the PostgreSQL
+ * client returned false.
+ */
@stof
stof added a note

Reading the php.net documentation, it looks like false is meant to be returned in case of an error too, meaning it does not allow any error reporting if we consider that a query returning 0 results may be a valid use case (which it is).

So now, I'm wondering why you guys choosed to use the pg_* functions to connect to your system, given their poor API. Would it work to use the PDO PosgreSQL driver instead ?

It is possible to use the PDO driver but there is PostgreSQL functionality that the PDO driver uses which Akiban does not support.

For example, the DEALLOCATE statement is used to free prepared statements (relevant source).

Another example of something we don't support is server-side cursors. These are used in a few places by the PDO driver.

That said, it might still be beneficial to see how far I could get using the PDO driver. What I might do is get this branch cleaned up with all your feedback applied and once that is done, try creating another implementation that uses PDO instead of pg_* and see how far I can get.

@stof
stof added a note

yeah, this sounds like a plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/DBAL/Driver/AkibanSrv/Driver.php
((36 lines not shown))
+ public function connect(array $params, $username = null, $password = null, array $driverOptions = array())
+ {
+ return new AkibanSrvConnection(
+ $this->constructConnectionString($params, $username, $password)
+ );
+ }
+
+ /**
+ * Constructs the Akiban Server connection string.
+ *
+ * @return string The connection string.
+ */
+ private function constructConnectionString(array $params, $username, $password)
+ {
+ $connString = '';
+ if (isset($params['host']) && $params['host'] !== '') {
@stof
stof added a note

I would simply use ``if ( ! empty($params['host'])) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/DBAL/Driver/AkibanSrv/Driver.php
((66 lines not shown))
+
+ return $connString;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function getDatabasePlatform()
+ {
+ return new AkibanServerPlatform();
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function getSchemaManager(\Doctrine\DBAL\Connection $conn)
@stof
stof added a note

please add a use statement for this class please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/DBAL/Platforms/AkibanServerPlatform.php
((175 lines not shown))
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function supportsForeignKeyConstraints()
+ {
+ return false;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function supportsForeignKeyOnUpdate()
+ {
+ return ($this->supportsForeignKeyConstraints() && true);
@stof
stof added a note

I would remove the && true here. It is useless (even if I recognize it is a copy-paste from some other code) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff
lib/Doctrine/DBAL/Platforms/AkibanServerPlatform.php
((235 lines not shown))
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getDropViewSQL($name)
+ {
+ return "DROP VIEW " . $name;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getListTableConstraintsSQL($table)
+ {
+ // TODO - do we only want unique and primary key indexes here?
@stof
stof added a note

@beberlei what is expected exactly here ?

Yes, I was a little unsure of what to return here.

The different driver implementations seem to be inconsistent here. The MySQL and SQL Server drivers returns the list of indexes on a table while the Oracle driver returns all constraints on the table including check and foreign key constraints. The PostgreSQL driver seems to only return unique and primary key indexes on the table

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on the diff
lib/Doctrine/DBAL/Schema/AkibanServerSchemaManager.php
((41 lines not shown))
+
+ /**
+ * {@inheritdoc}
+ */
+ public function dropDatabase($database = null)
+ {
+ if (null === $database) {
+ $database = $this->_conn->getDatabase();
+ }
+
+ $params = $this->_conn->getParams();
+ $params["dbname"] = "information_schema";
+ $tmpPlatform = $this->_platform;
+ $tmpConn = $this->_conn;
+
+ $this->_conn = \Doctrine\DBAL\DriverManager::getConnection($params);
@stof
stof added a note

please add a use statement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/DBAL/Schema/AkibanServerSchemaManager.php
((193 lines not shown))
+ case 'double precision':
+ case 'real':
+ case 'decimal':
+ case 'numeric':
+ // TODO
+ break;
+ case 'year':
+ $length = null;
+ break;
+ }
+
+ if ($tableColumn['default'] && preg_match("('([^']+)'::)", $tableColumn['default'], $match)) {
+ $tableColumn['default'] = $match[1];
+ }
+
+ $tableColumn['nullable'] == 'NO' ? $notnull = true : $notnull = false;
@stof
stof added a note

please write it as $notNull = $tableColumn['nullable'] === 'NO'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/DBAL/Schema/AkibanServerSchemaManager.php
((194 lines not shown))
+ case 'real':
+ case 'decimal':
+ case 'numeric':
+ // TODO
+ break;
+ case 'year':
+ $length = null;
+ break;
+ }
+
+ if ($tableColumn['default'] && preg_match("('([^']+)'::)", $tableColumn['default'], $match)) {
+ $tableColumn['default'] = $match[1];
+ }
+
+ $tableColumn['nullable'] == 'NO' ? $notnull = true : $notnull = false;
+ $tableColumn['index_type'] == 'PRIMARY' ? $primaryKey = true : $primaryKey = false;
@stof
stof added a note

same for this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/DBAL/Schema/AkibanServerSchemaManager.php
((209 lines not shown))
+ $tableColumn['index_type'] == 'PRIMARY' ? $primaryKey = true : $primaryKey = false;
+
+ $options = array(
+ 'length' => $length,
+ 'notnull' => $notnull,
+ 'default' => $tableColumn['default'],
+ 'primary' => $primaryKey,
+ 'precision' => $precision,
+ 'scale' => $scale,
+ 'fixed' => $fixed,
+ 'unsigned' => false,
+ 'autoincrement' => $autoincrement,
+ 'comment' => NULL,
+ );
+
+ return new Column($tableColumn['column_name'], \Doctrine\DBAL\Types\Type::getType($type), $options);
@stof
stof added a note

please add a use statement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ctrine/Tests/DBAL/Platforms/AkibanSrvPlatformTest.php
@@ -0,0 +1,182 @@
+<?php
+
+namespace Doctrine\Tests\DBAL\Platforms;
+
+use Doctrine\DBAL\Platforms\AkibanServerPlatform;
+use Doctrine\DBAL\Types\Type;
+
+require_once __DIR__ . '/../../TestInit.php';
@stof
stof added a note

I don't think this line is needed anymore. @beberlei can you confirm ?

The tests run fine without this line so I can remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@travisbot

This pull request passes (merged af23244 into e25c774).

@posulliv

Thanks for your review comments. I've applied the majority of them.

I'm going to create an implementation that uses PDO and see if I can get that as far. If I can get that functional this weekend, I'll close this pull request and open a new one with that implementation.

@stof

Note that a PDO-based implementation will normally require less code as the statement and connection classes are based on the PDO interfaces :)

@posulliv

Closing this pull request as it has been superseded by pull request 191.

@posulliv posulliv closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 11, 2012
  1. @posulliv
  2. @posulliv
  3. @posulliv

    Simple implementation of Connection interface for Akiban Server. Not …

    posulliv authored
    …all methods are implemented yet.
  4. @posulliv
  5. @posulliv

    Add skeleton for AkibanSrvStatement class that implements Statement i…

    posulliv authored
    …nterface. Nearly all methods still need to be implemented.
  6. @posulliv
  7. @posulliv
  8. @posulliv

    Update akiban driver implementation to use correct names for platform…

    posulliv authored
    … and schema manager implementations.
  9. @posulliv
  10. @posulliv
  11. @posulliv

    Akiban does not need to implement function for returning now expressi…

    posulliv authored
    …on. This is due to Akiban having a NOW() function that is compatible with MySQL.
  12. @posulliv
  13. @posulliv
  14. @posulliv
  15. @posulliv
  16. @posulliv
  17. @posulliv
  18. @posulliv
  19. @posulliv
  20. @posulliv
  21. @posulliv
  22. @posulliv
  23. @posulliv

    Add cascade to drop schema command. This ensures a schema is dropped …

    posulliv authored
    …even if it contains tables.
  24. @posulliv

    Akiban does not support ALTER SEQUENCE yet so throw an unsupported ex…

    posulliv authored
    …ception when that function is called.
Commits on Aug 13, 2012
  1. @posulliv
  2. @posulliv
  3. @posulliv
  4. @posulliv
  5. @posulliv
  6. @posulliv
  7. @posulliv
  8. @posulliv
  9. @posulliv
  10. @posulliv
  11. @posulliv
  12. @posulliv
  13. @posulliv
  14. @posulliv
  15. @posulliv

    Implement initializeDoctrineTypeMappings function. This function has …

    posulliv authored
    …the current types Akiban Server supports. As Akiban Server supports more types, this function will need to be updated.
  16. @posulliv
  17. @posulliv
  18. @posulliv
  19. @posulliv
  20. @posulliv

    Throw unsupported operation exception if an attempt is made to change…

    posulliv authored
    … transaction isolation level for a session.
  21. @posulliv
  22. @posulliv
  23. @posulliv
  24. @posulliv
  25. @posulliv
  26. @posulliv
  27. @posulliv
  28. @posulliv
  29. @posulliv
  30. @posulliv

    Throw un-supported operation exception for getListTableForeignKeysSQL…

    posulliv authored
    … function in Akiban for now.
  31. @posulliv

    Unit tests can now run against Akiban with most of them obviously fai…

    posulliv authored
    …ling right now since we have not implemented the majority of functionality.
Commits on Aug 14, 2012
  1. @posulliv

    Explicitly specify Akiban does not support savepoints. Flesh out a fe…

    posulliv authored
    …w methods in Connection and Statement classes so queries can be sent to Akiban now. This allows basic unit tests to pass. Will likely modify this implementation in the near future.
  2. @posulliv
  3. @posulliv
  4. @posulliv
  5. @posulliv
  6. @posulliv
  7. @posulliv
  8. @posulliv
  9. @posulliv

    Got more unit tests to pass. Failing tests are now related to serial …

    posulliv authored
    …data type not being supported at the moment. AkibanSrvStatement class needs to be cleaned up a little bit.
  10. @posulliv
  11. @posulliv
  12. @posulliv

    Akiban does not support nested transactions at the moment so only beg…

    posulliv authored
    …in a new transaction if not already in a transaction.
Commits on Aug 15, 2012
  1. @posulliv

    If running unit tests against Akiban, do not run tests related to tem…

    posulliv authored
    …porary tables since Akiban does not support those.
  2. @posulliv
  3. @posulliv

    Re-enable failing test.

    posulliv authored
Commits on Aug 17, 2012
  1. @posulliv

    Implement lastInsertId function in AkibanSrvConnection now that currv…

    posulliv authored
    …al support has been added to Akiban.
Commits on Aug 22, 2012
  1. @posulliv
Commits on Aug 23, 2012
  1. @posulliv

    Clear up exception message.

    posulliv authored
  2. @posulliv
  3. @posulliv
  4. @posulliv
  5. @posulliv
  6. @posulliv
  7. @posulliv
  8. @posulliv

    Akiban does not support foreign key constraints. Explicitly specify t…

    posulliv authored
    …hat in platform implementation.
  9. @posulliv
  10. @posulliv
  11. @posulliv
Commits on Aug 24, 2012
  1. @posulliv
  2. @posulliv
  3. @posulliv
  4. @posulliv
  5. @posulliv
  6. @posulliv

    Apply comments from pull request 188. Also clean up code to adhere to…

    posulliv authored
    … doctrine coding standards.
  7. @posulliv
Commits on Aug 25, 2012
  1. @posulliv
Something went wrong with that request. Please try again.