Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI-9786 - Explicitly preserve the default measure when migrating from 4.3 to 5.0 #130

Merged
merged 8 commits into from
Nov 4, 2024

Conversation

antoinetissier
Copy link
Collaborator

See UI-9786 and/or the multiline comment of _addDefaultMeasureIfNoneIsExplicitlyExpressed to understand the issue and the fix.

Copy link
Contributor

@NZepeda NZepeda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks good!

Copy link
Collaborator

@Nouzbe Nouzbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried to reproduce in AUI4 and I actually got no measures shown either:

image

Can you reproduce?


const cube = dataModelsForTests.sandbox.catalogs[0].cubes[0];

describe("_addDefaultMeasureIfNoneIsExplicitlyExpressed", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to add a test ensuring that the function does not early-return by mistake when the query does not contain any measure but a hierarchy is sorted. For instance:

WITH
 Member [Measures].[[Currency]].[Currency]].[Currency]]_for_order] AS Rank(
  [Currency].[Currency].CurrentMember,
  [Currency].[Currency].Members
) 
SELECT
  NON EMPTY Order(
    Hierarchize(
      Descendants(
        {
          [Currency].[Currency].[ALL].[AllMember]
        },
        1,
        SELF_AND_BEFORE
      )
    ),
    [Measures].[[Currency]].[Currency]].[Currency]]_for_order],
    DESC
  ) ON ROWS
  FROM [EquityDerivativesCube]

I expect [Measures].[contributors.COUNT] to be added in this query.

@Nouzbe Nouzbe assigned NZepeda and unassigned Nouzbe Oct 1, 2024
@NZepeda
Copy link
Contributor

NZepeda commented Oct 1, 2024

I just tried to reproduce in AUI4 and I actually got no measures shown either:

image

Can you reproduce?

@Nouzbe This happens only for hierarchies on columns, if there is a hierarchy on columns, but no measure expressed, the default measure is shown on AUI-4

Screenshot 2024-10-01 at 2 27 43 PM

* - not displayed in Atoti 5.0
* This function explicitly adds it to the MDX so that it is isofunctional for the user.
*/
export function _addDefaultMeasureIfNoneIsExplicitlyExpressed(
Copy link
Contributor

@NZepeda NZepeda Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On AUI-4, the default measure is only displayed when there is a hierarchy on columns with no explicit measures. It might be good to modify this function so that the measure is only explicitly added to the query in that scenario. Right now, it adds the measure if a hierarchy is expressed in either the rows or columns axis, which is technically not isofunctional to what AUI-4 does. Thoughts @Nouzbe ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the behavior of AUI-4 below:

Screen.Recording.2024-10-04.at.10.43.33.AM.mov

@NZepeda
Copy link
Contributor

NZepeda commented Oct 4, 2024

I've added some commits to this PR to align the migration script's functionality with that of AUI-4. On AUI-4, the default measure was only added when no measures were expressed and the query contained at least hierarchy on the columns axis. The default measure was not added when the columns axis was empty. See this video for details. @Nouzbe @antoinetissier Let me know if you agree/disagree with the changes. We can drop the new commits if you disagree with the changes.

@NZepeda NZepeda assigned Nouzbe and antoinetissier and unassigned NZepeda Oct 4, 2024
Copy link
Collaborator Author

@antoinetissier antoinetissier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @NZepeda 👍

@antoinetissier antoinetissier merged commit 1760329 into main Nov 4, 2024
2 checks passed
@antoinetissier antoinetissier deleted the personal/a.tissier/preserve-default-measure branch November 4, 2024 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants