[DDC-1845] QuoteStrategy #372

Merged
merged 37 commits into from Jun 25, 2012

Conversation

Projects
None yet
6 participants
Owner

FabioBatSilva commented Jun 12, 2012

QuoteStrategy

http://www.doctrine-project.org/jira/browse/DDC-1845

This patch fix some quote problems using a default quote strategy and allows users find solutions themselves for weird quote cases.

This DBAL PR shoud be merged to fix sqlite tests in : doctrine/dbal#158

There is a lote of new method calls, the performance tests in sqlite are the following :

My branch :

. testSimpleQueryScalarHydrationPerformance10000Rows - 0.37292098999023 seconds
. testSimpleQueryArrayHydrationPerformance10000Rows - 0.50411009788513 seconds
. testMixedQueryFetchJoinArrayHydrationPerformance10000Rows - 1.1323919296265 seconds
. testSimpleQueryPartialObjectHydrationPerformance10000Rows - 1.0243051052094 seconds
. testSimpleQueryFullObjectHydrationPerformance10000Rows - 4.1673181056976 seconds
. testMixedQueryFetchJoinPartialObjectHydrationPerformance2000Rows - 0.40403699874878 seconds
. testMixedQueryFetchJoinFullObjectHydrationPerformance2000Rows - 0.873291015625 seconds
. 99 CompanyContract: 0.023395
. 99 CompanyContract: 0.019528
. Memory usage before: 111715.53125 KB
. Memory usage after: 110282.875 KB
. Inserted 10000 objects in 5.5240259170532 seconds
. 100 CmsArticle findAll(): 0.018410
. 100 CmsArticle findAll(): 0.014157
. 100 CmsArticle find(): 0.043304
. 100 CmsArticle find(): 0.041451
. 100 CmsGroup: 0.009315
. 100 CmsGroup: 0.009239
. 100 CmsUser: 0.024051
. 100 CmsUser: 0.023655
. Compute ChangeSet 100 objects in 0.040482997894287 seconds

. Time: 19 seconds, Memory: 261.50Mb

Doctrine Master :

. testSimpleQueryScalarHydrationPerformance10000Rows - 0.37668490409851 seconds
. testSimpleQueryArrayHydrationPerformance10000Rows - 0.5009880065918 seconds
. testMixedQueryFetchJoinArrayHydrationPerformance10000Rows - 1.131824016571 seconds
. testSimpleQueryPartialObjectHydrationPerformance10000Rows - 1.037791967392 seconds
. testSimpleQueryFullObjectHydrationPerformance10000Rows - 4.1992859840393 seconds
. testMixedQueryFetchJoinPartialObjectHydrationPerformance2000Rows - 0.41158103942871 seconds
. testMixedQueryFetchJoinFullObjectHydrationPerformance2000Rows - 0.86575293540955 seconds
. 99 CompanyContract: 0.023206
. 99 CompanyContract: 0.020555
. Inserted 10000 objects in 5.4995818138123 seconds
. 100 CmsArticle findAll(): 0.018785
. 100 CmsArticle findAll(): 0.014160
. 100 CmsArticle find(): 0.042067
. 100 CmsArticle find(): 0.041076
. 100 CmsGroup: 0.009478
. 100 CmsGroup: 0.010591
. 100 CmsUser: 0.023727
. 100 CmsUser: 0.023073
. Compute ChangeSet 100 objects in 0.040009021759033 seconds

. Time: 19 seconds, Memory: 260.50Mb

This pull request fails (merged c3eaf521 into 7b75849).

lib/Doctrine/ORM/Configuration.php
+ public function getQuoteStrategy()
+ {
+ if ( ! isset($this->_attributes['quoteStrategy'])) {
+ $this->_attributes['quoteStrategy'] = new \Doctrine\ORM\Mapping\DefaultQuoteStrategy();
@stof

stof Jun 12, 2012

Member

you should add a use statement

@Ocramius

Ocramius Jun 12, 2012

Owner

@FabioBatSilva you forgot this one ;)

lib/Doctrine/ORM/Mapping/DefaultQuoteStrategy.php
+ {
+ return isset($class->table['quoted'])
+ ? $platform->quoteIdentifier($class->table['name'])
+ : $class->table['name'];
@stof

stof Jun 12, 2012

Member

too much indentation here

Owner

FabioBatSilva commented Jun 12, 2012

DONE !!!

Thanks @stof

This pull request fails (merged 80ef4c1e into 7b75849).

Member

stof commented Jun 12, 2012

please fix the code for PostgreSQL and SQLite

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
if (empty($joinColumn['referencedColumnName'])) {
$joinColumn['referencedColumnName'] = $this->namingStrategy->referenceColumnName();
}
+
+ if ($joinColumn['name'][0] == '`') {
+ $joinColumn['name'] = trim($joinColumn['name'], '`');
@Ocramius

Ocramius Jun 12, 2012

Owner

This is obviously for MySQL: are there other engines using different characters for quoting identifiers?

@FabioBatSilva

FabioBatSilva Jun 12, 2012

Owner

hi @Ocramius

This is just a doctrine quote token, not used in SQL level

Owner

FabioBatSilva commented Jun 12, 2012

Hi @stof

already is fixed.

PostgreSQL fails on cache, even in doctrine/master. not related with my patch.
To fix SQLite doctrine/dbal#158 shoud be merged.

This pull request fails (merged 3d4404dc into 7b75849).

This pull request fails (merged 0ce06e2b into 27b4f58).

Owner

beberlei commented Jun 18, 2012

+1 from me, @guilhermeblanco whats your take?

- $class->getTableName() . '_' . $class->columnNames[$class->identifier[0]] . '_seq' :
- null;
- $class->setIdGenerator(new \Doctrine\ORM\Id\IdentityGenerator($seqName));
+ $sequenceName = null;
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Missing line break between assignment and control structure

lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
+ $columnName = $class->getSingleIdentifierColumnName();
+ $quoted = isset($class->fieldMappings[$fieldName]['quoted']) || isset($class->table['quoted']);
+ $sequenceName = $class->getTableName() . '_' . $columnName . '_seq';
+ $definition = array(
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

"=" is not correctly indented to other ones.

lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
+ $definition = array(
+ 'sequenceName' => $this->targetPlatform->fixSchemaElementName($sequenceName)
+ );
+ if ($quoted) {
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Missing line break between assignment and control structure

lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
+ if ($quoted) {
+ $definition['quoted'] = true;
+ }
+ $sequenceName = $this->em->getConfiguration()->getQuoteStrategy()->getSequenceName($definition, $class, $this->targetPlatform);
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Missing line break between control structure and assignment

lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
+ }
+ $sequenceName = $this->em->getConfiguration()->getQuoteStrategy()->getSequenceName($definition, $class, $this->targetPlatform);
+ }
+ $class->setIdGenerator(new \Doctrine\ORM\Id\IdentityGenerator($sequenceName));
lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
+ $columnName = $class->getSingleIdentifierColumnName();
+ $quoted = isset($class->fieldMappings[$fieldName]['quoted']) || isset($class->table['quoted']);
+ $sequenceName = $class->getTableName() . '_' . $columnName . '_seq';
+ $definition['sequenceName'] = $this->targetPlatform->fixSchemaElementName($sequenceName);
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

We clearly have 2 logical groups of assignment here. Please consider a line break between them (variables creation and array indexes definition).

lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
+ $definition['sequenceName'] = $this->targetPlatform->fixSchemaElementName($sequenceName);
+ $definition['allocationSize'] = 1;
+ $definition['initialValue'] = 1;
+ if ($quoted) {
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Missing line break between assignment and control structure

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
+ }
+
+ if ($joinColumn['referencedColumnName'][0] == '`') {
+ $joinColumn['referencedColumnName'] = trim($joinColumn['referencedColumnName'], '`');
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Both "=" should be aligned since they imply a logical block creation

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
+ }
+
+ if ($joinColumn['referencedColumnName'][0] == '`') {
+ $joinColumn['referencedColumnName'] = trim($joinColumn['referencedColumnName'], '`');
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Align both "=" signs

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
+ }
+
+ if ($inverseJoinColumn['referencedColumnName'][0] == '`') {
+ $inverseJoinColumn['referencedColumnName'] = trim($inverseJoinColumn['referencedColumnName'], '`');
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Align both equal signs

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
if (empty($joinColumn['referencedColumnName'])) {
$joinColumn['referencedColumnName'] = $this->namingStrategy->referenceColumnName();
}
+
+ if ($joinColumn['name'][0] == '`') {
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Can't we use "===" here?

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
+ $joinColumn['quoted'] = true;
+ }
+
+ if ($joinColumn['referencedColumnName'][0] == '`') {
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Can't we use "===" here?

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
if (empty($joinColumn['referencedColumnName'])) {
$joinColumn['referencedColumnName'] = $this->namingStrategy->referenceColumnName();
}
+
+ if ($joinColumn['name'][0] == '`') {
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Can't we use "===" here?

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
+ $joinColumn['quoted'] = true;
+ }
+
+ if ($joinColumn['referencedColumnName'][0] == '`') {
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Can't we use "===" here?

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
if (empty($inverseJoinColumn['referencedColumnName'])) {
$inverseJoinColumn['referencedColumnName'] = $this->namingStrategy->referenceColumnName();
}
+
+ if ($inverseJoinColumn['name'][0] == '`') {
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Can't we use "===" here?

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
+ $inverseJoinColumn['quoted'] = true;
+ }
+
+ if ($inverseJoinColumn['referencedColumnName'][0] == '`') {
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Can't we use "===" here?

+ */
+class DefaultQuoteStrategy implements QuoteStrategy
+{
+
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Extra line here, remove it

lib/Doctrine/ORM/Mapping/DefaultQuoteStrategy.php
+ // Association defined as Id field
+ $joinColumns = $class->associationMappings[$fieldName]['joinColumns'];
+ $assocQuotedColumnNames = array_map(
+ function ($joinColumn) use ($platform) {
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Our PHP standards imply curly braces to be opened on a new line here.

- $this->_class->getQuotedJoinTableName($mapping, $this->_platform),
- array_combine($keys, $identifier)
- );
+
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Extra line here?

lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
- $id = array_combine($this->_class->getIdentifierColumnNames(), $identifier);
- $this->_conn->delete($this->_class->getQuotedTableName($this->_platform), $id);
+ $id = array_combine($this->quoteStrategy->getIdentifierColumnNames($this->_class, $this->_platform), $identifier);
+ $this->_conn->delete($this->quoteStrategy->getTableName($this->_class, $this->_platform), $id);
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Line break between assignment and function call

lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
+ foreach ($assoc['joinColumns'] as $joinColumn) {
+ $sourceColumn = $joinColumn['name'];
+ $targetColumn = $joinColumn['referencedColumnName'];
+
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Line break is wrong here. Combine assignments and add the line break between the logical blocks. One is variable creation the other is the array index definition.

lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
+ $resultColumnName = $this->getSQLColumnAlias($joinColumn['name']);
+ $columnList .= $this->_getSQLTableAlias($class->name, ($alias == 'r' ? '' : $alias) )
+ . '.' . $quotedColumn . ' AS ' . $resultColumnName;
+ $this->_rsm->addMetaResult($alias, $resultColumnName, $quotedColumn, isset($assoc['id']) && $assoc['id'] === true);
@guilhermeblanco

guilhermeblanco Jun 18, 2012

Owner

Missing line break here

This pull request fails (merged 039c23ad into 41d9f61).

This pull request fails (merged b9d94e7 into 41d9f61).

Owner

FabioBatSilva commented Jun 18, 2012

Hi @guilhermeblanco,

Fixed !!
Please take a look again

Thanks

This pull request fails (merged 65efda4 into 41d9f61).

This pull request fails (merged 7800a7e into 41d9f61).

- $class->setIdGenerator(new \Doctrine\ORM\Id\IdentityGenerator($seqName));
+ $sequenceName = null;
+
+ if($this->targetPlatform instanceof Platforms\PostgreSQLPlatform) {
@guilhermeblanco

guilhermeblanco Jun 22, 2012

Owner

Missing space between if and open parenthesis

+ $sequenceName = $this->em->getConfiguration()->getQuoteStrategy()->getSequenceName($definition, $class, $this->targetPlatform);
+ }
+
+ $class->setIdGenerator(new \Doctrine\ORM\Id\IdentityGenerator($sequenceName));
break;
@guilhermeblanco

guilhermeblanco Jun 22, 2012

Owner

Missing line break between break and next case

if (isset($this->_class->fieldMappings[$this->_class->fieldNames[$columnName]]['requireSQLConversion'])) {
$type = Type::getType($this->_columnTypes[$columnName]);
$placeholder = $type->convertToDatabaseValueSQL('?', $this->_platform);
}
+ } else if(isset($this->quotedColumns[$columnName])) {
@guilhermeblanco

guilhermeblanco Jun 22, 2012

Owner

Missing space between if and open parenthesis

- $this->_class->getQuotedJoinTableName($mapping, $this->_platform),
- array_combine($otherKeys, $identifier)
- );
+ $this->_conn->delete($joinTableName,array_combine($otherKeys, $identifier));
@guilhermeblanco

guilhermeblanco Jun 22, 2012

Owner

Missing space after comma

@@ -1069,13 +1108,16 @@ protected function _getSelectColumnAssociationSQL($field, $assoc, ClassMetadata
$columnList = '';
if ($assoc['isOwningSide'] && $assoc['type'] & ClassMetadata::TO_ONE) {
- foreach ($assoc['targetToSourceKeyColumns'] as $srcColumn) {
+ foreach ($assoc['joinColumns'] as $joinColumn) {
+
@guilhermeblanco

guilhermeblanco Jun 22, 2012

Owner

Extra line here?

-
- if (!$schema->hasSequence($seqDef['sequenceName'])) {
+ $seqDef = $class->sequenceGeneratorDefinition;
+ $quotedName = $this->quoteStrategy->getSequenceName($seqDef, $class, $this->platform);
@guilhermeblanco

guilhermeblanco Jun 22, 2012

Owner

Missing line break between assignment and control structure

lib/Doctrine/ORM/Tools/SchemaTool.php
}
}
}
+
if (!$table->hasIndex('primary')) {
@guilhermeblanco

guilhermeblanco Jun 22, 2012

Owner

Missing spaces around !.

This pull request fails (merged ca4862a into 41d9f61).

Owner

FabioBatSilva commented Jun 23, 2012

DONE !!!

Owner

Ocramius commented Jun 23, 2012

@FabioBatSilva are the failures now caused by the PR? They're not matching the ones I used to see on PostgreSQL...

Owner

FabioBatSilva commented Jun 23, 2012

Hi @Ocramius

PostgreSQL fails on cache, even in doctrine/master. not related with my patch.
To fix SQLite doctrine/dbal#158 shoud be merged.

Thanks .

Owner

Ocramius commented Jun 23, 2012

good to know =)

guilhermeblanco added a commit that referenced this pull request Jun 25, 2012

@guilhermeblanco guilhermeblanco merged commit cb72219 into doctrine:master Jun 25, 2012

@FabioBatSilva FabioBatSilva deleted the FabioBatSilva:DDC-1845 branch Jan 18, 2013

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