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

fix(datastore): query limit more than 300 #5592

Merged
merged 20 commits into from
Dec 19, 2022
Merged

fix(datastore): query limit more than 300 #5592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
63655f5
fix(datastore): fix query limit above 300
vishwarajanand Nov 9, 2022
0fc89a1
chore: move GQL query test
vishwarajanand Nov 9, 2022
f902098
fix: test for gql query 300 limit
vishwarajanand Nov 9, 2022
74d8ddd
Merge branch 'main' into feat_datastore_query_limit
vishwarajanand Nov 21, 2022
8ba0ac3
fix: limit last page size
vishwarajanand Nov 21, 2022
5086fa0
fix: style
vishwarajanand Nov 21, 2022
4fd4c7f
Merge branch 'main' into feat_datastore_query_limit
vishwarajanand Nov 21, 2022
29e660b
Merge branch 'main' into feat_datastore_query_limit
vishwarajanand Dec 8, 2022
e60e12e
fix: merge conflicts
vishwarajanand Dec 8, 2022
5940fdf
fix: test provider should be default db only
vishwarajanand Dec 8, 2022
291b45b
Merge branch 'main' into feat_datastore_query_limit
vishwarajanand Dec 13, 2022
691531e
fix: contained remainingLimit logic in runQueryFb
vishwarajanand Dec 13, 2022
950c2af
fix: ci build jobs cancel out
vishwarajanand Dec 13, 2022
9976618
Merge branch 'main' into feat_datastore_query_limit
vishwarajanand Dec 13, 2022
e88e152
Merge branch 'googleapis:main' into feat_datastore_query_limit
vishwarajanand Dec 13, 2022
8e348f7
Merge branch 'main' into feat_datastore_query_limit
vishwarajanand Dec 14, 2022
a37b3c4
revert: undo EntityPageIterator
vishwarajanand Dec 14, 2022
71ac41c
fix: removing resultLimit from Operation
vishwarajanand Dec 14, 2022
31448e4
nit: migrated to is_null for var check
vishwarajanand Dec 15, 2022
89b682c
Merge branch 'main' into feat_datastore_query_limit
vishwarajanand Dec 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Datastore/src/Connection/Grpc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ public function runQuery(array $args)
$query['filter'] = $this->convertFilterProps($query['filter']);
}

if (isset($query['limit'])) {
if (isset($query['limit']) && is_int($query['limit'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@vishwarajanand why was this added? This seems like it could break existing implementations if they supplied something like $query['limit'] = '1'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bshaffer I mentioned this in point #1 in PR description > Changes:

Under Grpc mode, when a Gql query string goes through runQuery, and a parsed query object is returned with correct limit array, it doesn't need to be modified in subsequent fetches. Added a check to skip that.

In Grpc mode, the non-Gql query provides$query['limit'] as an int which needs to be parsed into an array, but your comment suggests we support int-like string also.

If so, we should change the is_int to !is_array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bshaffer I raised another PR to fix this bug: #5826

$query['limit'] = [
'value' => $query['limit']
];
Expand Down
53 changes: 35 additions & 18 deletions Datastore/src/Operation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ public function keys($kind, array $options = [])
'number' => 1,
'ancestors' => [],
'id' => null,
'name' => null
'name' => null,
];

if ($options['number'] < 1) {
Expand All @@ -167,7 +167,7 @@ public function keys($kind, array $options = [])
$path[] = array_filter([
'kind' => $kind,
'id' => $options['id'],
'name' => $options['name']
'name' => $options['name'],
]);

$key = new Key($this->projectId, [
Expand Down Expand Up @@ -226,7 +226,7 @@ public function keys($kind, array $options = [])
public function entity($key = null, array $entity = [], array $options = [])
{
$options += [
'className' => null
'className' => null,
];

if ($key && !is_string($key) && !($key instanceof Key)) {
Expand Down Expand Up @@ -362,7 +362,7 @@ public function lookup(array $keys, array $options = [])
{
$options += [
'className' => Entity::class,
'sort' => false
'sort' => false,
];

$serviceKeys = [];
Expand Down Expand Up @@ -446,7 +446,7 @@ public function runQuery(QueryInterface $query, array $options = [])
$options += [
'className' => Entity::class,
'namespaceId' => $this->namespaceId,
'databaseId' => $this->databaseId
'databaseId' => $this->databaseId,
];

$iteratorConfig = [
Expand All @@ -465,25 +465,31 @@ public function runQuery(QueryInterface $query, array $options = [])
}

return false;
}
},
];

if (array_key_exists('limit', $query->queryObject())) {
$remainingLimit = $query->queryObject()['limit'];
}
$runQueryObj = clone $query;
$runQueryFn = function (array $args = []) use (&$runQueryObj, $options) {
$runQueryFn = function (array $args = []) use (&$runQueryObj, $options, &$remainingLimit) {
$args += [
'query' => []
'query' => [],
];

// The iterator provides the startCursor for subsequent pages as an argument.
$requestQueryArr = $args['query'] + $runQueryObj->queryObject();
if (isset($remainingLimit)) {
$requestQueryArr['limit'] = $remainingLimit;
}
$request = [
'projectId' => $this->projectId,
'partitionId' => $this->partitionId(
$this->projectId,
$options['namespaceId'],
$options['databaseId']
),
$runQueryObj->queryKey() => $requestQueryArr
$runQueryObj->queryKey() => $requestQueryArr,
] + $this->readOptions($options) + $options;

$res = $this->connection->runQuery($request);
Expand All @@ -497,6 +503,17 @@ public function runQuery(QueryInterface $query, array $options = [])
if (isset($res['query'])) {
$runQueryObj = new Query($this->entityMapper, $res['query']);
}
if (isset($res['query']['limit'])) {
// limit for GqlQuery in REST mode
$remainingLimit = $res['query']['limit'];
}
if (isset($remainingLimit['value'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks redundant - ['value'] could be coming from $res['query']['limit'], or from $query->queryObject()['limit']. So why set it above just to set it again here? It seems to me like what you'd want to do is set $remainingLimit to null, above, and then simply make this:

            if (isset($res['query']['limit']['value'])) {
                $remainingLimit = $res['query']['limit']['value'];
            }

But maybe there's something I'm missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are several cases here:

  1. In a standard Query, (either GRPC or REST) we don't get a query key in the response. So, we set $remainingLimit using $query->queryObject()['limit']:

    google-cloud-php/Datastore/src/Operation.php

    Line 472 in 71ac41c

    $remainingLimit = $query->queryObject()['limit'];
  2. In GqlQuery, we don't have limit from the query. So while using REST, we get limit in $res['query']['limit']:

    google-cloud-php/Datastore/src/Operation.php

    Line 507 in 71ac41c

    $remainingLimit = $res['query']['limit'];
  3. In GqlQuery while using GRPC, we get limit in $res['query']['limit']['value']:

    google-cloud-php/Datastore/src/Operation.php

    Lines 509 to 510 in 71ac41c

    if (isset($remainingLimit['value'])) {
    $remainingLimit = $remainingLimit['value'];

// limit for GqlQuery in GRPC mode
$remainingLimit = $remainingLimit['value'];
}
if (!is_null($remainingLimit)) {
$remainingLimit -= count($res['batch']['entityResults']);
}

return $res;
};
Expand Down Expand Up @@ -541,7 +558,7 @@ public function commit(array $mutations, array $options = [])
$res = $this->connection->commit($options + [
'mode' => ($options['transaction']) ? 'TRANSACTIONAL' : 'NON_TRANSACTIONAL',
'mutations' => $mutations,
'projectId' => $this->projectId
'projectId' => $this->projectId,
]);

return $res;
Expand Down Expand Up @@ -622,7 +639,7 @@ public function mutation(

return array_filter([
$operation => $data,
'baseVersion' => $baseVersion
'baseVersion' => $baseVersion,
]);
}

Expand Down Expand Up @@ -657,9 +674,9 @@ public function checkOverwrite(array $entities, $allowOverwrite = false)
foreach ($entities as $entity) {
if (!$entity->populatedByService() && !$allowOverwrite) {
throw new \InvalidArgumentException(sprintf(
'Given entity cannot be saved because it may overwrite an '.
'existing record. When updating manually created entities, '.
'please set the options `$allowOverwrite` flag to `true`. '.
'Given entity cannot be saved because it may overwrite an ' .
'existing record. When updating manually created entities, ' .
'please set the options `$allowOverwrite` flag to `true`. ' .
'Invalid entity key was %s',
(string) $entity->key()
));
Expand Down Expand Up @@ -734,7 +751,7 @@ private function mapEntityResult(array $result, $class)
'className' => $className,
'populatedByService' => true,
'excludeFromIndexes' => $excludes,
'meanings' => $meanings
'meanings' => $meanings,
]);
}

Expand All @@ -754,16 +771,16 @@ private function readOptions(array $options = [])
{
$options += [
'readConsistency' => null,
'transaction' => null
'transaction' => null,
];

$readOptions = array_filter([
'readConsistency' => $options['readConsistency'],
'transaction' => $options['transaction']
'transaction' => $options['transaction'],
]);

return array_filter([
'readOptions' => $readOptions
'readOptions' => $readOptions,
]);
}

Expand Down
28 changes: 26 additions & 2 deletions Datastore/tests/System/QueryResultPaginationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public static function set_up_before_class()
$key->ancestorKey(self::$parentKey);

$set[] = $client->entity($key, [
'a' => rand(1, 10)
'a' => rand(1, 10),
]);

if (count($set) === 100) {
Expand Down Expand Up @@ -128,12 +128,25 @@ public function testGqlQueryPaginationByPage(DatastoreClient $client)
$parentId
);
$q = $client->gqlQuery($queryString, [
'allowLiterals' => true
'allowLiterals' => true,
]);

$this->assertQueryPageCount(self::$expectedTotal, $client, $q);
}

/**
* @dataProvider defaultDbClientProvider
*/
public function testGqlQueryPaginationWithLimit(DatastoreClient $client)
{
$testLimit = 310;
$q = $client->gqlQuery(
sprintf('SELECT * FROM `%s` LIMIT %s', self::$testKind, $testLimit),
['allowLiterals' => true]
);
$this->assertQueryCount($testLimit, $client, $q);
}

/**
* @dataProvider defaultDbClientProvider
*/
Expand All @@ -144,6 +157,17 @@ public function testQueryPaginationByPage(DatastoreClient $client)
$this->assertQueryPageCount(self::$expectedTotal, $client, $q);
}

/**
* @dataProvider defaultDbClientProvider
*/
public function testQueryPaginationWithLimit(DatastoreClient $client)
{
$testLimit = 310;
$q = $client->query()->kind(self::$testKind)->limit($testLimit);

$this->assertQueryPageCount($testLimit, $client, $q);
}

private function assertQueryCount($expected, DatastoreClient $client, QueryInterface $query)
{
$res = $client->runQuery($query);
Expand Down