Skip to content

[ZEPPELIN-6471] Share a single aggregation-type constant between table and pivot#5313

Merged
tbonelee merged 1 commit into
apache:masterfrom
kimyenac:ZEPPELIN-6471
Jul 19, 2026
Merged

[ZEPPELIN-6471] Share a single aggregation-type constant between table and pivot#5313
tbonelee merged 1 commit into
apache:masterfrom
kimyenac:ZEPPELIN-6471

Conversation

@kimyenac

@kimyenac kimyenac commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

What is this PR for?

The Angular frontend hardcoded the same aggregation-type set (count, sum, min, max, avg) in two independent places — the table visualization and the pivot setting UI — with no single source of truth. The table component declared a local, unexported AggregationType union plus a value array; the pivot component had only an untyped string[] with the same values in a different order. Duplication meant the two could silently drift, and type safety was inconsistent.

This PR extracts a single shared type and has both components consume it, while each component keeps its own display-order array.

What changes were proposed in this PR?

  • Add common/util/aggregation-type.ts exporting AggregationType as the single shared type for the aggregation-type set.
  • table-visualization.component.ts: remove the local, unexported AggregationType type and import the shared one. aggregations keeps its existing order and is now typed readonly AggregationType[].
  • pivot-setting.component.ts: replace the untyped string[] with readonly AggregationType[] from the shared type, keeping its existing order.

Both usages are read-only iteration/display (@for), so readonly is safe. The table's switch (opt.aggregation) stays exhaustive against the shared type.

Why share the type only (not a shared value array)

Both arrays drive an aggregation dropdown in the UI:

  • pivot: pivot-setting.component.html @for (aggregate of aggregates; ...)
  • table: table-visualization.component.html:112 @for (aggregation of aggregations; ...) rendered via {{ aggregation | titlecase }}

The two arrays use different orders. Collapsing them onto one canonical array would reorder the table column's Aggregation dropdown (Count, Sum, Min, Max, Avg → Sum, Count, Avg, Min, Max) — cosmetic, but a visible change the issue did not intend. Sharing only the type removes the duplicated/untyped definition and guarantees the two sets stay in sync, while leaving both dropdowns exactly as they are today — no user-visible change.

How should this be tested?

There is no frontend unit-test infrastructure in this project, so regression is confirmed via lint + build:

cd zeppelin-web-angular && npm run lint && npm run build:angular

Both pass (0 lint errors, successful production build). Manually verified that both aggregation dropdowns are unchanged:

  • pivot setting dropdown still shows Sum, Count, Avg, Min, Max
  • table column Aggregation dropdown still shows Count, Sum, Min, Max, Avg

Questions

  • Does the license files need to update? No
  • Is there breaking change for older versions? No
  • Does this needs documentation? No

@tbonelee

Copy link
Copy Markdown
Contributor

Thanks for this, and to be clear up front: this PR faithfully implements the issue as I wrote it, so the gap is in my issue description, not your work. I've added a correction on ZEPPELIN-6471.

The one thing to fix here: the description (following the issue) says the table array "does not drive any display," but it does. aggregations is rendered as a dropdown in table-visualization.component.html:112:

@for (aggregation of aggregations; track aggregation) {
  <li nz-menu-item ...>{{ aggregation | titlecase }}</li>
}

So adopting the pivot's order as canonical keeps the pivot dropdown unchanged but reorders the table column's Aggregation dropdown: Count, Sum, Min, Max, Avg becomes Sum, Count, Avg, Min, Max. It is cosmetic, not a functional bug.

Since the issue's goal was no unintended visible change, the simplest way to get there is to share the type only and let each component keep its own order array (typed readonly AggregationType[]). That drops the untyped string[], keeps the table's switch exhaustive, and leaves both dropdowns exactly as they are today. Could you also update the description's "no visible impact" line and extend the manual check to the table dropdown?

…d pivot

Extract the AggregationType type into common/util/aggregation-type.ts as the
single shared definition for the aggregation-type set. Both table-visualization
and pivot-setting now import this type; the table's local, unexported
AggregationType and the pivot's untyped string[] are removed.

Only the type is shared, not a value array: each component keeps its own
display-order array (now typed readonly AggregationType[]). Both arrays drive an
aggregation dropdown, and they use different orders, so collapsing them onto one
canonical array would reorder the table column's Aggregation dropdown. Sharing
just the type keeps both dropdowns exactly as they are today (zero user-visible
change) while removing the duplicated/untyped definition and keeping the table's
switch exhaustive.
@kimyenac

Copy link
Copy Markdown
Contributor Author

@tbonelee Thanks for the review! You're right. I was following the issue's premise that the table array "doesn't drive any display," but table-visualization.component.html:112 renders it as the Aggregation dropdown — so a shared canonical order would have silently reordered it (Count, Sum, Min, Max, Avg → Sum, Count, Avg, Min, Max).

Fixed by sharing the type only. aggregation-type.ts now exports just AggregationType, and each component keeps its own order array typed readonly AggregationType[]. This drops the untyped string[], keeps the table's switch exhaustive, and leaves both dropdowns unchanged. I've also updated the description and extended the manual check to the table dropdown.

Force-pushed. Thanks!

@tbonelee
tbonelee merged commit 63e32d7 into apache:master Jul 19, 2026
17 of 18 checks passed
@tbonelee

Copy link
Copy Markdown
Contributor

Merged into master

gyowoo1113 pushed a commit to gyowoo1113/zeppelin that referenced this pull request Jul 19, 2026
…e and pivot

### What is this PR for?

The Angular frontend hardcoded the same aggregation-type set (count, sum, min, max, avg) in two independent places — the table visualization and the pivot setting UI — with no single source of truth. The table component declared a local, unexported `AggregationType` union plus a value array; the pivot component had only an untyped `string[]` with the same values in a different order. Duplication meant the two could silently drift, and type safety was inconsistent.

This PR extracts a single shared **type** and has both components consume it, while each component keeps its own display-order array.

### What changes were proposed in this PR?

- Add `common/util/aggregation-type.ts` exporting `AggregationType` as the single shared type for the aggregation-type set.
- `table-visualization.component.ts`: remove the local, unexported `AggregationType` type and import the shared one. `aggregations` keeps its existing order and is now typed `readonly AggregationType[]`.
- `pivot-setting.component.ts`: replace the untyped `string[]` with `readonly AggregationType[]` from the shared type, keeping its existing order.

Both usages are read-only iteration/display (`<at>for`), so `readonly` is safe. The table's `switch (opt.aggregation)` stays exhaustive against the shared type.

#### Why share the type only (not a shared value array)

Both arrays drive an aggregation dropdown in the UI:
- pivot: `pivot-setting.component.html` `<at>for (aggregate of aggregates; ...)`
- table: `table-visualization.component.html:112` `<at>for (aggregation of aggregations; ...)` rendered via `{{ aggregation | titlecase }}`

The two arrays use different orders. Collapsing them onto one canonical array would reorder the table column's Aggregation dropdown (Count, Sum, Min, Max, Avg → Sum, Count, Avg, Min, Max) — cosmetic, but a visible change the issue did not intend. Sharing only the type removes the duplicated/untyped definition and guarantees the two sets stay in sync, while leaving **both dropdowns exactly as they are today** — no user-visible change.

### How should this be tested?

There is no frontend unit-test infrastructure in this project, so regression is confirmed via lint + build:

```
cd zeppelin-web-angular && npm run lint && npm run build:angular
```

Both pass (0 lint errors, successful production build). Manually verified that both aggregation dropdowns are unchanged:
- pivot setting dropdown still shows `Sum, Count, Avg, Min, Max`
- table column Aggregation dropdown still shows `Count, Sum, Min, Max, Avg`

### Questions

- Does the license files need to update? No
- Is there breaking change for older versions? No
- Does this needs documentation? No


Closes apache#5313 from kimyenac/ZEPPELIN-6471.

Signed-off-by: ChanHo Lee <chanholee@apache.org>
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.

2 participants