Add support for TDS v2 (relation) groupBy in QueryBuilder #3892

travisstebbins · 2025-02-12T22:05:45Z

Summary

Add support for using the new TDS v2 (relation) protocol with groupBy expressions. This includes

Supporting converting the QueryBuilder state to the new protocol when the user has groupBy functions (i.e., count, sum, etc.)
Supporting converting the new protocol with groupBy functions to QueryBuilder state

How did you test this change?

Test(s) added
Manual testing (please provide screenshots/recordings)
No testing (please provide an explanation)

Group by:

Group by with multiple columns:

Group by with pre-filter:

Group by with post-filter:

Group by with pre-filter and post-filter:

…gation state

…ling nested properties

changeset-bot · 2025-02-12T22:05:49Z

🦋 Changeset detected

Latest commit: 495c377

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 31 packages

Name	Type
@finos/legend-graph	Patch
@finos/legend-query-builder	Patch
@finos/legend-application-data-cube	Patch
@finos/legend-application-pure-ide	Patch
@finos/legend-application-query-bootstrap	Patch
@finos/legend-application-query	Patch
@finos/legend-application-repl	Patch
@finos/legend-application-studio-bootstrap	Patch
@finos/legend-application-studio	Patch
@finos/legend-code-editor	Patch
@finos/legend-data-cube	Patch
@finos/legend-extension-assortment	Patch
@finos/legend-extension-dsl-data-quality	Patch
@finos/legend-extension-dsl-data-space-studio	Patch
@finos/legend-extension-dsl-data-space	Patch
@finos/legend-extension-dsl-diagram	Patch
@finos/legend-extension-dsl-persistence	Patch
@finos/legend-extension-dsl-service	Patch
@finos/legend-extension-dsl-text	Patch
@finos/legend-extension-store-flat-data	Patch
@finos/legend-extension-store-relational	Patch
@finos/legend-extension-store-service-store	Patch
@finos/legend-lego	Patch
@finos/legend-vscode-extension-dependencies	Patch
@finos/legend-application-data-cube-bootstrap	Patch
@finos/legend-application-pure-ide-deployment	Patch
@finos/legend-application-query-deployment	Patch
@finos/legend-application-repl-deployment	Patch
@finos/legend-application-studio-deployment	Patch
@finos/legend-application-data-cube-deployment	Patch
@finos/legend-server-showcase-deployment	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codecov · 2025-02-12T22:21:05Z

Codecov Report

Attention: Patch coverage is 91.07612% with 68 lines in your changes missing coverage. Please review.

Project coverage is 46.23%. Comparing base (82ea08b) to head (495c377).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
...pureGraph/from/V1_ValueSpecificationTransformer.ts	0.00%	22 Missing ⚠️
...ery-builder/src/stores/QueryBuilderStateBuilder.ts	67.50%	13 Missing ⚠️
...gation/QueryBuilderTypedAggregationStateBuilder.ts	95.65%	11 Missing ⚠️
...pure/v1/V1_QueryValueSpecificationBuilderHelper.ts	96.41%	8 Missing ⚠️
...QueryBuilderProjectionValueSpecificationBuilder.ts	54.54%	5 Missing ⚠️
...QueryBuilderRelationAggregationValueSpecBuilder.ts	97.95%	4 Missing ⚠️
...tion/changeDetection/ValueSpecificationObserver.ts	0.00%	3 Missing ⚠️
...e/valueSpecification/RelationValueSpecification.ts	0.00%	1 Missing ⚠️
...jection/QueryBuilderTypedProjectionStateBuilder.ts	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3892      +/-   ##
==========================================
- Coverage   46.41%   46.23%   -0.18%     
==========================================
  Files        2255     2264       +9     
  Lines      394495   397131    +2636     
  Branches    11894    17236    +5342     
==========================================
+ Hits       183086   183612     +526     
- Misses     210743   212529    +1786     
- Partials      666      990     +324

Files with missing lines	Coverage Δ
...l/pure/QueryBuilder_PureProtocolProcessorPlugin.ts	`87.84% <100.00%> (ø)`
...ry-builder/src/graph/QueryBuilderMetaModelConst.ts	`100.00% <100.00%> (ø)`
...ion/QueryBuilderRelationProjectValueSpecBuilder.ts	`81.95% <ø> (ø)`
...e/valueSpecification/RelationValueSpecification.ts	`39.53% <0.00%> (-0.47%)`	⬇️
...jection/QueryBuilderTypedProjectionStateBuilder.ts	`99.01% <87.50%> (+0.96%)`	⬆️
...tion/changeDetection/ValueSpecificationObserver.ts	`0.00% <0.00%> (ø)`
...QueryBuilderRelationAggregationValueSpecBuilder.ts	`97.95% <97.95%> (ø)`
...QueryBuilderProjectionValueSpecificationBuilder.ts	`86.60% <54.54%> (-0.69%)`	⬇️
...pure/v1/V1_QueryValueSpecificationBuilderHelper.ts	`95.09% <96.41%> (+0.20%)`	⬆️
...gation/QueryBuilderTypedAggregationStateBuilder.ts	`95.65% <95.65%> (ø)`
... and 2 more

... and 196 files with indirect coverage changes

…nExpression

… operators

…xpression

…ateBuilder functions

akphi

Nice job! The tests and general flow are very comprehensive.

I left some minor comments, mostly on some edge cases handling and code organization.

.changeset/light-icons-sparkle.md

akphi · 2025-02-16T22:52:30Z

packages/legend-query-builder/src/graph-manager/helpers/QueryBuilderAggregateOperatorHelper.ts

+} from '@finos/legend-graph';
+import { QUERY_BUILDER_SUPPORTED_FUNCTIONS } from '../../graph/QueryBuilderMetaModelConst.js';
+
+export const getNumericAggregateOperatorReturnType = (


Let's move away from having methods like this:

It doesn't really save you much time code writing, we don't use this anywhere else other than within the operator

The operator code is supposed to be modular, the current flow favors organizing the handling logic for the operator within the same file as the operator rather than dispersing them in classifier method like this ~~ think about why we don't have a giant method with big switch cases to handle all operators but have each file for one operator, the same logic holds true in that case

Makes sense! I removed the function and also removed us trying to manually get the column's return type when we convert from protocol to a SimpleFunctionExpression. Instead, we now update the groupBy() expression's return type when we process the graph here: https://github.com/finos/legend-studio/pull/3892/files#diff-2e5035c76b375d2a795ff0ba9a702405c2dec6dc4e0aabc1d115c63e659db6e3R144

...-query-builder/src/graph-manager/protocol/pure/v1/V1_QueryValueSpecificationBuilderHelper.ts

akphi · 2025-02-16T22:57:28Z

...nd-query-builder/src/graph-manager/protocol/pure/QueryBuilder_PureProtocolProcessorPlugin.ts

            QUERY_BUILDER_SUPPORTED_FUNCTIONS.TDS_GROUP_BY,
-          )
+            QUERY_BUILDER_SUPPORTED_FUNCTIONS.RELATION_GROUP_BY,
+          ])
        ) {
          return V1_buildGroupByFunctionExpression(


So an edge case, let's pass the functionName to V1_buildGroupByFunctionExpression
When the functionName is fully specified, i.e. meta::pure::tds::groupBy or meta::pure::functions::relation::groupBy we also know exactly what to handle

Makes sense. functionName was already passed to V1_buildGroupByFunctionExpression, but now in V1_buildGroupByFunctionExpression I check if functionName === QUERY_BUILDER_SUPPORTED_FUNCTIONS.RELATION_GROUP_BY and call V1_buildTypedGroupByFunctionExpression if that is true.

...rc/stores/fetch-structure/tds/aggregation/operators/QueryBuilderAggregateOperator_Average.ts

akphi · 2025-02-16T23:01:19Z

packages/legend-query-builder/src/stores/QueryBuilderStateBuilder.ts

-        this.queryBuilderState,
-        this.parentLambda,
-      );
+      if (isTypedGroupByExpression(valueSpecification)) {


same comment here, we should be careful of the case where people specify the fully specified function path

Makes sense. I updated isTypedGroupByExpression to first check if expression.functionName === QUERY_BUILDER_SUPPORTED_FUNCTIONS.RELATION_GROUP_BY. I also updated the isTypedProjectionExpression function to do the same.

…rator classes

…unctionExpression

…tion name

…n name

…tionColSpec function

travisstebbins added 6 commits February 12, 2025 11:05

feat: Add roundtrip test for new groupBy

bcb23c6

feat: Ensure ColSpec function2 is serialized

225c9bb

fix: Fix typo in QueryBuilderRelationProjectValueSpecBuilder

2aafb6c

feat: Handle converting query builder state to typed groupBy protocol

41dcea4

feat: Start adding support to convert from relation protocol to aggre…

8a5d954

…gation state

feat: Support converting TDSv2 groupBy protocol to state and fix hand…

b7fdc74

…ling nested properties

travisstebbins added 5 commits February 12, 2025 17:08

fix: Fix test expected value

645709b

fix: Remove console.log statements

77be33a

fix: Remove commented out code

47e8bf2

fix: Fix copyright year

7a9c658

fix: Fix missing match function name

063accf

travisstebbins added 16 commits February 13, 2025 10:39

fix: Add changeset file

ee89b4f

fix: Fix lint and copyright issues

77feff8

feat: Add additional test cases to QueryRoundtripGrammar tests

a5381f0

refactor: Clean up V1_buildTypedGroupByFunctionExpression

a664044

feat: Remove unused set variable

a38f493

fix: Tmp fix of expression genericType in V1_buildTypedGroupByFunctio…

7e4aa0d

…nExpression

feat: Add getNumericAggregateOperatorReturnType helper function

afd03a5

feat: Fix setting return type in V1_buildTypedGroupByFunctionExpression

01de070

feat: Use getNumericAggregateOperatorReturnType function in aggregate…

3efed95

… operators

feat: Clean up code in V1_QueryValueSpecificationBuilderHelper

6931ce4

feat: Clean up and add comments to buildRelationAggregation function

d93bcf0

feat: Remove options param from buildRelationAggregation function

b8d3528

refactor: Update some variable names in V1_buildTypedGroupByFunctionE…

b4d5f68

…xpression

refactor: Clean up and add comments to QueryBuilderTypedAggregationSt…

e08161d

…ateBuilder functions

feat: Add QueryBuilderLambdaRoundtrip test cases

672492a

fix: Remove extra param in function call

57f7204

travisstebbins marked this pull request as ready for review February 14, 2025 19:52

travisstebbins requested a review from a team as a code owner February 14, 2025 19:52

akphi reviewed Feb 16, 2025

View reviewed changes

travisstebbins added 9 commits February 18, 2025 09:50

fix: Update changeset files

5d9a390

feat: Don't use getNumericAggregateOperatorReturnType function in ope…

7bbbbc2

…rator classes

refactor: Update error message

4294d32

refactor: Check for fully qualified function name in V1_buildGroupByF…

97e8642

…unctionExpression

feat: Make isTypedProjectionExpression check for fully qualified func…

de21eb5

…tion name

feat: Make isTypedGroupByExpression check for fully qualified functio…

b1851a3

…n name

refactor: Set groupBy() expression return type in processTypedAggrega…

167c499

…tionColSpec function

refactor: Update comment

cd8084a

refactor: Update comment

495c377

akphi approved these changes Feb 18, 2025

View reviewed changes

akphi merged commit db7ebfb into finos:master Feb 18, 2025
16 checks passed

github-actions bot mentioned this pull request Feb 17, 2025

Iteration Release v13.30.0 #3898

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for TDS v2 (relation) groupBy in QueryBuilder #3892

Add support for TDS v2 (relation) groupBy in QueryBuilder #3892

travisstebbins commented Feb 12, 2025 •

edited

Loading

changeset-bot bot commented Feb 12, 2025 •

edited

Loading

codecov bot commented Feb 12, 2025 •

edited

Loading

akphi left a comment

akphi Feb 16, 2025

travisstebbins Feb 18, 2025 •

edited

Loading

akphi Feb 16, 2025

travisstebbins Feb 18, 2025

akphi Feb 16, 2025

travisstebbins Feb 18, 2025

Add support for TDS v2 (relation) groupBy in QueryBuilder #3892

Add support for TDS v2 (relation) groupBy in QueryBuilder #3892

Conversation

travisstebbins commented Feb 12, 2025 • edited Loading

Summary

How did you test this change?

changeset-bot bot commented Feb 12, 2025 • edited Loading

🦋 Changeset detected

codecov bot commented Feb 12, 2025 • edited Loading

Codecov Report

akphi left a comment

Choose a reason for hiding this comment

akphi Feb 16, 2025

Choose a reason for hiding this comment

travisstebbins Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

akphi Feb 16, 2025

Choose a reason for hiding this comment

travisstebbins Feb 18, 2025

Choose a reason for hiding this comment

akphi Feb 16, 2025

Choose a reason for hiding this comment

travisstebbins Feb 18, 2025

Choose a reason for hiding this comment

travisstebbins commented Feb 12, 2025 •

edited

Loading

changeset-bot bot commented Feb 12, 2025 •

edited

Loading

codecov bot commented Feb 12, 2025 •

edited

Loading

travisstebbins Feb 18, 2025 •

edited

Loading