chore: add hypercube generic functions - part02 #1715

Donya-qlik · 2025-04-06T21:27:31Z

Motivation

Moving generic hypercube functions from sense-client to nebula.js - PART02
In this PR the functions that are required to add field and alternative fields, are considered.

The source files for these functions are in sense-client (data-property-handler.ts & hypercube-handler.js)

P.S: (Part01 can be found here)

Requirements checklist

Api specification
- Ran yarn spec
  - No changes OR API changes has been formally approved
Unit/Component test coverage
Correct PR title for the changes (fix, chore, feat)

When build and tests have passed:

Add code reviewers, for example @qlik-oss/nebula-core

Copilot

Copilot reviewed 9 out of 12 changed files in this pull request and generated 2 comments.

Files not reviewed (3)

apis/nucleus/package.json: Language not supported
apis/supernova/package.json: Language not supported
package.json: Language not supported

apis/nucleus/src/utils/handlers/handler-helper.js

apis/nucleus/src/utils/handlers/data-property-handler.js

cbt1 · 2025-04-23T10:14:20Z

This is mostly just copy/pasted right? Have you done any major refactoring changes?

Caele · 2025-04-23T20:19:52Z

apis/supernova/src/handler/utils/field-helper/expand-field-derived-data/index.js

Could we change all these folders with index files into a more flat structure? To better match the rest of Nebula, it has more named files and no /folder/index.js setups.

Well...It is mostly because there are lots of functions that are not in use directly in hypercube-handler but only to define some other functions, so they are helper functions of the other functions. But yes, I can change this arrangement to a flatter structure and still keep them in separated files to be able to test them anyway.

cbt1

There are a couple of async functions and usage of Promise that I don't understand the usage of. Is there an reason behind it?

EDIT: Why is there a need to use async/Promise at all in the handler?

apis/supernova/src/handler/data-property-handler.js

cbt1 · 2025-04-24T05:51:17Z

apis/supernova/src/handler/hypercube-handler.js


    if (!this.hcProperties) {
-      return;
+      return undefined;


Why return undefined? The value from setProperties is not used anywhere?

cbt1 · 2025-04-24T05:54:25Z

apis/supernova/src/handler/utils/hypercube-helper.js

+  if (typeof self.measureDefinition.add === 'function') {
+    self.measureDefinition.add.call(null, meas, self.properties, self);
+  }
+  return Promise.resolve(addedMeasures);


Why does this return a Promise?

cbt1 · 2025-04-24T05:55:13Z

apis/supernova/src/handler/utils/hypercube-helper.js

+    return insertMainMeasure(measure, measures, idx);
+  }
+
+  return Promise.resolve();


Why return a Promise?

cbt1 · 2025-04-24T05:56:43Z

apis/supernova/src/handler/utils/hypercube-helper.js

+    arrayUtil.indexAdded(self.hcProperties.qInterColumnSortOrder, self.getDimensions().length + measure.length - 1);
+
+    if (typeof self.measureDefinition.add === 'function') {
+      return Promise.when(self.measureDefinition.add.call(null, measure, self.properties, self));


What is Promise.when? And why use a Promise?

cbt1 · 2025-04-24T05:57:35Z

apis/supernova/src/handler/utils/hypercube-helper.js

+  const measures = self.hcProperties.qLayoutExclude.qHyperCubeDef.qMeasures;
+  const idx = index ?? measures.length;
+  measures.splice(idx, 0, measure);
+  return Promise.resolve(measure);


Why return a Promise?

Donya-qlik · 2025-04-24T09:35:19Z

There are a couple of async functions and usage of Promise that I don't understand the usage of. Is there an reason behind it?

EDIT: Why is there a need to use async/Promise at all in the handler?

Yes, I see. In sense-client they all return promises, but using AngularJs promises, so I kept them as they were, to avoid breaking anything. I tried removing a couple of them, but the function failed, so it seems they are needed to ensure the data is fetched properly (e.x: adding field and sorting the orders at the right time). but I'll check those you left comment on, maybe I can remove those without facing any problem.

Donya-qlik · 2025-04-28T09:24:44Z

apis/supernova/src/handler/utils/hypercube-helper/hypercube-utils.js

-export function addMainDimension(self, dimension, index) {
-  const dimensions = self.getDimensions();
-  const idx = index ?? dimensions.length;
-
-  if (dimensions.length < self.maxDimensions()) {
-    return insertMainDimension(self, dimension, dimensions, idx);
-  }
-
-  return Promise.resolve();
-}


Moved it to a separated file add-main-dimension.js because of having a nested function

Donya-qlik · 2025-04-28T09:28:27Z

apis/supernova/src/handler/utils/hypercube-helper/hypercube-utils.js

-export function insertFieldAtIndex(self, field, alternative, currentFields, index = undefined) {
-  if (alternative || (self.maxDimensions() <= currentFields.length && currentFields.length < TOTAL_MAX.DIMENSIONS)) {
-    const fields = self.hcProperties.qLayoutExclude.qHyperCubeDef.qDimensions;
-    const idx = index ?? fields.length;
-    fields.splice(idx, 0, field);
-    return Promise.resolve(field);
-  }
-  return undefined;
-}
-


I had created this function as a helper fn, but noticed that is redundant.

Donya-qlik · 2025-04-28T09:30:33Z

apis/supernova/src/handler/utils/hypercube-helper/hypercube-utils.js

-export function insertDimensionAtIndex(self, dimension, alternative, currentDimensions, index = undefined) {
-  const dimensions = self.hcProperties.qLayoutExclude.qHyperCubeDef.qDimensions;
-  const idx = index ?? dimensions.length;
-  dimensions.splice(idx, 0, dimension);
-  return Promise.resolve(dimension);
-}
-


The same here, it is redundant, so I removed it.

Donya-qlik · 2025-04-28T09:31:44Z

apis/supernova/src/handler/utils/hypercube-helper/hypercube-utils.js

-export function addMainMeasure(self, measure, index) {
-  const measures = self.getMeasures();
-  const idx = index ?? measures.length;
-
-  if (measures.length < self.maxMeasures()) {
-    return insertMainMeasure(measure, measures, idx);
-  }
-
-  return Promise.resolve();
-}
-


Moved it to a separated file add-main-measure.js

Donya-qlik · 2025-04-28T09:45:41Z

@cbt1 @Caele could you take another look to my PR if you have some time, thanks.

cbt1

Yes, I see. In sense-client they all return promises, but using AngularJs promises, so I kept them as they were, to avoid breaking anything. I tried removing a couple of them, but the function failed, so it seems they are needed to ensure the data is fetched properly (e.x: adding field and sorting the orders at the right time). but I'll check those you left comment on, maybe I can remove those without facing any problem.

I can understand the concern to reduce risk of breaking something. But if there is no async operation in this code. The only other risk I can think of now is some outside code that consumes the code where it calls a method and expect it to be async, maybe it will call for example like this handler.someMethod().then(() => // Do something after method resolves)

cbt1 · 2025-04-28T12:46:28Z

apis/supernova/src/handler/data-property-handler.js

+    return this.addDimension(dimension, true);
+  }
+
+  maxDimensions(decrement) {


Assign a default value directly on the paramter decrement = 0. That way there is no need for const decr = decrement || 0;. Unless decrement can also be other falsy values.

cbt1 · 2025-04-28T12:49:24Z

apis/supernova/src/handler/data-property-handler.js

+    measure.qDef = measure.qDef ?? {};
+    measure.qDef.qNumFormat = measure.qDef.qNumFormat ?? {};
+
+    if (isEnabled('MASTER_measureURE_FORMAT')) {


Suggested change

if (isEnabled('MASTER_measureURE_FORMAT')) {

if (isEnabled('MASTER_MEASURE_FORMAT')) {

cbt1 · 2025-04-28T12:55:47Z

apis/supernova/src/handler/utils/hypercube-helper/hypercube-utils.js

+    arrayUtil.indexAdded(self.hcProperties.qInterColumnSortOrder, self.getDimensions().length + dimension.length - 1);
+
+    if (typeof self.dimensionDefinition.add === 'function') {
+      return Promise.when(self.dimensionDefinition.add.call(null, dimension, self.properties, self));


Promise.when does not exist.

Is this line not covered by unit tests?

Oh Sorry, I missed to fix it.
I splitted this function in my next PR, into two for different type of orders, sort-order and column-order, there it's correct and added some test cases for that but not here.

The fact is this function is not called when I normally add fields so I didn't get any incorrect value or error even when I tested adding-fields manually.

If you don't mind, in case of adding test cases, I'll go for the refactored version of this and the testcases in the next PR since I have already covered it there.

T-Wizard · 2025-04-29T13:25:11Z

apis/supernova/src/handler/hypercube-handler.js

+      dimensions.map(async (dimension) => {
+        if (hcHelper.isTotalDimensionsExceeded(this, existingDimensions)) {
+          return addedDimensions;
+        }
+
+        const dim = initializeField(dimension);
+
+        if (hcHelper.isDimensionAlternative(this, alternative)) {
+          const altDim = await hcHelper.addAlternativeDimension(this, dim);
+          addedDimensions.push(altDim);
+        } else if (existingDimensions.length < this.maxDimensions()) {
+          await hcHelper.addActiveDimension(this, dim, existingDimensions, addedDimensions, addedActive);
+          addedActive++;
+        }
+        return addedDimensions;
+      })


I do expect addDimensions to add thing in same order as dimensions array.
this looks to add them in the order of what is done first.

Good catch. But I do think it does add them in the correct order since the "awaited" call is not actually an async function and gets resolved directly.

Either way, the calls inside addDimensions does have to be async. But the method itself might still need to be, since it's possible called as an async function but some code that consumes it.

the .map (dimensions.map) will start on next item if the previous one needs awaiting.
making it possible for later items to be done before earlier items

Turns out addActiveDimension is making something that looks like an async call. So you're right, there is a risk of a race condition here.

I would then think for await (const value of object) {} is the way to go. Instead of using dimensions.map().

Ok, understand. It was for-await in the original code but I thought using .map() might be a better option. I'll change it to for-await.

@T-Wizard @cbt1 I ended up using await array.reduce() instead. Apparently for await requires regenerator-runtime, which is too heavyweight for it to allow as Eslint error says. The best replacement which follows the same concept is reduce, it waits for each previous round promise to be resolved and then it goes to the next one.

…romise at each round

cbt1 · 2025-05-05T10:51:30Z

apis/supernova/src/handler/hypercube-handler.js

+    const addedDimensions = [];
+    let addedActive = 0;
+
+    await dimensions.reduce(async (prevPromise, dimension) => {


While using reduce in this way will work. I do think you should use for await loop instead as it's easier to read and built to do what you want.

Eslint doesn't let me use it unless I ignore its error. Do you think reduce is risky to use ?

I seem like a hack to me. If someone in the future where to look at this they might wonder why reduce is being used.

IMO it's fine to just ignore the lint error. The error is not valid in this case.

cbt1

Just had some final comments. This LGTM now. But it is kind of hard to tell what has changed in this move. So for upcoming PR:s maybe change as little as possible and then in a follow-up PR make refactoring changes?

cbt1 · 2025-05-06T06:42:52Z

apis/supernova/src/handler/hypercube-handler.js

  setProperties(properties) {
    if (!properties) {
-      return;
+      return {};


Why return an object from setProperties?

In the original code in sense-client, there is an early 'return' here, however it doesn't return anything. Moving it to nebula, I faced Eslint error so I returned an empty object instead. Is there any better way to do so? then I can change it in the next PR.

This works fine for me without any lint error

setProperties(properties) { if (!properties) { return; } super.setProperties(properties); this.hcProperties = this.path ? utils.getValue(properties, `${this.path}.qHyperCubeDef`) : properties.qHyperCubeDef; if (!this.hcProperties) { return; } hcHelper.setDefaultProperties(this); hcHelper.setPropForLineChartWithForecast(this); // Set auto-sort property (compatibility 0.85 -> 0.9), // can probably be removed in 1.0 this.hcProperties.qDimensions = hcHelper.setFieldProperties(this.hcProperties.qDimensions); this.hcProperties.qMeasures = hcHelper.setFieldProperties(this.hcProperties.qMeasures); }

cbt1 · 2025-05-06T07:03:20Z

apis/supernova/src/handler/utils/field-helper/field-utils.js

+    if (!dim.qDef.qSortCriterias) {
+      const updatedDimension = { ...dim };
+      updatedDimension.qDef.qSortCriterias = [sortCriterias];
+      Object.assign(dim, updatedDimension);


I don't understand the usage of Object.assign here? Why not just assigned directly on dim.qDef.qSortCriterias instead?

Umm... true! couldn't remember why I did this... I'll change it.

Donya-qlik · 2025-05-06T11:06:57Z

Just had some final comments. This LGTM now. But it is kind of hard to tell what has changed in this move. So for upcoming PR:s maybe change as little as possible and then in a follow-up PR make refactoring changes?

Thanks @cbt1 for reviewing my PR I totally understand. These are not used anywhere in this repo to give a reference how they used to work. This one was the largest PR, hopefully the next one will be easier to review, however there also I made a few changes, but it is smaller.

Donya-qlik changed the base branch from main to dmh/VNA-46-move-hc-generic-fn April 6, 2025 21:28

Donya-qlik force-pushed the dmh/VNA-46-move-hc-generic-fn branch 4 times, most recently from eaa2870 to 0b1317b Compare April 7, 2025 08:02

Donya-qlik force-pushed the dmh/VNA-46-hs-generic-fn-part02 branch 3 times, most recently from c6ab81c to 8aafde3 Compare April 9, 2025 12:50

Donya-qlik force-pushed the dmh/VNA-46-move-hc-generic-fn branch 2 times, most recently from 222061c to 96e0ca5 Compare April 10, 2025 17:48

Donya-qlik force-pushed the dmh/VNA-46-hs-generic-fn-part02 branch 2 times, most recently from caa6d6a to ddd662f Compare April 10, 2025 23:17

Donya-qlik requested a review from Copilot April 10, 2025 23:18

Copilot AI reviewed Apr 10, 2025

View reviewed changes

apis/nucleus/src/utils/handlers/handler-helper.js Outdated Show resolved Hide resolved

apis/nucleus/src/utils/handlers/data-property-handler.js Outdated Show resolved Hide resolved

Donya-qlik marked this pull request as ready for review April 11, 2025 00:40

Donya-qlik changed the title ~~Dmh/vna 46 hs generic fn part02~~ add required functions to add dimensions & measures Apr 11, 2025

Donya-qlik changed the title ~~add required functions to add dimensions & measures~~ refactor: add required functions to add dimensions & measures Apr 11, 2025

Donya-qlik requested review from Caele, a-m-dev, cbt1, eliseeborn and enell April 11, 2025 00:44

Donya-qlik force-pushed the dmh/VNA-46-hs-generic-fn-part02 branch from b42c161 to c8d4a7d Compare April 11, 2025 00:58

Donya-qlik changed the title ~~refactor: add required functions to add dimensions & measures~~ refactor: add required functions to add dimensions & measures / part02 Apr 11, 2025

Donya-qlik force-pushed the dmh/VNA-46-move-hc-generic-fn branch 2 times, most recently from 7ee1ab0 to 430b60c Compare April 11, 2025 09:09

Donya-qlik marked this pull request as draft April 11, 2025 11:52

Donya-qlik force-pushed the dmh/VNA-46-hs-generic-fn-part02 branch 3 times, most recently from fc374d0 to 70cd75b Compare April 15, 2025 13:10

chore: remove redundants

0501e20

Caele reviewed Apr 23, 2025

View reviewed changes

cbt1 reviewed Apr 24, 2025

View reviewed changes

Donya-qlik force-pushed the dmh/VNA-46-hs-generic-fn-part02 branch from 3020075 to 87b836f Compare April 26, 2025 00:35

Donya-qlik requested review from Caele and cbt1 April 26, 2025 00:36

chore: change new helper files structure

8a4d712

Donya-qlik force-pushed the dmh/VNA-46-hs-generic-fn-part02 branch from 87b836f to 8a4d712 Compare April 27, 2025 14:24

chore: fix the path in unit tests

73f2c7b

Donya-qlik commented Apr 28, 2025

View reviewed changes

Donya-qlik requested a review from a team April 28, 2025 12:59

cbt1 reviewed Apr 28, 2025

View reviewed changes

chore: fix comments

10ee682

Donya-qlik requested a review from cbt1 April 29, 2025 11:32

T-Wizard reviewed Apr 29, 2025

View reviewed changes

chore: replace map() with reduce() to keep the loop waiting for the p…

558d04f

…romise at each round

cbt1 reviewed May 5, 2025

View reviewed changes

chore: replace reduce() with for-await

9aa52c3

Donya-qlik requested review from T-Wizard and cbt1 May 5, 2025 13:47

cbt1 approved these changes May 6, 2025

View reviewed changes

chore: refactoring a code line

8bc6b5d

Donya-qlik merged commit 445d694 into main May 6, 2025
10 checks passed

Donya-qlik deleted the dmh/VNA-46-hs-generic-fn-part02 branch May 6, 2025 11:07

Donya-qlik mentioned this pull request May 12, 2025

chore: add hypercube generic functions - part03 #1729

Merged

6 tasks

Donya-qlik mentioned this pull request May 21, 2025

chore: add hypercube generic functions - part04 #1746

Merged

6 tasks

	if (isEnabled('MASTER_measureURE_FORMAT')) {
	if (isEnabled('MASTER_MEASURE_FORMAT')) {

chore: add hypercube generic functions - part02 #1715

chore: add hypercube generic functions - part02 #1715

Uh oh!

Conversation

Donya-qlik commented Apr 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Requirements checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cbt1 commented Apr 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbt1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Donya-qlik commented Apr 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Donya-qlik commented Apr 28, 2025

Uh oh!

cbt1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

T-Wizard Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbt1 Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Donya-qlik commented Apr 6, 2025 •

edited

Loading

cbt1 left a comment •

edited

Loading

T-Wizard Apr 30, 2025 •

edited

Loading

cbt1 Apr 30, 2025 •

edited

Loading