Snapshot building for filter operations (part of roundtrip) #3737

gs-gunjan · 2024-12-13T16:02:11Z

Summary

How did you test this change?

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

changeset-bot · 2024-12-13T16:02:16Z

🦋 Changeset detected

Latest commit: 8c41ad9

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

This PR includes changesets to release 28 packages

Name	Type
@finos/legend-data-cube	Patch
@finos/legend-application-data-cube-bootstrap	Patch
@finos/legend-application-data-cube	Patch
@finos/legend-application-query	Patch
@finos/legend-application-repl	Patch
@finos/legend-application-studio	Patch
@finos/legend-query-builder	Patch
@finos/legend-vscode-extension-dependencies	Patch
@finos/legend-application-data-cube-deployment	Patch
@finos/legend-application-query-bootstrap	Patch
@finos/legend-extension-dsl-service	Patch
@finos/legend-application-repl-deployment	Patch
@finos/legend-application-studio-bootstrap	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-diagram	Patch
@finos/legend-extension-dsl-persistence	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-extension-dsl-data-space	Patch
@finos/legend-application-query-deployment	Patch
@finos/legend-application-studio-deployment	Patch
@finos/legend-application-pure-ide	Patch
@finos/legend-application-pure-ide-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

...s/legend-data-cube/src/stores/core/__tests__/DataCubeQueryFilterSnapshotBuilder-repl.test.ts

packages/legend-data-cube/src/stores/core/filter/DataCubeQueryFilterOperation.ts

packages/legend-data-cube/src/stores/core/DataCubeQueryEngine.ts

packages/legend-data-cube/src/stores/core/DataCubeQuerySnapshotBuilderUtils.ts

packages/legend-data-cube/src/stores/core/DataCubeQuerySnapshotBuilder.ts

packages/legend-data-cube/src/stores/core/DataCubeQuerySnapshotBuilderUtils.ts

...s/legend-data-cube/src/stores/core/__tests__/DataCubeQueryFilterSnapshotBuilder-repl.test.ts

codecov · 2024-12-23T17:28:14Z

Codecov Report

Attention: Patch coverage is 69.55556% with 274 lines in your changes missing coverage. Please review.

Project coverage is 45.26%. Comparing base (fe9b778) to head (8c41ad9).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
...c/stores/core/DataCubeQuerySnapshotBuilderUtils.ts	81.64%	58 Missing ⚠️
...eryFilterOperation__EqualCaseInsensitiveColumn.tsx	27.77%	13 Missing ⚠️
...lter/DataCubeQueryFilterOperation__EqualColumn.tsx	27.77%	13 Missing ⚠️
...ataCubeQueryFilterOperation__GreaterThanColumn.tsx	27.77%	13 Missing ⚠️
...QueryFilterOperation__GreaterThanOrEqualColumn.tsx	27.77%	13 Missing ⚠️
...r/DataCubeQueryFilterOperation__LessThanColumn.tsx	27.77%	13 Missing ⚠️
...ubeQueryFilterOperation__LessThanOrEqualColumn.tsx	27.77%	13 Missing ⚠️
...FilterOperation__NotEqualCaseInsensitiveColumn.tsx	27.77%	13 Missing ⚠️
...be/src/stores/core/DataCubeQuerySnapshotBuilder.ts	73.68%	10 Missing ⚠️
...e/filter/DataCubeQueryFilterOperation__Contain.tsx	50.00%	9 Missing ⚠️
... and 13 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3737      +/-   ##
==========================================
+ Coverage   44.48%   45.26%   +0.78%     
==========================================
  Files        2245     2245              
  Lines      389572   390289     +717     
  Branches    16380    16580     +200     
==========================================
+ Hits       173306   176671    +3365     
+ Misses     215798   212836    -2962     
- Partials      468      782     +314

