Fix Double -> Long conversion after Parquet migration - #121
Fix Double -> Long conversion after Parquet migration#121kevindetry-milaboratories wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a bug where float-formatted integer strings (e.g., "192.0") are nulled during strict String-to-Long casting in importFileParquet. It introduces a stripDecimal regular expression preprocessor and applies it to several columns, including totalReadsCountInTree, parentId, and readCount. The reviewer notes that several other Long-typed columns (such as mutation counts and unique cell/molecule counts) that allow NA or are emitted as doubles will likely suffer from the same issue and recommends applying the stripDecimal preprocessor to them as well.
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.
| // These columns reach importFileParquet float-formatted ("192.0"); its strict | ||
| // String->Long cast then nulls the whole column, and an all-null parentId makes | ||
| // the tree viewer show "multiple roots". Strip the fractional part before the | ||
| // cast to coerce them back to Long. Two independent causes converge here: | ||
| // - parentId: MiXCR emits a clean integer, but the Pandas-based ptransform | ||
| // aggregation (tables-aggregation.lib.tengo) promotes any NA-bearing integer | ||
| // column to float, and tree roots have an empty parent. | ||
| // - readCount / totalReadsCountInTree: MiXCR emits these as doubles itself. | ||
| // This truncates; it is not mixcr-clonotyping's numeric round().cast (that needs | ||
| // a pt step). Equivalent only because every value here is integer-valued (X.0). | ||
| stripDecimal := [{ type: `regexpReplace`, pattern: `^(\d+)\.\d+$`, replacement: `$1` }] |
There was a problem hiding this comment.
Any other Long column that allows NA and goes through the ptransform aggregation (or is emitted as double by MiXCR) will also suffer from the same issue where it is converted to float and then nulled during the strict String->Long cast in importFileParquet.\n\nThe following columns should also have preProcess: stripDecimal applied:\n1. In shmTreeNodes:\n - nMutationCount...BasedOnMrca (line 507)\n - aaMutationCount...BasedOnMrca (line 523)\n - nMutationCount...BasedOnGermline (line 539)\n - aaMutationCount...BasedOnGermline (line 555)\n2. In shmTreeNodesWithClones:\n - uniqueCellCount (line 775)\n - uniqueMoleculeCount (line 789)\n\nSince these lines are not all in the active diff hunks, please make sure to apply preProcess: stripDecimal to them in a follow-up or by expanding this PR.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // - readCount / totalReadsCountInTree: MiXCR emits these as doubles itself. | ||
| // This truncates; it is not mixcr-clonotyping's numeric round().cast (that needs | ||
| // a pt step). Equivalent only because every value here is integer-valued (X.0). | ||
| stripDecimal := [{ type: `regexpReplace`, pattern: `^(\d+)\.\d+$`, replacement: `$1` }] |
There was a problem hiding this comment.
The pattern ^(\d+)\.\d+$ only matches decimal-point notation (e.g. "192.0"). Pandas/NumPy can switch to scientific notation for large counts (e.g. "1.5e+09") when the value exceeds a display threshold. If that happens, the regex silently fails to strip the decimal, the downstream strict String→Long cast receives an unmodified scientific-notation string, and the entire column is nulled — the same root problem this PR is fixing. Read counts and tree-level counts are typically small enough that this is unlikely in practice, but it's worth validating that no real-world dataset can produce scientific-notation output from the Pandas aggregation step.
Prompt To Fix With AI
This is a comment left during a code review.
Path: workflow/src/export-settings.lib.tengo
Line: 34
Comment:
**Regex doesn't handle scientific notation**
The pattern `^(\d+)\.\d+$` only matches decimal-point notation (e.g. "192.0"). Pandas/NumPy can switch to scientific notation for large counts (e.g. "1.5e+09") when the value exceeds a display threshold. If that happens, the regex silently fails to strip the decimal, the downstream strict String→Long cast receives an unmodified scientific-notation string, and the entire column is nulled — the same root problem this PR is fixing. Read counts and tree-level counts are typically small enough that this is unlikely in practice, but it's worth validating that no real-world dataset can produce scientific-notation output from the Pandas aggregation step.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Greptile Summary
This PR patches a data-corruption regression introduced by the Parquet migration: integer columns (
parentId,readCount,totalReadsCountInTree) were arriving atimportFileParquetas float-formatted strings (e.g.\"192.0\"), causing its strict String→Long cast to null the entire column and making the tree viewer display "multiple roots".stripDecimalpreprocessing rule — a singleregexpReplacethat strips.0suffixes before the Long cast — and applies it to the four affected column definitions across all four exporter functions.parentId), while MiXCR itself emitsreadCount/totalReadsCountInTreeas doubles.Confidence Score: 4/5
The fix is targeted and well-explained; the four patched columns are the ones confirmed to arrive as float-formatted strings after Parquet migration.
The stripDecimal regex only handles decimal notation and silently passes through scientific-notation strings unchanged. Additionally, the comment's own statement that Pandas promotes any NA-bearing integer column to float raises the question of whether other Long columns in the same tables are equally broken but not yet fixed. Both are speculative rather than confirmed regressions.
workflow/src/export-settings.lib.tengo — verify whether the other NA-bearing Long columns need the same preProcess step, and whether scientific-notation output is possible for any of the patched columns.
Important Files Changed
stripDecimalregex preProcess tototalReadsCountInTree,parentId,readCount(x2) to fix null Long columns after Parquet migration; other NA-bearing Long columns may need the same treatment.minorfor the Parquet double-to-Long fix.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant MiXCR participant Pandas as Pandas aggregation participant Parquet as importFileParquet participant Viewer as Tree Viewer MiXCR->>Pandas: readCount/totalReadsCountInTree as Double MiXCR->>Pandas: parentId as Integer, NA for tree roots Pandas->>Parquet: Float-formatted strings (192.0) Note over Parquet: strict String->Long cast fails -> null Parquet-->>Viewer: all-null parentId / readCount Viewer-->>Viewer: multiple roots error Note over Parquet: After this PR Pandas->>Parquet: Float-formatted strings (192.0) Note over Parquet: stripDecimal: 192.0 -> 192 Parquet->>Viewer: Long values (192, 5...) Viewer->>Viewer: correct tree topology%%{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"}}}%% sequenceDiagram participant MiXCR participant Pandas as Pandas aggregation participant Parquet as importFileParquet participant Viewer as Tree Viewer MiXCR->>Pandas: readCount/totalReadsCountInTree as Double MiXCR->>Pandas: parentId as Integer, NA for tree roots Pandas->>Parquet: Float-formatted strings (192.0) Note over Parquet: strict String->Long cast fails -> null Parquet-->>Viewer: all-null parentId / readCount Viewer-->>Viewer: multiple roots error Note over Parquet: After this PR Pandas->>Parquet: Float-formatted strings (192.0) Note over Parquet: stripDecimal: 192.0 -> 192 Parquet->>Viewer: Long values (192, 5...) Viewer->>Viewer: correct tree topologyPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "Fix Double -> Long conversion after Parq..." | Re-trigger Greptile
Context used: