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

feat: Sum and average aggregation queries #1097

Merged
merged 44 commits into from Sep 1, 2023
Merged
Show file tree
Hide file tree
Changes from 41 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
ce9413f
Initial sum aggregation
danieljbruce Mar 27, 2023
cadb0cc
Modify encoding
danieljbruce Mar 27, 2023
274c606
PropertyAggregateField with tests
danieljbruce Mar 27, 2023
a87e277
Improve transaction tests
danieljbruce Apr 27, 2023
7548e81
Change the description in the describe block
danieljbruce Apr 28, 2023
aaaff65
Change return type to average
danieljbruce Apr 28, 2023
02f0f7d
Make alias optional
danieljbruce Apr 28, 2023
256a4d8
Fix the transaction tests to fail on rollback
danieljbruce Apr 28, 2023
970c0f4
Add additional assertions to existing tests
danieljbruce Apr 28, 2023
ba7aef8
Revert "Add additional assertions to existing tests"
danieljbruce Apr 28, 2023
3a204f5
Add describe block for comparing equivalent query
danieljbruce Apr 28, 2023
b403185
Average, sum and count toProto tests
danieljbruce Apr 28, 2023
8312db7
Add tests for the sum aggregation
danieljbruce May 1, 2023
035df48
Add a test for sum and snapshot reads
danieljbruce May 1, 2023
6a44483
Add two test blocks for special cases
danieljbruce May 2, 2023
cfd745e
Export aggregate field from the client
danieljbruce May 2, 2023
d74c158
Merge branch 'main' of https://github.com/googleapis/nodejs-datastore…
danieljbruce Jun 9, 2023
a67b34b
PR follow-up changes
danieljbruce Jun 9, 2023
4d58133
Adjust the values so that tests pass
danieljbruce Aug 9, 2023
e3b2c62
Add average aggregations
danieljbruce Aug 9, 2023
5164596
Add snapshot reads for run query and aggregate q
danieljbruce Aug 10, 2023
7db60c1
Remove Google error and entity filter
danieljbruce Aug 10, 2023
8868bbb
Should use null for an aggregation query read time
danieljbruce Aug 11, 2023
ff2cd79
Remove tests from a bad cherry pick
danieljbruce Aug 11, 2023
e3b6841
Merge branch 'main' of https://github.com/googleapis/nodejs-datastore…
danieljbruce Aug 11, 2023
6acfb77
Linting fix
danieljbruce Aug 11, 2023
530df2c
Do the test on rating instead of appearances
danieljbruce Aug 11, 2023
eea8602
The assertion says the request should have failed
danieljbruce Aug 11, 2023
8c4db96
Add a comment about using limits in test
danieljbruce Aug 16, 2023
c7de99c
Add rollbacks to transaction tests
danieljbruce Aug 16, 2023
a57c4a8
refactor getSharedOptionsOnly
danieljbruce Aug 17, 2023
8ee7061
Remove test related to snapshot reads
danieljbruce Aug 18, 2023
4f01c43
Add a test for multiple types of aggregates
danieljbruce Aug 18, 2023
5be7d69
Correct descriptions of two tests on overflow
danieljbruce Aug 18, 2023
0ed5509
Merge branch 'main' into sum-avg
danieljbruce Aug 18, 2023
3b53859
Add a comment for setting the alias
danieljbruce Aug 25, 2023
af55599
Add tests to compare various ways to encode alias
danieljbruce Aug 25, 2023
98afdd6
Added tests for when an empty alias is provided
danieljbruce Aug 28, 2023
15840dd
Add a comment clarifying the use of snapshot reads
danieljbruce Aug 28, 2023
bbff19f
Add two tests to explore mixed aggregations alias
danieljbruce Aug 28, 2023
2b66bae
Better names for some internal private functions
danieljbruce Aug 28, 2023
6d882a3
Add a comment explaining why the sleep is needed
danieljbruce Aug 30, 2023
f7a6873
Add getReadTime function and use for sum/avg
danieljbruce Aug 31, 2023
3681c06
Rename variable to emptyData
danieljbruce Aug 31, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
104 changes: 97 additions & 7 deletions src/aggregate.ts
Expand Up @@ -46,11 +46,35 @@
* @param {string} alias
* @returns {AggregateQuery}
*/
count(alias: string): AggregateQuery {
count(alias?: string): AggregateQuery {
this.aggregations.push(AggregateField.count().alias(alias));
return this;
}

/**
* Add a `sum` aggregate query to the list of aggregations.
*
* @param {string} property
* @param {string} alias
* @returns {AggregateQuery}
*/
sum(property: string, alias?: string): AggregateQuery {
this.aggregations.push(AggregateField.sum(property).alias(alias));
return this;
}

/**
* Add a `average` aggregate query to the list of aggregations.
*
* @param {string} property
* @param {string} alias
* @returns {AggregateQuery}
*/
average(property: string, alias?: string): AggregateQuery {
this.aggregations.push(AggregateField.average(property).alias(alias));
return this;
}

/**
* Add a custom aggregation to the list of aggregations.
*
Expand Down Expand Up @@ -99,8 +123,7 @@
* Get the proto for the list of aggregations.
*
*/
// eslint-disable-next-line
toProto(): any {

Check warning on line 126 in src/aggregate.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
return this.aggregations.map(aggregation => aggregation.toProto());
}
}
Expand All @@ -122,23 +145,42 @@
}

/**
* Gets a copy of the Count aggregate field.
* Gets a copy of the Sum aggregate field.
*
* @returns {Sum}
*/
static sum(property: string): Sum {
return new Sum(property);
}

/**
* Gets a copy of the Average aggregate field.
*
* @returns {Average}
*/
static average(property: string): Average {
return new Average(property);
danieljbruce marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Sets the alias on the aggregate field that should be used.
*
* @param {string} alias The label used in the results to describe this
* aggregate field when a query is run.
* @returns {AggregateField}
*/
alias(alias: string): AggregateField {
this.alias_ = alias;
alias(alias?: string): AggregateField {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add some additional testing on Count now that this is optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Added in commit called "Added tests for when an empty alias is provided".

if (alias) {
this.alias_ = alias;
}
return this;
}

/**
* Gets the proto for the aggregate field.
*
*/
// eslint-disable-next-line
abstract toProto(): any;

Check warning on line 183 in src/aggregate.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
}

/**
Expand All @@ -146,15 +188,63 @@
*
*/
class Count extends AggregateField {
// eslint-disable-next-line
/**
* Gets the proto for the count aggregate field.
*
*/
toProto(): any {

Check warning on line 195 in src/aggregate.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
const count = Object.assign({});
return Object.assign({count}, this.alias_ ? {alias: this.alias_} : null);
}
}

/**
* A PropertyAggregateField is a class that contains data that defines any
* aggregation that is performed on a property.
*
*/
abstract class PropertyAggregateField extends AggregateField {
abstract operator: string;

/**
* Build a PropertyAggregateField object.
*
* @param {string} property
*/
constructor(public property_: string) {
super();
}

/**
* Gets the proto for the property aggregate field.
*
*/
toProto(): any {

Check warning on line 222 in src/aggregate.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
const aggregation = this.property_
? {property: {name: this.property_}}
: {};
return Object.assign(
{operator: this.operator},
this.alias_ ? {alias: this.alias_} : null,
{[this.operator]: aggregation}
);
}
}

/**
* A Sum is a class that contains data that defines a Sum aggregation.
*
*/
class Sum extends PropertyAggregateField {
operator = 'sum';
}

/**
* An Average is a class that contains data that defines an Average aggregation.
*
*/
class Average extends PropertyAggregateField {
operator = 'avg';
}

export {AggregateField, AggregateQuery, AGGREGATE_QUERY};
4 changes: 2 additions & 2 deletions src/index.ts
Expand Up @@ -40,8 +40,9 @@ import * as is from 'is';
import {Transform, pipeline} from 'stream';

import {entity, Entities, Entity, EntityProto, ValueProto} from './entity';
import {AggregateField} from './aggregate';
import Key = entity.Key;
export {Entity, Key};
ruyadorno marked this conversation as resolved.
Show resolved Hide resolved
export {Entity, Key, AggregateField};
import {PropertyFilter, and, or} from './filter';
export {PropertyFilter, and, or};
import {
Expand Down Expand Up @@ -1818,7 +1819,6 @@ promisifyAll(Datastore, {
'isDouble',
'geoPoint',
'getProjectId',
'getSharedQueryOptions',
ruyadorno marked this conversation as resolved.
Show resolved Hide resolved
'isGeoPoint',
'index',
'int',
Expand Down
53 changes: 25 additions & 28 deletions src/request.ts
Expand Up @@ -44,7 +44,7 @@
KeyProto,
ResponseResult,
Entities,
ValueProto,

Check warning on line 47 in src/request.ts

View workflow job for this annotation

GitHub Actions / lint

'ValueProto' is defined but never used
} from './entity';
import {
Query,
Expand Down Expand Up @@ -276,28 +276,8 @@
}

const makeRequest = (keys: entity.Key[] | KeyProto[]) => {
const reqOpts: RequestOptions = {
keys,
};

if (options.consistency) {
const code = CONSISTENCY_PROTO_CODE[options.consistency.toLowerCase()];

reqOpts.readOptions = {
readConsistency: code,
};
}
if (options.readTime) {
if (reqOpts.readOptions === undefined) {
reqOpts.readOptions = {};
}
const readTime = options.readTime;
const seconds = readTime / 1000;
reqOpts.readOptions.readTime = {
seconds: Math.floor(seconds),
};
}

const reqOpts = this.getRequestOptions(options);
Object.assign(reqOpts, {keys});
this.request_(
{
client: 'DatastoreClient',
Expand Down Expand Up @@ -596,7 +576,7 @@
setImmediate(callback, e as Error);
return;
}
const sharedQueryOpts = this.getSharedQueryOptions(query.query, options);
const sharedQueryOpts = this.getQueryOptions(query.query, options);
const aggregationQueryOptions: AggregationQueryOptions = {
nestedQuery: queryProto,
aggregations: query.toProto(),
Expand All @@ -616,9 +596,9 @@
const results = res.batch.aggregationResults;
const finalResults = results
.map(
(aggregationResult: any) => aggregationResult.aggregateProperties

Check warning on line 599 in src/request.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
)
.map((aggregateProperties: any) =>

Check warning on line 601 in src/request.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
Object.fromEntries(
new Map(
Object.keys(aggregateProperties).map(key => [
Expand Down Expand Up @@ -811,7 +791,7 @@
setImmediate(onResultSet, e as Error);
return;
}
const sharedQueryOpts = this.getSharedQueryOptions(query, options);
const sharedQueryOpts = this.getQueryOptions(query, options);

const reqOpts: RequestOptions = sharedQueryOpts;
reqOpts.query = queryProto;
Expand Down Expand Up @@ -887,9 +867,8 @@
return stream;
}

private getSharedQueryOptions(
query: Query,
options: RunQueryStreamOptions = {}
private getRequestOptions(
options: RunQueryStreamOptions
): SharedQueryOptions {
const sharedQueryOpts = {} as SharedQueryOptions;
if (options.consistency) {
Expand All @@ -898,6 +877,24 @@
readConsistency: code,
};
}
if (options.readTime) {
if (sharedQueryOpts.readOptions === undefined) {
sharedQueryOpts.readOptions = {};
}
const readTime = options.readTime;
const seconds = readTime / 1000;
sharedQueryOpts.readOptions.readTime = {
seconds: Math.floor(seconds),
};
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code was taken from

if (options.readTime) {
if (reqOpts.readOptions === undefined) {
reqOpts.readOptions = {};
}
const readTime = options.readTime;
const seconds = readTime / 1000;
reqOpts.readOptions.readTime = {
seconds: Math.floor(seconds),
};
}
and can be refactored after the merge as per the TODO.

Copy link
Contributor

Choose a reason for hiding this comment

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

any reason why we can't refactor this now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason. It is a choice between having two simpler PRs and one PR that includes both changes. Doing so now is fine though.

return sharedQueryOpts;
}

private getQueryOptions(
query: Query,
options: RunQueryStreamOptions = {}
): SharedQueryOptions {
const sharedQueryOpts = this.getRequestOptions(options);
if (query.namespace) {
sharedQueryOpts.partitionId = {
namespaceId: query.namespace,
Expand Down Expand Up @@ -936,7 +933,7 @@
callback?: SaveCallback
): void | Promise<CommitResponse> {
const transaction = this.datastore.transaction();
transaction.run(async (err: any) => {

Check warning on line 936 in src/request.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
if (err) {
try {
await transaction.rollback();
Expand Down Expand Up @@ -1191,7 +1188,7 @@
* that a callback is omitted.
*/
promisifyAll(DatastoreRequest, {
exclude: ['getSharedQueryOptions'],
exclude: ['getQueryOptions', 'getRequestOptions'],
});

/**
Expand Down