Files with missing lines	Coverage Δ
...beQueryFilterOperation__EndWithCaseInsensitive.tsx	`82.07% <100.00%> (+82.07%)`	⬆️
...ore/filter/DataCubeQueryFilterOperation__Equal.tsx	`84.49% <100.00%> (+84.49%)`	⬆️
...taCubeQueryFilterOperation__GreaterThanOrEqual.tsx	`75.00% <100.00%> (+75.00%)`	⬆️
...re/filter/DataCubeQueryFilterOperation__IsNull.tsx	`72.34% <100.00%> (+72.34%)`	⬆️
.../filter/DataCubeQueryFilterOperation__NotEqual.tsx	`74.33% <100.00%> (+74.33%)`	⬆️
...eQueryFilterOperation__NotEqualCaseInsensitive.tsx	`82.56% <100.00%> (+82.56%)`	⬆️
...r/DataCubeQueryFilterOperation__NotEqualColumn.tsx	`77.77% <100.00%> (+77.77%)`	⬆️
...end-data-cube/src/stores/view/DataCubeViewState.ts	`0.00% <0.00%> (ø)`
...filter/DataCubeQueryFilterOperation__IsNotNull.tsx	`59.79% <45.45%> (+59.79%)`	⬆️
...e/filter/DataCubeQueryFilterOperation__Contain.tsx	`66.00% <50.00%> (+66.00%)`	⬆️
... and 20 more

... and 199 files with indirect coverage changes

packages/legend-data-cube/src/stores/core/__tests__/DataCubeQuerySnapshotBuilder.repl-test.ts

...s/legend-data-cube/src/stores/core/__tests__/DataCubeQueryFilterSnapshotBuilder.repl-test.ts

packages/legend-data-cube/src/stores/core/DataCubeQueryEngine.ts

akphi · 2024-12-24T14:38:12Z

packages/legend-data-cube/src/stores/core/DataCubeQuerySnapshotBuilderUtils.ts

+  return undefined;
+}
+
+function _buildFilterConditionSnapshot(


as discussed, distribute this logic to each of the operator. For common logic that can be shared, we can leave them here, e.g. _not(), etc.

pls check , if this looks better, thanks

packages/legend-data-cube/src/stores/core/__tests__/Test__DataCubeEngine.ts

...end-data-cube/src/stores/core/__tests__/DataCubeQueryFilterSnapshotBuilder.data-cube-test.ts

akphi

Left some comments, this is good enough, let's merge this. We'll come back for some cleanups.

packages/legend-data-cube/src/stores/core/__tests__/Test__DataCubeEngine.ts

...end-data-cube/src/stores/core/__tests__/DataCubeQueryFilterSnapshotBuilder.data-cube-test.ts

packages/legend-data-cube/src/stores/core/DataCubeQuerySnapshotBuilderUtils.ts

akphi · 2025-01-06T22:40:29Z

packages/legend-data-cube/src/stores/core/DataCubeQuerySnapshotBuilderUtils.ts

+      return _buildFilterSnapshot(vs, filterOperations);
+    }
+  }
+  return undefined;


One key thing you miss is that what we're doing is not a high-tolerable read, the algo expects a very particular form of query, if deviated, we throw error.

akphi · 2025-01-06T22:47:45Z

packages/legend-data-cube/src/stores/core/DataCubeQuerySnapshotBuilderUtils.ts

+  | undefined {
+  if (vs instanceof V1_AppliedFunction) {
+    if (
+      Object.values(DataCubeFunction)


Not sure why we need this?

DataCubeFunction does define all the functions DataCube should understand but they are not pertained to functions used within a filter. The logic here should be really simple:

Found an V1_AppliedFunction, run through the list of filter operations until we found a match

If no match found, try NOT flavor

If still not match found, throw error.

If not found V1_AppliedFunction in step (1), throw error.

akphi · 2025-01-06T23:03:23Z

packages/legend-data-cube/src/stores/core/DataCubeQuerySnapshotBuilderUtils.ts

+    af.parameters[0] instanceof V1_AppliedFunction &&
+    af.parameters[0].function === _functionName(DataCubeFunction.TO_LOWERCASE)
+  ) {
+    return _buildCaseInsensitiveFilterConditionSnapshot(af);


See the above comment on the algo outline. I think you're making this a little too complicated. Each branch of this method in theory can be converted into a utility function, e.g. how to read the V1_AppliedProperty and see if it fits what a particular operator supports and then to build the snapshot, an operator from the list of DataCubeQueryFilterOperator has to be picked, nothing outside of that list hsould be

For example, in case of Contains, the logic in the buildConditionSnapshot method of DatacubeQueryFilterOperation__Contain needs

akphi · 2025-01-06T23:37:12Z

packages/legend-data-cube/src/stores/core/filter/DataCubeQueryFilterOperation__Equal.tsx

-    //         operatorFunctionFullPath,
-    //       )}() expects '1 argument'`,
-    //     );
+  // buildConditionSnapshot(expression: V1_AppliedFunction) {


let's remove all these stuffs

akphi · 2025-01-06T23:38:18Z

packages/legend-data-cube/src/stores/core/filter/DataCubeQueryFilterOperation__Contain.tsx

+    if (value instanceof V1_PrimitiveValueSpecification) {
+      filterConditionSnapshot.value = _dataCubeOperationValue(value);
+    }
+    return filterConditionSnapshot satisfies DataCubeQuerySnapshotFilterCondition;


no need to do this, TS is smart enough to force this to be DataCubeQuerySnapshotFilterCondition

akphi · 2025-01-06T23:43:44Z

packages/legend-data-cube/src/stores/core/filter/DataCubeQueryFilterOperation__Contain.tsx

@@ -73,8 +80,15 @@ export class DataCubeQueryFilterOperation__Contain extends DataCubeQueryFilterOp
  }

  buildConditionSnapshot(expression: V1_AppliedFunction) {
-    /** TODO: @datacube roundtrip */
-    return undefined;
+    const value = expression.parameters[1];


again, the spirit here is to verify the expression follows a specific form, else, return undefined.

akphi · 2025-01-07T06:32:55Z

packages/legend-data-cube/src/stores/core/DataCubeQuerySnapshotBuilder.ts

+  if (funcMap.filter) {
+    data.filter = _buildFilterSnapshot(
+      _lambdaParam(funcMap.filter, 0).body[0]!,
+      filterOperations!,


Avoid using the non-null assertion operator like this, this is pretty much a typing hack, and we strive to be ax explicit as we could when giving out proper message

akphi requested changes Dec 16, 2024

View reviewed changes

akphi reviewed Dec 16, 2024

View reviewed changes

...s/legend-data-cube/src/stores/core/__tests__/DataCubeQueryFilterSnapshotBuilder-repl.test.ts Outdated Show resolved Hide resolved

akphi reviewed Dec 23, 2024

View reviewed changes

gs-gunjan force-pushed the snapshot_filter branch from efc0e74 to 5887c4e Compare December 24, 2024 09:56

gs-gunjan marked this pull request as ready for review December 24, 2024 11:03

gs-gunjan requested a review from a team as a code owner December 24, 2024 11:03

akphi reviewed Dec 24, 2024

View reviewed changes

gs-gunjan added 3 commits January 6, 2025 12:20

Snapshot building for filter operations (part of roundtrip)

5ab43fd

doing minor cleanup

e1306ad

decentralizing filter condition logic

c6f7e65

gs-gunjan force-pushed the snapshot_filter branch from e17a1b3 to c6f7e65 Compare January 6, 2025 09:08

gs-gunjan added 2 commits January 6, 2025 14:46

using correct enum to check condition

e3935a3

fixing tests

8c41ad9

akphi approved these changes Jan 7, 2025

View reviewed changes

akphi merged commit 1dd08af into finos:master Jan 7, 2025
16 checks passed

akphi added a commit to akphi/legend-studio that referenced this pull request Jan 7, 2025

datacube: cleanups post finos#3737

e9327d8

akphi reviewed Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snapshot building for filter operations (part of roundtrip) #3737

Snapshot building for filter operations (part of roundtrip) #3737

gs-gunjan commented Dec 13, 2024

changeset-bot bot commented Dec 13, 2024 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading

akphi Dec 24, 2024

gs-gunjan Jan 6, 2025

akphi left a comment

akphi Jan 6, 2025

akphi Jan 6, 2025

akphi Jan 6, 2025

akphi Jan 6, 2025

akphi Jan 6, 2025

akphi Jan 6, 2025

akphi Jan 7, 2025

Snapshot building for filter operations (part of roundtrip) #3737

Snapshot building for filter operations (part of roundtrip) #3737

Conversation

gs-gunjan commented Dec 13, 2024

Summary

How did you test this change?

changeset-bot bot commented Dec 13, 2024 • edited Loading

🦋 Changeset detected

codecov bot commented Dec 23, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akphi 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Dec 13, 2024 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading