Migrate to ptabler #123
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| --- | ||
| '@platforma-open/milaboratories.mixcr-shm-trees.workflow': minor | ||
| '@platforma-open/milaboratories.mixcr-shm-trees': minor | ||
| --- | ||
|
|
||
| Replace ptransform with ptabler for all table aggregation | ||
|
|
||
| The by-nodes deduplication and the sequence-of-interest per-tree | ||
| aggregation now run in-process via ptabler instead of shelling out to | ||
| the ptransform binary. Integer columns are coerced back to their | ||
| declared types after the TSV read, so they are no longer float-promoted | ||
| (e.g. `192` -> `192.0`) and nulled by the Parquet importer. The | ||
| software-ptransform dependency is removed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,82 +1,65 @@ | ||
| ll := import("@platforma-sdk/workflow-tengo:ll") | ||
| pt := import("@platforma-sdk/workflow-tengo:pt") | ||
| slices := import("@platforma-sdk/workflow-tengo:slices") | ||
|
|
||
| // Column name + declared type for every axis and value column, so ensureUniqueness | ||
| // knows which columns to coerce back to an integer type after reading the | ||
| // intermediate TSV as all-String. | ||
| ensureUniquenessParamsFromPconvParams := func(pfConvParams) { | ||
| return { | ||
| axes: slices.map(pfConvParams.axes, func(axis) { | ||
| return { column: axis.column, type: axis.spec.type } | ||
| }), | ||
| columns: slices.map(pfConvParams.columns, func(col) { | ||
| return { column: col.column, type: col.spec.valueType } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| /** Aggregating by-nodes output to make it uniquely addressable by its native key. */ | ||
| ensureUniqueness := func(inputTsv, params, ...aggParams) { | ||
| allCols := [] | ||
| for axis in params.axes { | ||
| allCols = append(allCols, axis) | ||
| } | ||
| for col in params.columns { | ||
| allCols = append(allCols, col) | ||
| } | ||
|
|
||
| keyNames := slices.map(params.axes, func(axis) { return axis.column }) | ||
|
|
||
| wf := pt.workflow() | ||
|
|
||
| df := wf.frame({ file: inputTsv, xsvType: "tsv" }, { inferSchema: false }) | ||
|
|
||
| intCasts := [] | ||
| for c in allCols { | ||
| if c.type == "Long" || c.type == "Int" { | ||
| intCasts = append(intCasts, pt.col(c.column).cast("Double").round().cast(c.type).alias(c.column)) | ||
| } | ||
| } | ||
| if len(intCasts) > 0 { | ||
| df = df.withColumns(intCasts...) | ||
| } | ||
|
|
||
| result := undefined | ||
| if len(aggParams) > 1 { | ||
| // max_by: keep, per key, the row with the greatest ranking column value. | ||
| rankCol := aggParams[1] | ||
| aggs := slices.map(params.columns, func(col) { | ||
| return pt.col(col.column).maxBy(pt.col(rankCol)).alias(col.column) | ||
| }) | ||
| result = df.groupBy(keyNames...).agg(aggs...) | ||
| } else { | ||
| // first: dedup rows by key, keeping the first. | ||
| result = df.unique({ subset: keyNames, keep: "first", maintainOrder: true }) | ||
| } | ||
|
Comment on lines
+53
to
+56
There was a problem hiding this comment. The previous implementation supported arbitrary aggregation types passed via
Comment on lines
+46
to
+56
There was a problem hiding this comment.
The old API used Prompt To Fix With AIThis is a comment left during a code review.
Path: workflow/src/tables-aggregation.lib.tengo
Line: 46-56
Comment:
**`aggParams[0]` type string silently ignored**
The old API used `aggParams[0]` as the aggregation-type discriminator ("max_by" or "first"). The new implementation discards it entirely and dispatches only on `len(aggParams) > 1`. A call like `ensureUniqueness(tsv, params, "max_by")` (type provided but ranking column omitted) now silently falls into the `df.unique()` branch instead of raising an error, and a hypothetical future caller passing a third aggregation type with two arguments would silently be treated as `max_by`. Since all current call sites happen to map 1:1 (1 arg → first, 2 args → max_by), there is no current regression, but the contract is no longer enforced.
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! |
||
|
|
||
| result.save("output.tsv") | ||
| return wf.run().getFile("output.tsv") | ||
| } | ||
|
|
||
| export ll.toStrict({ | ||
| ensureUniqueness: ensureUniqueness, | ||
| ensureUniquenessParamsFromPconvParams: ensureUniquenessParamsFromPconvParams | ||
| }) | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To prevent potential runtime panics, we should defensively check if
axis.specandcol.specare defined before accessing their properties (typeandvalueType). In Tengo, accessing properties on anundefinedvalue will cause a runtime panic.