Optimize Duplications of `ComputedField` queries #126

fisehara · 2023-04-26T10:23:57Z

For each modelName the a ComputedField is only compiled into abstract-sql-query once. Aftewards it's used as ReferencedField

Change-type: minor

Page- · 2023-05-12T11:09:04Z

src/odata-to-abstract-sql.ts

@@ -117,6 +117,8 @@ type Overwrite<T, U> = Pick<T, Exclude<keyof T, keyof U>> & U;
 type RequiredField<T, F extends keyof T> = Overwrite<T, Required<Pick<T, F>>>;
 type AliasedResource = RequiredField<Resource, 'tableAlias'>;

+type AlreadyComputedFieldsLookup = { [key: string]: boolean };


You can name key whatever so in cases like this it can help a lot to use a more descriptive name, eg

Suggested change

type AlreadyComputedFieldsLookup = { [key: string]: boolean };

type AlreadyComputedFieldsLookup = { [fieldName: string]: boolean };

I see, thanks!

Page- · 2023-05-12T13:01:07Z

src/odata-to-abstract-sql.ts

@@ -991,11 +1004,16 @@ export class OData2AbstractSQL {
 		fieldName: string,
 		computed?: AbstractSqlQuery,
 		alias: string = fieldName,
+		alwaysCompileComputed: boolean = false,


Is this flag ever used?

Yes, it's used in the case, where the actual computed fields a generated here:
https://github.com/balena-io-modules/odata-to-abstract-sql/blob/931e1e021310bc9788dd75bcc94da8bd06cdf789/src/odata-to-abstract-sql.ts#LL1713C1-L1719C9

Ah, I didn't see it because the fn name isn't part of the diff

Page- · 2023-05-12T13:01:45Z

src/odata-to-abstract-sql.ts

+		resource.fields.some(
+			(f) =>
+				f.computed != null &&
+				!alreadyComputedFieldsLookup[resource.name + f.fieldName],


It's good to add some form of separator to further reduce the chances of accidental clashes, eg

Suggested change

!alreadyComputedFieldsLookup[resource.name + f.fieldName],

!alreadyComputedFieldsLookup[resource.name + '$' + f.fieldName],

Page- · 2023-05-12T13:51:14Z

src/odata-to-abstract-sql.ts

+		resource.fields.some(
+			(f) =>
+				f.computed != null &&
+				!alreadyComputedFieldsLookup[resource.name + f.fieldName],


It's good to add some form of separator to further reduce the chances of accidental clashes, eg

Suggested change

!alreadyComputedFieldsLookup[resource.name + f.fieldName],

!alreadyComputedFieldsLookup[resource.name + '$' + f.fieldName],

Page- · 2023-06-26T14:02:18Z

src/odata-to-abstract-sql.ts

@@ -991,11 +1004,16 @@ export class OData2AbstractSQL {
 		fieldName: string,
 		computed?: AbstractSqlQuery,
 		alias: string = fieldName,
+		alwaysCompileComputed: boolean = false,


Ah, I didn't see it because the fn name isn't part of the diff

Page- · 2023-06-26T14:03:00Z

src/odata-to-abstract-sql.ts

@@ -991,11 +1004,16 @@ export class OData2AbstractSQL {
 		fieldName: string,
 		computed?: AbstractSqlQuery,
 		alias: string = fieldName,
+		alwaysCompileComputed: boolean = false,


Can you add a comment giving a brief/basic explanation of what the flag is for so that future people looking at the code have a starting point for understanding it please

Added a comment and found a better flag name

Page-

A bunch of my previous review comments still stand, and also I'd like to see the version bumps as a separate PR so they can be merged separately based upon whichever part is approved first

Page- · 2023-10-17T13:58:07Z

tsconfig.json

@@ -15,7 +15,7 @@
 		"allowJs": true
 	},
 	"include": [
-		"src/**/*"
+		"src/**/*",


This is invalid json

Page- · 2023-10-17T15:26:22Z

.eslintrc.cjs

+		sourceType: 'module',
+	},
+	env: {
+		jest: true,


We don't use jest?

Page- · 2023-10-17T15:29:18Z

And I've just seen it is a separate PR.. I've converted this to draft until #137 is merged as it depends on that

fisehara · 2023-11-17T12:22:14Z

@Page- Sorry for the confusion, I didn't push the changes when I commented on the remarks. Thanks for the review again.

For each modelName the a `ComputedField` is only compiled into abstract-sql-query once. Afterwards it's used as ReferencedField Change-type: minor Signed-off-by: fisehara <[email protected]>

fisehara · 2023-11-24T17:36:39Z

LGTM

flowzone-app bot enabled auto-merge April 26, 2023 10:24

fisehara force-pushed the fisehara/optimize-computed-fields branch 6 times, most recently from 870744d to 931e1e0 Compare May 3, 2023 10:51

Page- reviewed May 12, 2023

View reviewed changes

fisehara force-pushed the fisehara/optimize-computed-fields branch from 931e1e0 to 9bb741c Compare May 16, 2023 09:18

fisehara requested a review from Page- May 16, 2023 11:39

Page- reviewed Jun 26, 2023

View reviewed changes

fisehara force-pushed the fisehara/optimize-computed-fields branch from 9bb741c to 0229bd5 Compare June 28, 2023 15:33

fisehara requested a review from Page- June 28, 2023 15:34

fisehara force-pushed the fisehara/optimize-computed-fields branch 2 times, most recently from 0d5403f to 15e6a0d Compare October 17, 2023 11:32

Page- reviewed Oct 17, 2023

View reviewed changes

Page- marked this pull request as draft October 17, 2023 15:28

auto-merge was automatically disabled October 17, 2023 15:28
Pull request was converted to draft

fisehara force-pushed the fisehara/optimize-computed-fields branch from 15e6a0d to 6f3293d Compare November 17, 2023 12:21

fisehara requested a review from Page- November 17, 2023 12:22

fisehara marked this pull request as ready for review November 17, 2023 12:24

Optimize Duplications of ComputedField queries

87c73d4

For each modelName the a `ComputedField` is only compiled into abstract-sql-query once. Afterwards it's used as ReferencedField Change-type: minor Signed-off-by: fisehara <[email protected]>

fisehara force-pushed the fisehara/optimize-computed-fields branch from 6f3293d to 87c73d4 Compare November 17, 2023 12:25

flowzone-app bot enabled auto-merge November 17, 2023 12:27

flowzone-app bot merged commit c7c9679 into master Nov 24, 2023
48 checks passed

flowzone-app bot deleted the fisehara/optimize-computed-fields branch November 24, 2023 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize Duplications of `ComputedField` queries #126

Optimize Duplications of `ComputedField` queries #126

fisehara commented Apr 26, 2023

Page- May 12, 2023

fisehara May 16, 2023

Page- May 12, 2023

fisehara May 16, 2023

Page- Jun 26, 2023

Page- May 12, 2023

Page- May 12, 2023

Page- Jun 26, 2023

Page- Jun 26, 2023

fisehara Jun 28, 2023

Page- left a comment

Page- Oct 17, 2023

Page- Oct 17, 2023

Page- commented Oct 17, 2023

fisehara commented Nov 17, 2023

fisehara commented Nov 24, 2023

	type AlreadyComputedFieldsLookup = { [key: string]: boolean };
	type AlreadyComputedFieldsLookup = { [fieldName: string]: boolean };

	!alreadyComputedFieldsLookup[resource.name + f.fieldName],
	!alreadyComputedFieldsLookup[resource.name + '$' + f.fieldName],

Optimize Duplications of ComputedField queries #126

Optimize Duplications of ComputedField queries #126

Conversation

fisehara commented Apr 26, 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

Page- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Page- commented Oct 17, 2023

fisehara commented Nov 17, 2023

fisehara commented Nov 24, 2023

Optimize Duplications of `ComputedField` queries #126

Optimize Duplications of `ComputedField` queries #126