Skip to content

fix: two CellService bugs — multi-hierarchy dict overwrite and missing kwargs#1421

Open
kaizeenn wants to merge 1 commit into
cubewise-code:masterfrom
kaizeenn:fix/1068-1113-cellservice-bugs
Open

fix: two CellService bugs — multi-hierarchy dict overwrite and missing kwargs#1421
kaizeenn wants to merge 1 commit into
cubewise-code:masterfrom
kaizeenn:fix/1068-1113-cellservice-bugs

Conversation

@kaizeenn

Copy link
Copy Markdown

Summary

Fixes two bugs in CellService.py that have been open for a while.


Fix #1068clear_with_dataframe overwrites hierarchies

Root cause: When dimension_mapping maps a dimension to a list of hierarchies, the loop at line ~776 does:

for hierarchy in hierarchies:
    mdx_selections[dimension] = MdxHierarchySet.tm1_subset_all(...)

Each iteration overwrites the same dict key, so only the last hierarchy survives and the generated MDX is wrong.

Fix: Accumulate a MdxHierarchySet.union() across all hierarchies so every one contributes to the MDX crossjoin:

combined = None
for hierarchy in hierarchies:
    hset = MdxHierarchySet.tm1_subset_all(dimension=dimension, hierarchy=hierarchy).filter_by_level(0)
    combined = hset if combined is None else combined.union(hset)
mdx_selections[dimension] = combined

Also fixed a minor error-message bug on the same line (reported in the same issue): the ValueError for a non-str mapping value referenced loop variable dimension instead of the DataFrame column variable column.


Fix #1113execute_mdx_dataframe_pivot swallows kwargs

Root cause: The method signature had no **kwargs, so calling it with e.g. cell_properties=["FormattedValue"] or use_compact_json=True raised TypeError even though extract_cellset_dataframe_pivot accepts those arguments.

Fix: Add **kwargs to the signature and forward it:

def execute_mdx_dataframe_pivot(self, mdx, dropna=False, fill_value=None, sandbox_name=None, **kwargs):
    cellset_id = self.create_cellset(mdx=mdx, sandbox_name=sandbox_name)
    return self.extract_cellset_dataframe_pivot(
        cellset_id=cellset_id, dropna=dropna, fill_value=fill_value, sandbox_name=sandbox_name, **kwargs
    )

Tests

Added Tests/test_cellservice_unit.py — 6 unit tests that run without a live TM1 server (mocking the REST layer and cube service):

  • Single hierarchy str mapping still works
  • Two hierarchies both appear in MDX with UNION
  • Three hierarchies all appear
  • **kwargs forwarded correctly to extract_cellset_dataframe_pivot
  • Extra kwargs like cell_properties forwarded
  • No kwargs still works

All 6 pass.

…g kwargs

Fix cubewise-code#1068: clear_with_dataframe overwrites mdx_selections[dimension] on
each iteration when dimension_mapping maps a dimension to a list of
hierarchies, so only the last hierarchy appears in the generated MDX.
Replace the overwriting loop with a MdxHierarchySet.union() accumulation
so every hierarchy contributes to the MDX crossjoin.

Fix cubewise-code#1113: execute_mdx_dataframe_pivot has no **kwargs parameter, so
callers cannot pass arguments like cell_properties=["FormattedValue"] or
use_compact_json=True that extract_cellset_dataframe_pivot accepts. Add
**kwargs to the signature and forward it to extract_cellset_dataframe_pivot.

Also fix a minor error-message bug on the same line (cubewise-code#1068 report):
the ValueError for a non-str dimension_mapping value referred to the
loop variable 'dimension' instead of the DataFrame column variable
'column'.

Add Tests/test_cellservice_unit.py with 6 unit tests that run without
a live TM1 server, covering both fixes.

Signed-off-by: kaizeenn <khairil0153@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes two long-standing CellService issues: (1) clear_with_dataframe incorrectly overwrote MDX selections when a dimension was mapped to multiple hierarchies, and (2) execute_mdx_dataframe_pivot did not accept/forward keyword arguments needed by downstream extraction helpers.

Changes:

  • Update clear_with_dataframe to accumulate multiple hierarchy selections via MdxHierarchySet.union() so all hierarchies contribute to the MDX (issue #1068), and fix the related error message to reference the correct key (column).
  • Update execute_mdx_dataframe_pivot to accept **kwargs and forward them to extract_cellset_dataframe_pivot (issue #1113).
  • Add unit tests covering both regressions without requiring a live TM1 server.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
TM1py/Services/CellService.py Fixes multi-hierarchy handling in clear_with_dataframe and forwards **kwargs in execute_mdx_dataframe_pivot.
Tests/test_cellservice_unit.py Adds regression tests for multi-hierarchy MDX generation and kwargs forwarding.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

"""

import unittest
from unittest.mock import MagicMock, patch, call
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.

Passing kwargs to CellService.execute_mdx_dataframe_pivot Bug in clear_with_dataframe() when using multiple hierarchies in dimension_mapping

2 participants