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.

Sign up for GitHub

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

Jump to bottom

Use the defined func()->count() instead of manual counting #11942

Merged
merged 1 commit into from
Nov 12, 2018
Merged

Use the defined func()->count() instead of manual counting #11942

merged 1 commit into from
Nov 12, 2018

Conversation

nickvergessen
Copy link
Member

Now there are only 2 "functions" left which are both called twice and don't have a wrapper:

  • GREATEST
  • COUNT(DISTINCT …)

Not sure if it's worth the effort.

@nickvergessen nickvergessen added this to the Nextcloud 15 milestone Oct 19, 2018
icewind1991
icewind1991 reviewed Oct 19, 2018
View reviewed changes
lib/private/DB/QueryBuilder/FunctionBuilder/FunctionBuilder.php
@@ -72,7 +72,8 @@ public function subtract($x, $y) {
return new QueryFunction($this->helper->quoteColumnName($x) . ' - ' . $this->helper->quoteColumnName($y));
}

public function count($input) {
return new QueryFunction('COUNT(' . $this->helper->quoteColumnName($input) . ')');
public function count($count, $alias = '') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't alias work by just using selectAlias($qb->func()->count(...), 'alias')

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, i added it here because its done quite often

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having it here doesn't make sense to me though, since for the caller it's not any easier (the only difference is where you place the )) and it's duplicating code/functionality

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this here. It also reduces function nesting.

MorrisJobke
MorrisJobke approved these changes Nov 5, 2018
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good 👍

@MorrisJobke MorrisJobke mentioned this pull request Nov 6, 2018
Merged
29 tasks
@MorrisJobke
Copy link
Member

@icewind1991 @rullzer Any comments on this one?

icewind1991
icewind1991 approved these changes Nov 7, 2018
View reviewed changes
Copy link
Member

@icewind1991 icewind1991 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine with me

@MorrisJobke
Copy link
Member

Yay PHPunit tests fail 😢 I should have checked that earlier

@MorrisJobke
Copy link
Member

Yay PHPunit tests fail 😢 I should have checked that earlier

Ah - disk was just full -> let's do another round: https://drone.nextcloud.com/nextcloud/server/12482

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 7, 2018
@MorrisJobke
Copy link
Member

This causes the same "Disk is full" errors 😨

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Nov 7, 2018
@nickvergessen
Copy link
Member Author

strange, let me investigate

@nickvergessen nickvergessen force-pushed the techdebt/noid/use-count-function branch 2 times, most recently from 08ee437 to 7365078 Compare November 7, 2018 19:02
nickvergessen
nickvergessen commented Nov 7, 2018
View reviewed changes
lib/private/Comments/Manager.php
$qb->createFunction('COUNT(' . $qb->getColumnName('c.id') . ')'),
'num_ids'
)
->addSelect($qb->func()->count('c.id', 'num_ids'))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MorrisJobke the problem was select here instead of addSelect which caused a lot of spam with missing index fileId across tests

@nickvergessen nickvergessen added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Nov 7, 2018
@MorrisJobke
Copy link
Member

Still this one:

Test\SystemTag\SystemTagObjectMapperTest::testHaveTagAllMatches

Doctrine\DBAL\Exception\InvalidFieldNameException: An exception occurred while executing 'SELECT COUNT(`1`) FROM `oc_systemtag_object_mapping` WHERE (`objectid` IN (?)) AND (`objecttype` = ?) AND (`systemtagid` = ?)' with params ["1", "testtype", "1"]:


SQLSTATE[42S22]: Column not found: 1054 Unknown column '1' in 'field list'

...

@nickvergessen
Copy link
Member Author

fixed

@nickvergessen nickvergessen force-pushed the techdebt/noid/use-count-function branch from 7365078 to 82fb91b Compare November 8, 2018 11:21
@MorrisJobke MorrisJobke mentioned this pull request Nov 8, 2018
Merged
24 tasks
@MorrisJobke
Copy link
Member

Fails on Postgres https://drone.nextcloud.com/nextcloud/server/12537/167:

1) Test\SystemTag\SystemTagObjectMapperTest::testHaveTagAllMatches
Doctrine\DBAL\Exception\DriverException: An exception occurred while executing 'SELECT COUNT(?) FROM "oc_systemtag_object_mapping" WHERE ("objectid" IN (?)) AND ("objecttype" = ?) AND ("systemtagid" = ?)' with params [1, "1", "testtype", "1"]:

SQLSTATE[42P18]: Indeterminate datatype: 7 ERROR:  could not determine data type of parameter $1

/drone/src/github.com/nextcloud/server/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/AbstractPostgreSQLDriver.php:92
/drone/src/github.com/nextcloud/server/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/DBALException.php:128
/drone/src/github.com/nextcloud/server/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:855
/drone/src/github.com/nextcloud/server/lib/private/DB/Connection.php:195
/drone/src/github.com/nextcloud/server/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Query/QueryBuilder.php:206
/drone/src/github.com/nextcloud/server/lib/private/DB/QueryBuilder/QueryBuilder.php:214
/drone/src/github.com/nextcloud/server/lib/private/SystemTag/SystemTagObjectMapper.php:231
/drone/src/github.com/nextcloud/server/tests/lib/SystemTag/SystemTagObjectMapperTest.php:291

@MorrisJobke MorrisJobke added the 2. developing Work in progress label Nov 8, 2018
@MorrisJobke MorrisJobke removed the 4. to release Ready to be released and/or waiting for tests to finish label Nov 8, 2018
@nickvergessen
Use the defined func()->count() instead of manual counting
bb352fb 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the techdebt/noid/use-count-function branch from 82fb91b to bb352fb Compare November 8, 2018 14:44
MorrisJobke
MorrisJobke approved these changes Nov 12, 2018
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@MorrisJobke MorrisJobke merged commit fd8eecc into master Nov 12, 2018
@MorrisJobke MorrisJobke deleted the techdebt/noid/use-count-function branch November 12, 2018 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@icewind1991 icewind1991 icewind1991 approved these changes

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@rullzer rullzer Awaiting requested review from rullzer

Assignees
No one assigned
Labels
2. developing Work in progress technical debt
Projects
None yet
Milestone
Nextcloud 15
Development

Successfully merging this pull request may close these issues.
3 participants
@nickvergessen @MorrisJobke @icewind1991