Aggregated queries #1 #8345

ijreilly · 2024-11-05T15:49:02Z

First step of #6868

Adds min.., max.. queries for DATETIME fields
adds min.., max.., avg.., sum.. queries for NUMBER fields

(count distinct operation and composite fields such as CURRENCY handling will be dealt with in a future PR)

...graphql/workspace-schema-builder/utils/get-available-aggregations-from-object-fields.util.ts

ijreilly · 2024-11-06T11:43:11Z

packages/twenty-front/tsconfig.build.json

@@ -18,5 +18,5 @@
    "src/**/*.d.ts",
    "src/**/*.ts",
    "src/**/*.tsx"
-  ]
+, "../twenty-server/src/utils/is-uuid.ts", "../twenty-server/src/utils/is-uuid.utils.spec.ts"  ]


Commited this although I suppose it should not be this way. I suppose something is wrong with my test file

Yes this line seems strange, why would you add this? Sorry I don't understand the goal

...runner/graphql-query-parsers/graphql-query-selected-fields/graphql-selected-fields.parser.ts

...graphql/workspace-schema-builder/utils/get-available-aggregations-from-object-fields.util.ts

ijreilly · 2024-11-06T11:50:49Z

Question - do we want the operations to be computed on all of the records (as we do for total counts), or on the records matching the potential filters in the query?
@Bonapara @FelixMalfait

greptile-apps

PR Summary

This PR adds initial support for aggregate queries in the GraphQL API, implementing min/max operations for datetime fields and min/max/avg/sum for number fields.

Added getAvailableAggregationsFromObjectFields utility to dynamically generate aggregation fields based on field types
Modified GraphqlQueryFindManyResolverService to handle aggregated field queries using SQL OVER() clause
Added UUID validation utility to properly filter UUID fields during aggregation processing
Updated ConnectionTypeDefinitionFactory to include aggregation fields in GraphQL schema
Potential issue: AggregationValue type incorrectly uses GraphQLString instead of union type for different field types

_{10 file(s) reviewed, 15 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-11-06T11:50:06Z

packages/twenty-front/tsconfig.build.json

@@ -18,5 +18,5 @@
    "src/**/*.d.ts",
    "src/**/*.ts",
    "src/**/*.tsx"
-  ]
+, "../twenty-server/src/utils/is-uuid.ts", "../twenty-server/src/utils/is-uuid.utils.spec.ts"  ]


logic: Server-side files should not be included in front-end build config. Remove these files from the include array.

greptile-apps · 2024-11-06T11:50:06Z

packages/twenty-front/tsconfig.build.json

@@ -18,5 +18,5 @@
    "src/**/*.d.ts",
    "src/**/*.ts",
    "src/**/*.tsx"
-  ]
+, "../twenty-server/src/utils/is-uuid.ts", "../twenty-server/src/utils/is-uuid.utils.spec.ts"  ]


logic: Test files (*.spec.ts) should be excluded as per line 8, but are being included here

greptile-apps · 2024-11-06T11:50:27Z

packages/twenty-front/tsconfig.dev.json

@@ -9,5 +9,5 @@
    "src/**/*.d.ts",
    "src/**/*.ts",
    "src/**/*.tsx"
-  ]
+, "../twenty-server/src/utils/is-uuid.ts", "../twenty-server/src/utils/is-uuid.utils.spec.ts"  ]


logic: Server files should not be included in frontend tsconfig. Remove '../twenty-server/src/utils/is-uuid.ts' and '../twenty-server/src/utils/is-uuid.utils.spec.ts'

Remove also, no?

greptile-apps · 2024-11-06T11:50:48Z

packages/twenty-front/tsconfig.spec.json

@@ -15,5 +15,5 @@
    "tsup.config.ts",
    "tsup.ui.index.tsx",
    "vite.config.ts"
-  ]
+, "../twenty-server/src/utils/is-uuid.utils.spec.ts"  ]


logic: Remove this line - server test files should not be included in front-end test config

Remove also, no?

greptile-apps · 2024-11-06T11:51:16Z

...runner/graphql-query-parsers/graphql-query-selected-fields/graphql-selected-fields.parser.ts

+      if (hasEdges && fieldKey !== 'edges') {
+        continue;
+      }


logic: skipping non-edges fields when edges exist could prevent aggregation fields from being processed at the root level

greptile-apps · 2024-11-06T11:53:09Z

...graphql/workspace-schema-builder/utils/get-available-aggregations-from-object-fields.util.ts

+type AggregationValue = {
+  type: typeof GraphQLString;
+  description: string;
+  fromField: string;
+  aggregationOperation: AGGREGATION_OPERATIONS;
+};


logic: AggregationValue.type is incorrectly typed as GraphQLString when it needs to handle both GraphQLISODateTime and GraphQLFloat

...graphql/workspace-schema-builder/utils/get-available-aggregations-from-object-fields.util.ts

greptile-apps · 2024-11-06T11:53:35Z

packages/twenty-server/src/utils/is-uuid.ts

+  const uuidRegex =
+    /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;


style: Consider caching this regex pattern outside the function since it's constant and regex compilation is expensive

greptile-apps · 2024-11-06T11:53:36Z

packages/twenty-server/src/utils/is-uuid.ts

@@ -0,0 +1,14 @@
+export const isUuid = (value: unknown): boolean => {


style: Function lacks JSDoc documentation explaining its purpose and expected input/output

greptile-apps · 2024-11-06T11:54:01Z

packages/twenty-server/src/utils/is-uuid.utils.spec.ts

+
+    it('should return false for similar looking strings', () => {
+      expect(isUuid('xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx')).toBe(false);
+      expect(isUuid('00000000-0000-0000-0000-000000000000')).toBe(true); // valid but all zeros


logic: test case for all-zeros UUID returns true, but this may be a security risk if used for validation in sensitive contexts

FelixMalfait · 2024-11-06T12:26:31Z

@ijreilly I'm surprised that totalCount returns the count without applying the filters. How do we display the totalCount of a view for example? My intuition was that filter should be applied everywhere

ijreilly · 2024-11-06T13:02:33Z

@FelixMalfait my bad - both totalCount and aggregated queries take the filter into account. all good !

FelixMalfait · 2024-11-06T13:04:41Z

...ngine/api/graphql/graphql-query-runner/resolvers/graphql-query-find-many-resolver.service.ts

+      objectFields: Object.values(
+        Object.fromEntries(
+          Object.entries(objectMetadataMapItem.fields).filter(
+            ([key, _value]) => !isUuid(key), // remove objectMetadataMapItem fields duplicates


This feels a bit ugly, did you consider solving the root cause in generateObjectMetadataMap directly, introducing 2 maps instead of 1? I know it might be a largr refactor with many downstream consequences but it's probably better than this uuid hack which we'll have to pay back one day?
Note: We'll need to make sure to clear all the metadata cache when we deploy if we go with the "clean" way

When I say 2 objects it's 2 different map for object level but also have 2 properties instead of one (for the field map by id + field map by name) within the ObjectMetadataMapItem

I am not sure how this map with ids as key are used today, I just know/think its for perf improvement reasons. @charlesBochet has more context and probably knows better if a refactor is desirable i think !
I think you understood that from the review but to give context, I introduced the filter here not to duplicate the aggregated fields computed (otherwise we would find "minCreatedAt" twice for instance), which we could also do differently not to allow a duplicated value in the selectedAggregatedFields array, which I had done initially and is maybe less ugly (but still a bit hacky)!

Yes I understand! Thank you. But we should introduce two maps on byName and one byId. It doesn't make sense to mix both and lose information, to then re-create it later

FelixMalfait · 2024-11-06T14:31:33Z

...runner/graphql-query-parsers/graphql-query-selected-fields/graphql-selected-fields.parser.ts

@@ -83,9 +89,7 @@ export class GraphqlQuerySelectedFieldsParser {
  }

  private shouldNotParseField(fieldKey: string): boolean {
-    return ['__typename', 'totalCount', 'pageInfo', 'cursor'].includes(


Should we replace totalCount everywhere in the app by countUniqueId? We'll need to give a heads up for deprecation but we can start replacing it in our frontend at least. We plan to email people about API Key deprecation so we can let them know about this at the same time.
If I'm not mistaken we're doing a separate query for totalCount now while this could fit in the same query with all other aggregated fields?

ijreilly · 2024-11-08T11:57:22Z

Follow-ups to this PR

Refactor fields maps generated by generateObjectMetadataMap to differentiate between map with field names as keys and map with ids as keys (@FelixMalfait I know you would have wanted it to be integrated to this PR but im trying to close things before leaving on holidays today, @charlesBochet agreed it could wait - I did change the way the duplicates in fieldMetadataMap are handled though)
Replace totalCount with countUniqueId and compute it in the same query as other fields (and aggregated fields)
Implement aggregation fields for all other field types
Use aggregated queries in the FE (they are only exposed in the api with this PR)

Other ideas @charlesBochet @FelixMalfait ?

FelixMalfait · 2024-11-08T14:50:47Z

Great @ijreilly looking forward to having all this!

charlesBochet · 2024-11-08T15:23:16Z

.../engine/api/graphql/workspace-schema-builder/factories/connection-type-definition.factory.ts

-    const fields: GraphQLFieldConfigMap<any, any> = {};
+    const fields: GraphQLFieldConfigMap<any, any> = Object.assign(
+      {},
+      ...getAvailableAggregationsFromObjectFields(objectMetadata.fields).map(


I think we should follow the existing factory pattern here:
this.connectionTypeFactory.create(...) see lines below

charlesBochet · 2024-11-08T15:24:17Z

...graphql/workspace-schema-builder/utils/get-available-aggregations-from-object-fields.util.ts

+}
+
+type AggregationValue = {
+  type: typeof GraphQLString;


not a big fan of typeOf! why not directly putting the right one?

charlesBochet · 2024-11-11T16:10:50Z

Todo:

check hasEdges
Use feature flag
Take relations into consideration
Handle composite amount field
Test integration

Weiko

LGTM, well done @ijreilly !!
I agree with Charles comments and will try to address them.

...runner/graphql-query-parsers/graphql-query-selected-fields/graphql-selected-fields.parser.ts

…ginated results

First step of twentyhq#6868 Adds min.., max.. queries for DATETIME fields adds min.., max.., avg.., sum.. queries for NUMBER fields (count distinct operation and composite fields such as CURRENCY handling will be dealt with in a future PR) <img width="1422" alt="Capture d’écran 2024-11-06 à 15 48 46" src="https://github.com/user-attachments/assets/4bcdece0-ad3e-4536-9720-fe4044a36719"> --------- Co-authored-by: Charles Bochet <charles@twenty.com> Co-authored-by: Weiko <corentin@twenty.com>

wip

4b51b58

ijreilly commented Nov 5, 2024

View reviewed changes

...graphql/workspace-schema-builder/utils/get-available-aggregations-from-object-fields.util.ts Outdated Show resolved Hide resolved

charlesBochet added -PR: awaiting author -PR: wip labels Nov 5, 2024

ijreilly changed the title ~~wip~~ Aggregated queries - wip Nov 6, 2024

ijreilly added 3 commits November 6, 2024 15:19

Refactor code

3f929c9

Add test for isUuid util

49bc25a

fix selectedFields and add number fields aggregations

10fdfd5

ijreilly commented Nov 6, 2024

View reviewed changes

...runner/graphql-query-parsers/graphql-query-selected-fields/graphql-selected-fields.parser.ts Outdated Show resolved Hide resolved

ijreilly commented Nov 6, 2024

View reviewed changes

...graphql/workspace-schema-builder/utils/get-available-aggregations-from-object-fields.util.ts Show resolved Hide resolved

minor fixes

6b3e533

ijreilly marked this pull request as ready for review November 6, 2024 11:49

ijreilly changed the title ~~Aggregated queries - wip~~ Aggregated queries #1 Nov 6, 2024

ijreilly added -PR: awaiting review and removed -PR: wip -PR: awaiting author labels Nov 6, 2024

greptile-apps bot reviewed Nov 6, 2024

View reviewed changes

FelixMalfait reviewed Nov 6, 2024

View reviewed changes

FelixMalfait mentioned this pull request Nov 6, 2024

Update total count on record deletion #8366

Closed

ijreilly added 2 commits November 8, 2024 15:50

Change way to filter out duplicates of fieldMetadataMap

3fb0fe0

Revert changes to tsconfig files

7adb8ce

charlesBochet reviewed Nov 8, 2024

View reviewed changes

charlesBochet added 10 commits November 11, 2024 14:44

Fix

52e8a88

Fix

55ce64d

Fix

f8d4c2a

Fix

5af68f8

Fix

db02867

Add feature flag

2c6549a

Refacto maps

b347f39

Fix

6ca4628

Fix

7c7ee44

Fix

22d0fb9

charlesBochet force-pushed the aggregate-queries-1 branch from df11294 to 22d0fb9 Compare November 11, 2024 15:58

charlesBochet added 2 commits November 11, 2024 17:00

Fix

b182252

Fix

2d7ff9c

charlesBochet approved these changes Nov 11, 2024

View reviewed changes

charlesBochet added 3 commits November 11, 2024 18:12

Fixes

fce5c81

Remove unused date time scalar

c1df0e5

Fix tests

765d9a6

charlesBochet assigned Weiko Nov 12, 2024

Merge branch 'main' into aggregate-queries-1

e5a9e3d

Weiko approved these changes Nov 12, 2024

View reviewed changes

...runner/graphql-query-parsers/graphql-query-selected-fields/graphql-selected-fields.parser.ts Outdated Show resolved Hide resolved

charlesBochet added -PR: awaiting author and removed -PR: awaiting review labels Nov 13, 2024

Add aggregation to nested queries + totalCount + fix aggregate for pa…

b44ceb9

…ginated results

charlesBochet approved these changes Nov 14, 2024

View reviewed changes

Weiko added 2 commits November 14, 2024 17:20

fix

1aee63c

fix tests

853126a

Weiko merged commit a799370 into main Nov 14, 2024
16 checks passed

Weiko deleted the aggregate-queries-1 branch November 14, 2024 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aggregated queries #1 #8345

Aggregated queries #1 #8345

ijreilly commented Nov 5, 2024 •

edited

Loading

ijreilly Nov 6, 2024

FelixMalfait Nov 6, 2024

ijreilly commented Nov 6, 2024

greptile-apps bot left a comment

greptile-apps bot Nov 6, 2024

greptile-apps bot Nov 6, 2024

greptile-apps bot Nov 6, 2024

FelixMalfait Nov 6, 2024

greptile-apps bot Nov 6, 2024

FelixMalfait Nov 6, 2024

greptile-apps bot Nov 6, 2024

greptile-apps bot Nov 6, 2024

greptile-apps bot Nov 6, 2024

greptile-apps bot Nov 6, 2024

greptile-apps bot Nov 6, 2024

FelixMalfait commented Nov 6, 2024

ijreilly commented Nov 6, 2024

FelixMalfait Nov 6, 2024

FelixMalfait Nov 6, 2024

ijreilly Nov 6, 2024 •

edited

Loading

FelixMalfait Nov 6, 2024

FelixMalfait Nov 6, 2024

ijreilly commented Nov 8, 2024

FelixMalfait commented Nov 8, 2024

charlesBochet Nov 8, 2024

charlesBochet Nov 8, 2024

charlesBochet commented Nov 11, 2024 •

edited

Loading

Weiko left a comment

		const uuidRegex =
		/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;

		@@ -0,0 +1,14 @@
		export const isUuid = (value: unknown): boolean => {

Aggregated queries #1 #8345

Aggregated queries #1 #8345

Conversation

ijreilly commented Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijreilly commented Nov 6, 2024

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Nov 6, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 6, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greptile-apps bot Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greptile-apps bot Nov 6, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 6, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 6, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 6, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 6, 2024

Choose a reason for hiding this comment

FelixMalfait commented Nov 6, 2024

ijreilly commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijreilly Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijreilly commented Nov 8, 2024

FelixMalfait commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesBochet commented Nov 11, 2024 • edited Loading

Weiko left a comment

Choose a reason for hiding this comment

ijreilly commented Nov 5, 2024 •

edited

Loading

ijreilly Nov 6, 2024 •

edited

Loading

charlesBochet commented Nov 11, 2024 •

edited

Loading