UI-8742 - More robust MDX cleanup (#115)

* UI-8742 - More robust MDX cleanup * Bump to latest versions of the SDK * add robustness * format test snapshot * PR feedback * Update src/4.3_to_5.0/_cleanupDrilldownLevel.ts Co-authored-by: Benjamin Amelot <[email protected]> * Update src/4.3_to_5.0/_cleanupDrilldownLevel.ts Co-authored-by: Benjamin Amelot <[email protected]> --------- Co-authored-by: Benjamin Amelot <[email protected]>
activeviam · Sep 29, 2023 · 65c3c42 · 65c3c42
1 parent 441e048
commit 65c3c42
Show file tree

Hide file tree

Showing 9 changed files with 956 additions and 886 deletions.
diff --git a/__mocks__/clipboard-polyfill/text.js b/__mocks__/clipboard-polyfill/text.js
@@ -0,0 +1,4 @@
+// This module is a transitive dependency of @activeviam packages.
+// It is used in the browser in Atoti UI, but it is not necessary for atoti-ui-migration.
+// See https://github.com/activeviam/atoti-ui-migration/blob/c06e91517d4898bf54e919944e49fae6e270036c/jest.config.js#L7-L8
+module.exports = {};
diff --git a/package.json b/package.json
@@ -43,7 +43,7 @@
     "xml2js": "^0.4.23"
   },
   "devDependencies": {
-    "@activeviam/activeui-sdk-scripts": "file:vendor\\activeviam-activeui-sdk-scripts-5.0.29-20230915115910-fc34c05.tgz",
+    "@activeviam/activeui-sdk-scripts": "file:vendor\\activeviam-a2ctiveui-sdk-scripts-5.0.29-20230925104351-05a6f39.tgz",
     "@ant-design/icons": "^4.4.0",
     "@types/cli-progress": "^3.11.0",
     "@types/jest": "^26.0.14",

diff --git a/src/_cleanupDescendants.test.ts b/src/_cleanupDescendants.test.ts
@@ -54,13 +54,13 @@ describe("_cleanupDescendants", () => {
     `);
   });
 
-  it("replaces a useless Descendants function when the second argument is a level index", () => {
+  it("replaces a useless Descendants function when the first argument is a set of members from the leaf level, and the second argument is a distance", () => {
     const mdx = parse<MdxSelect>(`                                 
     SELECT                                                                                                                                                              
       NON EMPTY Hierarchize(
             Descendants(
               {
-                [Booking].[Desk].[ALL].[AllMember].[LegalEntityA]
+                [Booking].[Desk].[ALL].[AllMember].[LegalEntityA].[BusinessUnitA].[DeskA].[0]
               },
               1
             )
@@ -71,13 +71,29 @@ describe("_cleanupDescendants", () => {
       "SELECT
         NON EMPTY Hierarchize(
           {
-            [Booking].[Desk].[ALL].[AllMember].[LegalEntityA]
+            [Booking].[Desk].[ALL].[AllMember].[LegalEntityA].[BusinessUnitA].[DeskA].[0]
           }
         ) ON ROWS
         FROM [EquityDerivativesCube]"
     `);
   });
 
+  it("does not replace a Descendants function when the first argument is a set of members from a non-leaf level, and the second argument is a distance", () => {
+    const mdx = parse<MdxSelect>(`                                 
+    SELECT                                                                                                                                                              
+      NON EMPTY Hierarchize(
+            Descendants(
+              {
+                [Booking].[Desk].[ALL].[AllMember].[LegalEntityA].[BusinessUnitA]
+              },
+              1
+            )
+          ) ON ROWS
+        FROM [EquityDerivativesCube]`);
+    const cleanMdx = _cleanupDescendants(mdx, cube);
+    expect(cleanMdx).toStrictEqual(mdx);
+  });
+
   it("replaces a useless Descendants function called on a set with members from the leaf level, without a second argument", () => {
     const mdx = parse<MdxSelect>(`                                 
     SELECT                                                                                                                                                              

diff --git a/src/_cleanupDescendants.ts b/src/_cleanupDescendants.ts
@@ -3,9 +3,7 @@ import { getHierarchy, getLevelIndex } from "@activeviam/data-model";
 import {
   MdxFunction,
   findDescendant,
-  getHierarchies,
   getLevels,
-  isMdxCompoundIdentifier,
   isMdxFunction,
   isMdxLiteral,
   stringify,
@@ -20,50 +18,31 @@ const canDescendantsBeReplacedByItsFirstArgument = (
   descendantsNode: MdxFunction,
   cube: Cube,
 ) => {
-  const [set, downToLevel] = descendantsNode.arguments;
+  const set = descendantsNode.arguments[0];
   const levelsInSet = getLevels(set, { cube });
-
-  // If no levels are found in the input set, for instance when it consists only in the `CurrentMember` of a hierarchy (which could be on any level), the `Descendants` function should not be removed.
   if (levelsInSet.length === 0) {
+    // If no levels are found in the input set, for instance when it consists only in the `CurrentMember` of a hierarchy (which could be on any level), the `Descendants` function should not be removed.
     return false;
   }
 
-  const shallowestLevelIndexInSet = Math.min(
-    ...levelsInSet.map((levelCoordinates) =>
-      getLevelIndex({ cube, ...levelCoordinates }),
-    ),
-  );
+  const { dimensionName, hierarchyName } = levelsInSet[0];
 
-  if (!downToLevel) {
-    // Only the set as argument: check whether it holds only leaf members.
-    const hierarchyCoordinates = getHierarchies(set, { cube })[0];
-    const hierarchy = getHierarchy(hierarchyCoordinates, cube);
-    return shallowestLevelIndexInSet === hierarchy.levels.length - 1;
-  }
+  const levelIndexesInSet = levelsInSet.map((levelCoordinates) =>
+    getLevelIndex({ cube, ...levelCoordinates }),
+  );
+  const shallowestLevelIndexInSet = Math.min(...levelIndexesInSet);
+  const deepestLevelIndexInSet = Math.max(...levelIndexesInSet);
 
-  let downToLevelIndex;
-  if (isMdxLiteral(downToLevel)) {
-    downToLevelIndex = parseInt(downToLevel.value, 10);
-  } else {
-    if (!isMdxCompoundIdentifier(downToLevel)) {
-      throw new Error(
-        `Invalid second argument of Descendants. Expected a level index or a level unique name, but got: ${stringify(
-          downToLevel,
-          { indent: true },
-        )}`,
-      );
-    }
-    const [dimensionName, hierarchyName, levelName] =
-      downToLevel.identifiers.map((identifier) => identifier.value);
-    downToLevelIndex = getLevelIndex({
-      cube,
+  const deepestLevelExpressedInDescendantsNode =
+    getIndexOfDeepestLevelExpressedInDescendantsNode({
+      descendantsNode,
       dimensionName,
       hierarchyName,
-      levelName,
+      indexOfDeepestLevelExpressedInInputSet: deepestLevelIndexInSet,
+      cube,
     });
-  }
 
-  return shallowestLevelIndexInSet >= downToLevelIndex;
+  return shallowestLevelIndexInSet >= deepestLevelExpressedInDescendantsNode;
 };
 
 /**
@@ -73,10 +52,7 @@ const canDescendantsBeReplacedByItsFirstArgument = (
  * Descendants([Currency].[Currency].[ALL].[AllMember].[EUR], [Currency].[Currency].[Currency]) => [Currency].[Currency].[ALL].[AllMember].[EUR]
  *
  */
-export function _cleanupDescendants<T extends MdxSelect | MdxDrillthrough>(
-  mdx: T,
-  cube: Cube,
-): T {
+export function _cleanupDescendants<T extends Mdx>(mdx: T, cube: Cube): T {
   return produce(mdx, (draft) => {
     let nextNodeToCleanup;
     while (

diff --git a/src/_cleanupDrilldownLevel.test.ts b/src/_cleanupDrilldownLevel.test.ts
@@ -6,7 +6,7 @@ import { _cleanupDrilldownLevel } from "./_cleanupDrilldownLevel";
 const cube = dataModelsForTests.sandbox.catalogs[0].cubes[0];
 
 describe("_cleanupDrilldownLevel", () => {
-  it("replaces a DrilldownLevel function by its input set if the deepest level expressed in the set is a leaf level", () => {
+  it("replaces a DrilldownLevel function by its input set if it is the only argument, and if the deepest level expressed within it is a leaf level", () => {
     const mdx = parse(`                                 
     SELECT                                                                                                                                                              
       NON EMPTY Hierarchize(
@@ -28,4 +28,41 @@ describe("_cleanupDrilldownLevel", () => {
         FROM [EquityDerivativesCube]"
     `);
   });
+
+  it("does not replace a DrilldownLevel function if it has several arguments", () => {
+    // See the function doc: https://learn.microsoft.com/en-us/sql/mdx/drilldownlevel-mdx?view=sql-server-ver16
+    // Extra arguments can include a level expression or a hierarchy index to specify where to drilldown to.
+    // These more complex cases are not covered by _cleanupDrilldownLevel: better to keep suboptimal MDX than break it.
+    const mdx = parse(`                                 
+    SELECT                                                                                                                                                              
+      NON EMPTY Hierarchize(
+            DrilldownLevel(
+              {
+                (
+                  [Geography].[City].[ALL].[AllMember].[Berlin],
+                  [Currency].[Currency].[ALL].[AllMember].[EUR]
+                )
+              },
+              1
+            )
+          ) ON ROWS
+        FROM [EquityDerivativesCube]`);
+    const cleanMdx = _cleanupDrilldownLevel(mdx, cube);
+    expect(cleanMdx).toStrictEqual(mdx);
+  });
+
+  it("does not replace a DrilldownLevel function if the deepest level expressed in the input set is not a leaf level", () => {
+    const mdx = parse(`                                 
+    SELECT                                                                                                                                                              
+      NON EMPTY Hierarchize(
+            DrilldownLevel(
+              {
+                [Booking].[Desk].[ALL].[AllMember].[LegalEntityA].[BusinessUnitA]
+              }
+            )
+          ) ON ROWS
+        FROM [EquityDerivativesCube]`);
+    const cleanMdx = _cleanupDrilldownLevel(mdx, cube);
+    expect(cleanMdx).toStrictEqual(mdx);
+  });
 });
diff --git a/src/_cleanupDrilldownLevel.ts b/src/_cleanupDrilldownLevel.ts
@@ -1,5 +1,5 @@
 import { Mdx, Cube } from "@activeviam/activeui-sdk";
-import { getHierarchy, getLevelIndex } from "@activeviam/data-model";
+import { getHierarchy, getLevelIndex, areHierarchiesEqual } from "@activeviam/data-model";
 import {
   MdxFunction,
   findDescendant,
@@ -11,21 +11,34 @@ import { produce } from "immer";
 import _set from "lodash/set";
 
 /**
- * If the input set holds members of the leaf level, then DrilldownLevel can be replaced by it.
+ * A DrilldownLevel can be replaced by its first argument if both the following conditions are met:
+ * - It has a single argument, which is a set.
+ * - In this set, the deepest level expressed in the first hierarchy (which is the one being implicitly drilled) is a leaf level (and therefore cannot be drilled down)
  */
 const canDrilldownLevelBeReplacedByItsFirstArgument = (
   drilldownLevelNode: MdxFunction,
   cube: Cube,
 ) => {
-  const set = drilldownLevelNode.arguments[0];
-  const levelsInSet = getLevels(set, { cube });
+  const [set, ...otherArguments] = drilldownLevelNode.arguments;
+  if (otherArguments.length > 0) {
+    // Do not attempt to cleanup the more complex DrilldownLevel expressions.
+    return false;
+  }
+
+  const hierarchyCoordinates = getHierarchies(set, { cube })[0];
+  const hierarchy = getHierarchy(hierarchyCoordinates, cube);
+
+  const levelsInSet = getLevels(set, {
+    cube,
+  }).filter((levelCoordinates) =>
+    areHierarchiesEqual(levelCoordinates, hierarchyCoordinates),
+  );
   const deepestLevelIndexInSet = Math.max(
     ...levelsInSet.map((levelCoordinates) =>
       getLevelIndex({ cube, ...levelCoordinates }),
     ),
   );
-  const hierarchyCoordinates = getHierarchies(set, { cube })[0];
-  const hierarchy = getHierarchy(hierarchyCoordinates, cube);
+
   return deepestLevelIndexInSet === hierarchy.levels.length - 1;
 };
 

diff --git a/src/_fixErroneousExpansionMdx.ts b/src/_fixErroneousExpansionMdx.ts
@@ -20,16 +20,15 @@ import {
  * But the returned mdx does not have the typical useless (and dangerous) parts of queries created by collapsing then re-expanding a member in ActiveUI 4.
  * See https://support.activeviam.com/jira/browse/UI-6692
  */
-export function _fixErroneousExpansionMdx(
-  mdx: MdxSelect | MdxDrillthrough,
-  cube: Cube,
-): MdxSelect | MdxDrillthrough {
+export function _fixErroneousExpansionMdx<
+  T extends MdxSelect | MdxDrillthrough,
+>(mdx: T, cube: Cube): T {
   if (isMdxDrillthrough(mdx)) {
     return mdx;
   }
 
-  return produce<MdxSelect>(mdx, (draft) => {
-    draft.axes.forEach((axis) => {
+  return produce<T>(mdx, (draft) => {
+    (draft as MdxSelect).axes.forEach((axis) => {
       const levelsOnAxis = getLevels(axis, { cube });
 
       let erroneousUnionNode:

diff --git a/src/_migrateQuery.ts b/src/_migrateQuery.ts
@@ -77,19 +77,21 @@ export const _migrateQuery = <T extends MdxSelect | MdxDrillthrough>({
   }
 
   const parsedMdx = parse<T>(mdx);
-  const mdxWithoutObsoleteDescendants = _cleanupDescendants(parsedMdx, cube);
-  const mdxWithoutObsoleteDrilldownLevel = _cleanupDrilldownLevel(
-    mdxWithoutObsoleteDescendants,
-    cube,
-  );
-  const fixedMdx = _fixErroneousExpansionMdx(
-    mdxWithoutObsoleteDrilldownLevel,
-    cube,
-  );
-  const filters = getFilters(fixedMdx, { cube }).map(
+
+  let improvedMdx = parsedMdx;
+  try {
+    improvedMdx = _cleanupDescendants(improvedMdx, cube);
+    improvedMdx = _cleanupDrilldownLevel(improvedMdx, cube);
+    improvedMdx = _fixErroneousExpansionMdx(improvedMdx, cube);
+  } catch (e) {
+    // The MDX may be suboptimal, but it is expected to be valid from the perspective of the MDX specification.
+    // So attempt to improve it, but do not block the migration if the process fails.
+  }
+
+  const filters = getFilters(improvedMdx, { cube }).map(
     ({ mdx: filterMdx }) => filterMdx,
   );
-  const mdxWithoutFilters = setFilters(fixedMdx, { filters: [], cube });
+  const mdxWithoutFilters = setFilters(improvedMdx, { filters: [], cube });
 
   // TODO UI-5036 Migrate query ranges.
   return [