Skip to content

Comments

[BI Data Mapper] Re-enable array aggregating option in array mappings with type based aggregate options support #879

Merged
KCSAbeywickrama merged 10 commits intowso2:mainfrom
KCSAbeywickrama:bi-dm-aggr
Nov 24, 2025
Merged

[BI Data Mapper] Re-enable array aggregating option in array mappings with type based aggregate options support #879
KCSAbeywickrama merged 10 commits intowso2:mainfrom
KCSAbeywickrama:bi-dm-aggr

Conversation

@KCSAbeywickrama
Copy link
Contributor

@KCSAbeywickrama KCSAbeywickrama commented Nov 11, 2025

Purpose

Resolve wso2/product-ballerina-integrator#1795

Screen.Recording.2025-11-12.at.12.39.58.PM.mov

Summary by CodeRabbit

  • Enhanced Features
    • Mapping options menu now generates contextual suggestions based on source and target data types instead of a fixed set.
    • Improved array-to-singleton handling with conditional aggregation and an option to extract a single element when appropriate.
    • Type-aware aggregation: numeric sources get numeric aggregates, string sources get join operations, and matching primitive types enable direct aggregate-and-map options.
    • Menu choices are computed dynamically to reflect pending mapping intent.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 11, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Replaces static mapping-menu definitions with type-aware, dynamically generated menu items; adds a numeric-type helper to drive conditional aggregate and array-to-singleton options based on source/target types.

Changes

Cohort / File(s) Summary
Mapping options widget
workspaces/ballerina/data-mapper/src/components/Diagram/Label/MappingOptionsWidget.tsx
Removes static a2s/a2sAggregate definitions and aggregate wiring; adds imports for InputOutputPortModel and type utilities; introduces genAggregateItems, genArrayToSingletonItems, and genMenuItems to compute menuItems dynamically based on mapping kind and type compatibility; replaces static menuItems with computed result.
Type utilities
workspaces/ballerina/data-mapper/src/components/Diagram/utils/type-utils.ts
Adds exported isNumericType(typeKind: TypeKind): boolean which normalizes to a generic type kind and detects Int/Float/Decimal numeric kinds.
Manifest
package.json
Listed in diff manifest (no logical changes to package config).

Sequence Diagram(s)

sequenceDiagram
    participant UI as User Interface
    participant Widget as MappingOptionsWidget
    participant Utils as Type Utils
    participant Menu as Menu Generator

    UI->>Widget: Open mapping options for mapping
    Widget->>Menu: genMenuItems(mappingType, sourcePort, targetPort)
    alt ArrayToSingletonAggregate
        Menu->>Utils: isNumericType(sourceMemberType)
        Utils-->>Menu: boolean
        Menu-->>Widget: numeric/string aggregates + common (first,last)
    else ArrayToSingleton
        Menu->>Utils: compare sourceMemberType vs targetType (isPrimitive?)
        Utils-->>Menu: boolean(s)
        Menu-->>Widget: Extract-single + optional Aggregate-and-map
    else ArrayToArray / Default
        Menu-->>Widget: Default menu items
    end
    Widget-->>UI: Render filtered menu
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Areas to focus:
    • MappingOptionsWidget.tsx: branching in genAggregateItems, genArrayToSingletonItems, and genMenuItems.
    • Type compatibility checks: isNumericType and primitive/type-equality checks for generic/nullable cases.
    • Ensure existing event handlers and menu wiring remain intact after replacing static lists.

Suggested reviewers

  • hevayo
  • gigara

Poem

🐇
I hop through types and prune the view,
sums and joins appear when true,
single picks and aggregates align,
menu sprouts choices, small and fine,
mapping hops — a tidy cue. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is incomplete; it only provides a purpose reference to issue #1795 and a video link, missing Goals, Approach, and other required sections from the template. Complete the PR description by adding Goals (solutions for the problem), Approach (implementation details), and other required template sections like release notes and test coverage information.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main changes: re-enabling array aggregating options and adding type-based filtering support for aggregation functions.
Linked Issues check ✅ Passed The code changes successfully implement the objectives from issue #1795 by adding type-aware filtering logic for aggregate options based on source data types.
Out of Scope Changes check ✅ Passed All changes are directly scoped to implementing type-based aggregate filtering in the data mapper, with new utility functions and dynamic option generation aligned to the linked issue requirements.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0fa32cb and c7393b0.

📒 Files selected for processing (1)
  • workspaces/ballerina/data-mapper/src/components/Diagram/Label/MappingOptionsWidget.tsx (3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-13T05:16:29.503Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 879
File: workspaces/ballerina/data-mapper/src/components/Diagram/Label/MappingOptionsWidget.tsx:168-172
Timestamp: 2025-11-13T05:16:29.503Z
Learning: In Ballerina langlib functions, qualified identifiers use a leading apostrophe. The correct syntax is `module:'function`, for example `string:'join` not `string:join`. This applies to all langlib module functions in Ballerina.

Applied to files:

  • workspaces/ballerina/data-mapper/src/components/Diagram/Label/MappingOptionsWidget.tsx
🧬 Code graph analysis (1)
workspaces/ballerina/data-mapper/src/components/Diagram/Label/MappingOptionsWidget.tsx (1)
workspaces/ballerina/data-mapper/src/components/Diagram/utils/type-utils.ts (3)
  • isNumericType (94-99)
  • getGenericTypeKind (73-92)
  • isPrimitive (63-71)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b53a11f and 7f3cffe.

📒 Files selected for processing (2)
  • workspaces/ballerina/data-mapper/src/components/Diagram/Label/MappingOptionsWidget.tsx (3 hunks)
  • workspaces/ballerina/data-mapper/src/components/Diagram/utils/type-utils.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
workspaces/ballerina/data-mapper/src/components/Diagram/Label/MappingOptionsWidget.tsx (1)
workspaces/ballerina/data-mapper/src/components/Diagram/utils/type-utils.ts (3)
  • isNumericType (94-99)
  • getGenericTypeKind (73-92)
  • isPrimitive (63-71)
🔇 Additional comments (5)
workspaces/ballerina/data-mapper/src/components/Diagram/utils/type-utils.ts (1)

94-99: LGTM! Clean type utility function.

The implementation correctly identifies numeric types by normalizing to generic types and checking for Int, Float, or Decimal. This follows the same pattern as the existing isPrimitive function and integrates well with the PR's objective of type-aware filtering.

workspaces/ballerina/data-mapper/src/components/Diagram/Label/MappingOptionsWidget.tsx (4)

31-32: LGTM! Imports align with new functionality.

The added imports support the type-aware menu generation logic introduced in this PR.


213-224: LGTM! Clean switch logic for menu selection.

The function correctly routes to the appropriate menu generator based on mapping type. However, ensure the issues in genAggregateItems() and genArrayToSingletonItems() are addressed, as they are called from here.


226-226: LGTM! Dynamic menu generation enables type-aware filtering.

This change successfully replaces static menu items with runtime-computed options, fulfilling the PR's objective of showing type-compatible aggregates.


168-186: Add defensive type guards for port attributes to prevent runtime errors.

The function casts link.getSourcePort() without null checks, then immediately accesses nested properties. While the code may execute in a valid context, defensive checks align with patterns used elsewhere in the codebase (e.g., modification-utils.ts:32-34). Consider applying guards:

 const genAggregateItems = () => {
     const aggregateFnsNumeric = ["sum", "avg", "min", "max"];
     const aggregateFnsString = ["string:'join"];
     const aggregateFnsCommon = ["first", "last"];

     const sourcePort = link.getSourcePort() as InputOutputPortModel;
+    if (!sourcePort?.attributes?.field) {
+        return [];
+    }
     const sourceType = sourcePort.attributes.field.kind;

Apply the same pattern to genArrayToSingletonItems() at lines 197–200.

Copy link
Contributor

Copilot AI left a comment

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 re-enables array aggregation options in the data mapper with type-based aggregate function suggestions. Instead of showing static aggregate options, the menu now dynamically generates contextual suggestions based on the source array element types and target types.

Key changes:

  • Added isNumericType utility function to identify numeric types (Int, Float, Decimal)
  • Refactored static menu items into dynamic generation functions that inspect source/target port types
  • Implemented type-aware aggregation: numeric types get sum/avg/min/max, string types get join, and all types get first/last options

Reviewed Changes

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

File Description
workspaces/ballerina/data-mapper/src/components/Diagram/utils/type-utils.ts Added isNumericType utility function to identify numeric types for type-based aggregate selection
workspaces/ballerina/data-mapper/src/components/Diagram/Label/MappingOptionsWidget.tsx Refactored static aggregate menus into dynamic generation functions (genAggregateItems, genArrayToSingletonItems) that provide contextual options based on data types

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
workspaces/ballerina/data-mapper/src/components/Diagram/Label/MappingOptionsWidget.tsx (1)

174-179: Element type must drive aggregate options.

sourcePort.attributes.field.kind is the array type itself (e.g., TypeKind.Array), so isNumericType(sourceType) always returns false. As a result the numeric/string-specific aggregates never show up, defeating the goal of filtering the menu by element type. Please derive the element kind via field.member?.kind (falling back to field.kind for non-arrays) before running the type checks.

-        const sourceType = sourcePort.attributes.field.kind;
+        const sourceType = sourcePort.attributes.field.member?.kind ?? sourcePort.attributes.field.kind;
 
-        const aggregateFns = [...(isNumericType(sourceType) ? aggregateFnsNumeric :
-            getGenericTypeKind(sourceType) === TypeKind.String ? aggregateFnsString :
-                []),
+        const aggregateFns = [...(sourceType && isNumericType(sourceType) ? aggregateFnsNumeric :
+            sourceType && getGenericTypeKind(sourceType) === TypeKind.String ? aggregateFnsString :
+                []),
         ...aggregateFnsCommon];
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7f3cffe and 0fa32cb.

📒 Files selected for processing (1)
  • workspaces/ballerina/data-mapper/src/components/Diagram/Label/MappingOptionsWidget.tsx (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
workspaces/ballerina/data-mapper/src/components/Diagram/Label/MappingOptionsWidget.tsx (1)
workspaces/ballerina/data-mapper/src/components/Diagram/utils/type-utils.ts (3)
  • isNumericType (94-99)
  • getGenericTypeKind (73-92)
  • isPrimitive (63-71)

@KCSAbeywickrama KCSAbeywickrama merged commit 816940a into wso2:main Nov 24, 2025
6 checks passed
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.

[Data Mapper] Re-enable array aggregating option in array mappings with type based aggregate options support

3 participants