Skip to content

Fix Double -> Long conversion after Parquet migration - #122

Closed
kevindetry-milaboratories wants to merge 1 commit into
mainfrom
push-rwzqpwqtlkux
Closed

Fix Double -> Long conversion after Parquet migration#122
kevindetry-milaboratories wants to merge 1 commit into
mainfrom
push-rwzqpwqtlkux

Conversation

@kevindetry-milaboratories

@kevindetry-milaboratories kevindetry-milaboratories commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a type-conversion regression introduced by the Parquet migration, where integer columns (Long/Int) arrive float-formatted (e.g., "192.0") — either because Pandas promotes NA-bearing integer columns to float or because MiXCR emits certain counts as doubles. A module-level stripDecimal regex step and a stripDecimalOnIntColumns helper are added; the helper is called in all four export functions (shmTree, shmTreeNodes, shmTreeNodesWithClones, shmTreeNodesUniqueIsotype) before their pfconvParams is returned.

  • stripDecimal — new module-level constant: a single-element preProcess array containing a regexpReplace that strips a trailing .digits suffix (e.g., "192.0""192"), making the value safe for the subsequent strict-cast to Long/Int.
  • stripDecimalOnIntColumns — new helper function that iterates all axes (spec.type) and columns (spec.valueType) and appends stripDecimal to any Long/Int entry's preProcess, preserving any pre-existing steps.
  • All four export functions now call stripDecimalOnIntColumns(axes, columns) immediately before their return statement, ensuring every Long/Int field across all table shapes is covered without a fragile hand-picked column list.

Confidence Score: 4/5

Safe to merge; the fix is well-scoped and all four export functions are updated consistently.

The logic change is small and correct — the regex handles the described float-formatted-integer cases and is a no-op for already-clean integers. All four export functions are covered. The only note is a changeset version bump (minor vs patch) that has no runtime impact but affects release semantics.

.changeset/soft-ghosts-shake.md — version type may want a second look before the release is cut.

Important Files Changed

Filename Overview
.changeset/soft-ghosts-shake.md Changeset entry classifying the fix as minor — conventionally a patch for a bug fix, but may be intentional given the data-behaviour change introduced by the Parquet migration.
workflow/src/export-settings.lib.tengo Adds stripDecimal regex step and stripDecimalOnIntColumns helper; correctly wired into all four export functions (shmTree, shmTreeNodes, shmTreeNodesWithClones, shmTreeNodesUniqueIsotype) before their return statements.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Export function called\n(shmTree / shmTreeNodes /\nshmTreeNodesWithClones /\nshmTreeNodesUniqueIsotype)"] --> B["Build axes[] and columns[]\nwith Long/Int specs"]
    B --> C["stripDecimalOnIntColumns(axes, columns)"]
    C --> D{"For each axis:\nspec.type == Long or Int?"}
    D -- Yes --> E["Append stripDecimal\nregexpReplace to axis.preProcess\ne.g. '192.0' → '192'"]
    D -- No --> F["Leave axis unchanged"]
    E --> G{"For each column:\nspec.valueType == Long or Int?"}
    F --> G
    G -- Yes --> H["Append stripDecimal\nregexpReplace to col.preProcess\ne.g. '99.0' → '99'"]
    G -- No --> I["Leave column unchanged"]
    H --> J["Return pfconvParams\nwith patched axes + columns"]
    I --> J
    J --> K["xsv.importFileMap reads Parquet\nString → stripDecimal → strict-cast Long/Int"]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["Export function called\n(shmTree / shmTreeNodes /\nshmTreeNodesWithClones /\nshmTreeNodesUniqueIsotype)"] --> B["Build axes[] and columns[]\nwith Long/Int specs"]
    B --> C["stripDecimalOnIntColumns(axes, columns)"]
    C --> D{"For each axis:\nspec.type == Long or Int?"}
    D -- Yes --> E["Append stripDecimal\nregexpReplace to axis.preProcess\ne.g. '192.0' → '192'"]
    D -- No --> F["Leave axis unchanged"]
    E --> G{"For each column:\nspec.valueType == Long or Int?"}
    F --> G
    G -- Yes --> H["Append stripDecimal\nregexpReplace to col.preProcess\ne.g. '99.0' → '99'"]
    G -- No --> I["Leave column unchanged"]
    H --> J["Return pfconvParams\nwith patched axes + columns"]
    I --> J
    J --> K["xsv.importFileMap reads Parquet\nString → stripDecimal → strict-cast Long/Int"]
Loading

Fix All in Claude Code

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
.changeset/soft-ghosts-shake.md:2-3
Changeset version type is `minor` for what the title and description call a bug fix. Semver convention is `patch` for a fix that doesn't add or change public API surface. Using `minor` here will bump the minor version of both packages unnecessarily. If the intention is simply to fix the Double→Long conversion regression, consider downgrading to `patch`.

```suggestion
'@platforma-open/milaboratories.mixcr-shm-trees.workflow': patch
'@platforma-open/milaboratories.mixcr-shm-trees': patch
```

Reviews (1): Last reviewed commit: "Fix Double -> Long conversion after Parq..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a helper function stripDecimalOnIntColumns to strip trailing decimals from integer columns and axes, preventing conversion failures after Parquet migration. The helper is applied across several tree-export functions. Feedback highlights a potential issue where negative integers represented as floats (e.g., -1.0) would fail to match the current regular expression pattern, and suggests updating the pattern to support negative numbers.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread workflow/src/export-settings.lib.tengo Outdated
Comment thread .changeset/soft-ghosts-shake.md
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.

1 participant