feat: Sum and average aggregation queries #1097

danieljbruce · 2023-03-27T21:41:00Z

This PR demonstrates how we are going to leverage the current framework to support sum/average queries for later use.

This commit introduces the sum aggregation with a simple test to ensure it works

This change modifies the encoding so that the right data reaches the grpc layer

Adds helper functions and a super class so that average and sum can share the same properties.

src/aggregate.ts

Add sum and average to the transaction tests here to improve test coverage for transactions.

The return type for this function is wrong. The function returns an Average so we should use Average.

Make alias optional since the query still works without providing an alias.

The transaction tests should not pass if the transaction is rolled back like they do currently. We must not catch errors and instead let the test fail.

Ensure that the addAggregation function works correctly with an additional assertion check.

This reverts commit 970c0f4.

This test ensures that all the aggregate queries are actually the same no matter how you build them

Write tests to effectively document the toProto output of the various aggregate fields

Equivalent tests to count are written for sum in system tests and some more tests are written too to meet requirements outlined by team.

vicb · 2023-05-01T19:44:26Z

src/aggregate.ts

+   *
+   */
+  toProto(): any {
+    const aggregation = Object.assign(


Could you just

const aggregation = this.property_ ? {property: {name: this.property_}} : {};

vicb · 2023-05-01T19:47:29Z

src/aggregate.ts

+    return Object.assign(
+      {operator: this.operator},
+      this.alias_ ? {alias: this.alias_} : null,
+      aggregationObject


You can also drop the intermediate object and replace this line by

{[this.operator]: aggregation}

vicb · 2023-05-01T19:48:35Z

src/aggregate.ts

+   *
+   * @param {string} property
+   */
+  constructor(property: string) {


nit:

constructor(public property_: string) {

and remove the fields + the assignment

The test for sum and snapshot reads should look at the database before the data is created and run the tests based on the database in that state

Add a test block for a dataset with overflow values and a dataset with NaN values.

Aggregate field should be exported from the client so that it can be used easily by users.

…into sum-avg # Conflicts: # src/index.ts

Some idiomatic changes to improve the state of the code in the PR.

Values for sum and average should be different from those of count and these tests provide the right values now. # Conflicts: # system-test/datastore.ts

Average aggregations regarding appearances have been added in tests and correct values have been assigned

At this point the datastore is populated with data about ratings so computations should be done on that instead.

An assertion error should be thrown so that the test doesn’t pass if the request is successful.

The query with the limit will include all data points with the lowest appearance values. This is likely desired, but also important to document.

The rollbacks for the transaction tests ensure that if a test fails then the data gets reset to where it was before.

danieljbruce · 2023-08-16T21:12:08Z

src/request.ts

+      sharedQueryOpts.readOptions.readTime = {
+        seconds: Math.floor(seconds),
+      };
+    }


This code was taken from

nodejs-datastore/src/request.ts

Lines 290 to 299 in e7c3aaf

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.

any reason why we can't refactor this now?

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

kolea2 · 2023-08-17T15:06:53Z

src/request.ts

@@ -891,13 +891,26 @@ class DatastoreRequest {
    query: Query,
    options: RunQueryStreamOptions = {}
  ): SharedQueryOptions {
+    // TODO: This needs to be refactored later:


is this to be done in this PR or in a future one?

Same the above comment. We can do it now if preferred.

kolea2 · 2023-08-17T15:07:12Z

src/request.ts

+      sharedQueryOpts.readOptions.readTime = {
+        seconds: Math.floor(seconds),
+      };
+    }


any reason why we can't refactor this now?

kolea2 · 2023-08-17T15:08:12Z

system-test/datastore.ts

@@ -704,6 +710,13 @@ describe('Datastore', () => {
      assert.strictEqual(secondEntities!.length, 3);
    });

+    it('should query the datastore with snapshot read', async () => {


is this related to SUM/AVG changes? If not, let's move it into a separate PR

system-test/datastore.ts

danieljbruce · 2023-08-17T15:18:32Z

system-test/datastore.ts

+    after(async () => {
+      await datastore.delete(keys);
+    });
+    it('should run a sum aggregation', async () => {


Include overflow word here

Introducing getSharedOptionsOnly allows us to use that function in two different places to avoid a repeated block of code.

This test belongs inside another PR because it is not directly related to sum/avg.

A test should be included that looks at multiple aggregations in a single query.

The tests themselves should include the word overflow so that it is clear they are working with an overflow dataset.

src/aggregate.ts

kolea2 · 2023-08-25T15:41:58Z

src/aggregate.ts

+  static average(property: string): Average {
+    return new Average(property);
+  }
+
  /**
   * Gets a copy of the Count aggregate field.


should this be updated to remove "Count" since it applies to all?

This description needs to be updated.

kolea2 · 2023-08-25T15:42:25Z

src/aggregate.ts

  /**
   * Gets a copy of the Count aggregate field.
   *
   * @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 {
+  alias(alias?: string): AggregateField {


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

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

kolea2 · 2023-08-25T15:43:21Z

src/request.ts

-  private getSharedQueryOptions(
-    query: Query,
-    options: RunQueryStreamOptions = {}
+  private getSharedOptionsOnly(


what is the difference between this method and getSharedQueryOptions? The naming is confusing here.

getSharedQueryOptions also accepts a query and calls getSharedOptionsOnly. getSharedOptionsOnly is also called for fetches in createReadStream where options need to be built without considering a query.

Thanks for the explanation! Maybe getSharedQueryOptions and getSharedRequestOptions, or getQueryOptions and getRequestOptions would be a little more readable, wdyt?

getQueryOptions and getRequestOptions would work really well. The good thing about these two functions is that they are both private so we can change the names later too if need be.

system-test/datastore.ts

The comment for setting the alias should not make any mention of count since it is agnostic to the aggregation type.

No matter how alias is encoded, the data structures should store the aggregations the same way inside an aggregate field so as not to create any confusion.

Tests for when an empty alias is provided should check that each aggregate query still works.

The sleep function should enable us to test snapshot reads for aggregate queries

Two tests should be explored that evaluate what happens when multiple aggregations are used and when too many aggregations are used.

Shared options functions could be given a better name so that they make more sense to the code that is using them.

ruyadorno

A couple of questions and tweaking a comment but otherwise it's looking good to me 👍

src/index.ts

ruyadorno · 2023-08-29T20:01:17Z

system-test/datastore.ts

@@ -676,12 +677,18 @@ describe('Datastore', () => {
    ];

    before(async () => {
+      // This function is used for testing snapshot reads by getting a time and then sleeping before adding data.


I believe it would be important to rephrase this comment so that it explains why we need the sleeping:
is it simulating process time from datastore? is it simulating latency?

Done. I actually added some code in the latest commit to ensure the test won't be flakey. The intention of this was to be sure that a recorded read time occurred prior to the time of saving the data we wish to query against. This way a snapshot read at the recorded time would be sure to be against data prior to a save. This is now accomplished by reading data from the database, recording the read time, sleeping and then saving to ensure the save time on the server is later than the recorded read time. This will make sure the test isn't flakey.

The code must explain why the sleep function is needed in the test because its purpose should be clear.

A sum/average test uses snapshot reads for an aggregate query. The key here is write code that will guarantee the read time occurs well before all the data is saved in order to ensure the test isn’t flakey.

kolea2 · 2023-08-31T18:50:12Z

system-test/datastore.ts

-      timeBeforeDataCreation = Date.now();
-      await sleep(1000);
+      // Save for a key so that a read time can be accessed for snapshot reads.
+      const dummyData = Object.assign(Object.assign({}, keysToSave[0]), {


rename to "emptyData"

emptyData is a more accurate variable name for the datastore save data

kolea2

lgtm, please wait for @ruyadorno's approval as well before merging. Thanks for all the hard work on this!

ruyadorno

LGTM 👍

danieljbruce added 3 commits March 27, 2023 17:32

Initial sum aggregation

ce9413f

This commit introduces the sum aggregation with a simple test to ensure it works

Modify encoding

cadb0cc

This change modifies the encoding so that the right data reaches the grpc layer

PropertyAggregateField with tests

274c606

Adds helper functions and a super class so that average and sum can share the same properties.

product-auto-label bot added size: m Pull request size is medium. api: datastore Issues related to the googleapis/nodejs-datastore API. labels Mar 27, 2023

danieljbruce commented Apr 19, 2023

View reviewed changes

src/aggregate.ts Show resolved Hide resolved

danieljbruce added 9 commits April 27, 2023 17:32

Improve transaction tests

a87e277

Add sum and average to the transaction tests here to improve test coverage for transactions.

Change the description in the describe block

7548e81

Change return type to average

aaaff65

The return type for this function is wrong. The function returns an Average so we should use Average.

Make alias optional

02f0f7d

Make alias optional since the query still works without providing an alias.

Fix the transaction tests to fail on rollback

256a4d8

The transaction tests should not pass if the transaction is rolled back like they do currently. We must not catch errors and instead let the test fail.

Add additional assertions to existing tests

970c0f4

Ensure that the addAggregation function works correctly with an additional assertion check.

Revert "Add additional assertions to existing tests"

ba7aef8

This reverts commit 970c0f4.

Add describe block for comparing equivalent query

3a204f5

This test ensures that all the aggregate queries are actually the same no matter how you build them

Average, sum and count toProto tests

b403185

Write tests to effectively document the toProto output of the various aggregate fields

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Apr 28, 2023

Add tests for the sum aggregation

8312db7

Equivalent tests to count are written for sum in system tests and some more tests are written too to meet requirements outlined by team.

vicb reviewed May 1, 2023

View reviewed changes

danieljbruce added 5 commits May 1, 2023 15:56

Add a test for sum and snapshot reads

035df48

The test for sum and snapshot reads should look at the database before the data is created and run the tests based on the database in that state

Add two test blocks for special cases

6a44483

Add a test block for a dataset with overflow values and a dataset with NaN values.

Export aggregate field from the client

cfd745e

Aggregate field should be exported from the client so that it can be used easily by users.

Merge branch 'main' of https://github.com/googleapis/nodejs-datastore …

d74c158

…into sum-avg # Conflicts: # src/index.ts

PR follow-up changes

a67b34b

Some idiomatic changes to improve the state of the code in the PR.

danieljbruce changed the title ~~Sum and average aggregation queries~~ feat: Sum and average aggregation queries Jun 9, 2023

danieljbruce marked this pull request as ready for review June 9, 2023 18:40

danieljbruce requested review from a team as code owners June 9, 2023 18:40

danieljbruce added 2 commits August 11, 2023 10:22

Adjust the values so that tests pass

4d58133

Values for sum and average should be different from those of count and these tests provide the right values now. # Conflicts: # system-test/datastore.ts

Add average aggregations

e3b2c62

Average aggregations regarding appearances have been added in tests and correct values have been assigned

danieljbruce added 4 commits August 16, 2023 14:43

Do the test on rating instead of appearances

530df2c

At this point the datastore is populated with data about ratings so computations should be done on that instead.

The assertion says the request should have failed

eea8602

An assertion error should be thrown so that the test doesn’t pass if the request is successful.

Add a comment about using limits in test

8c4db96

The query with the limit will include all data points with the lowest appearance values. This is likely desired, but also important to document.

Add rollbacks to transaction tests

c7de99c

The rollbacks for the transaction tests ensure that if a test fails then the data gets reset to where it was before.

danieljbruce commented Aug 16, 2023

View reviewed changes

kolea2 reviewed Aug 17, 2023

View reviewed changes

danieljbruce commented Aug 17, 2023

View reviewed changes

danieljbruce and others added 5 commits August 18, 2023 10:06

refactor getSharedOptionsOnly

a57c4a8

Introducing getSharedOptionsOnly allows us to use that function in two different places to avoid a repeated block of code.

Remove test related to snapshot reads

8ee7061

This test belongs inside another PR because it is not directly related to sum/avg.

Add a test for multiple types of aggregates

4f01c43

A test should be included that looks at multiple aggregations in a single query.

Correct descriptions of two tests on overflow

5be7d69

The tests themselves should include the word overflow so that it is clear they are working with an overflow dataset.

Merge branch 'main' into sum-avg

0ed5509

kolea2 requested changes Aug 25, 2023

View reviewed changes

danieljbruce added 6 commits August 25, 2023 16:52

Add a comment for setting the alias

3b53859

The comment for setting the alias should not make any mention of count since it is agnostic to the aggregation type.

Add tests to compare various ways to encode alias

af55599

No matter how alias is encoded, the data structures should store the aggregations the same way inside an aggregate field so as not to create any confusion.

Added tests for when an empty alias is provided

98afdd6

Tests for when an empty alias is provided should check that each aggregate query still works.

Add a comment clarifying the use of snapshot reads

15840dd

The sleep function should enable us to test snapshot reads for aggregate queries

Add two tests to explore mixed aggregations alias

bbff19f

Two tests should be explored that evaluate what happens when multiple aggregations are used and when too many aggregations are used.

Better names for some internal private functions

2b66bae

Shared options functions could be given a better name so that they make more sense to the code that is using them.

ruyadorno reviewed Aug 29, 2023

View reviewed changes

danieljbruce added 2 commits August 30, 2023 15:44

Add a comment explaining why the sleep is needed

6d882a3

The code must explain why the sleep function is needed in the test because its purpose should be clear.

Add getReadTime function and use for sum/avg

f7a6873

A sum/average test uses snapshot reads for an aggregate query. The key here is write code that will guarantee the read time occurs well before all the data is saved in order to ensure the test isn’t flakey.

kolea2 requested changes Aug 31, 2023

View reviewed changes

Rename variable to emptyData

3681c06

emptyData is a more accurate variable name for the datastore save data

kolea2 approved these changes Sep 1, 2023

View reviewed changes

danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 1, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Sep 1, 2023

danieljbruce merged commit 44ba6f8 into googleapis:main Sep 1, 2023
15 checks passed

release-please bot mentioned this pull request Sep 1, 2023

chore(main): release 8.1.0 #1145

Merged

ruyadorno reviewed Sep 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Sum and average aggregation queries #1097

feat: Sum and average aggregation queries #1097

danieljbruce commented Mar 27, 2023

vicb May 1, 2023

danieljbruce Aug 11, 2023

vicb May 1, 2023

danieljbruce Aug 11, 2023

vicb May 1, 2023

danieljbruce Aug 11, 2023

danieljbruce Aug 16, 2023

kolea2 Aug 17, 2023

danieljbruce Aug 17, 2023

kolea2 Aug 17, 2023

danieljbruce Aug 17, 2023

kolea2 Aug 17, 2023

kolea2 Aug 17, 2023

danieljbruce Aug 17, 2023

danieljbruce Aug 17, 2023

danieljbruce Aug 18, 2023

kolea2 Aug 25, 2023

danieljbruce Aug 25, 2023

danieljbruce Aug 25, 2023

kolea2 Aug 25, 2023

danieljbruce Aug 28, 2023

kolea2 Aug 25, 2023

danieljbruce Aug 25, 2023

kolea2 Aug 28, 2023

danieljbruce Aug 28, 2023

ruyadorno left a comment

ruyadorno Aug 29, 2023

danieljbruce Aug 31, 2023

kolea2 Aug 31, 2023

danieljbruce Aug 31, 2023

kolea2 left a comment

ruyadorno left a comment

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

feat: Sum and average aggregation queries #1097

feat: Sum and average aggregation queries #1097

Conversation

danieljbruce commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruyadorno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolea2 left a comment

Choose a reason for hiding this comment

ruyadorno left a comment

Choose a reason for hiding this comment