You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "richardfogaca (via GitHub)" <gi...@apache.org> on 2025/12/04 01:55:00 UTC
[PR] Feat dynamic currency [superset]
richardfogaca opened a new pull request, #36416:
URL: https://github.com/apache/superset/pull/36416
### SUMMARY
Adds dynamic currency detection for chart formatting. When a metric's currency is set to "Auto-detect", Superset queries the dataset's designated currency code column to determine the appropriate currency symbol.
**How it works:**
- Dataset owners configure a `currency_code_column` (e.g., a column containing "USD", "EUR", "GBP")
- When building a chart, users select "Auto-detect" in the currency format dropdown
- Backend detects the currency from filtered data:
- Single currency → formats with that currency symbol
- Mixed currencies → falls back to neutral numeric formatting
**Key changes:**
**Backend:**
- Added `currency_code_column` field to SqlaTable model with DB migration
- Added `detect_currency()` utility that queries distinct currencies from filtered data
- Integrated currency detection into both QueryContext API and legacy viz API
- Returns `detected_currency` in API response for frontend consumption
**Frontend:**
- Added "Auto-detect" option to CurrencyControl (only shown when dataset has currency_code_column)
- Updated chart plugins to handle AUTO currency resolution: Timeseries, MixedTimeseries, BigNumber variants, Table, PivotTable, Heatmap, Sunburst, Treemap, Pie, Funnel, Gauge
- Refactored DatasourceEditor: moved `main_dttm_col` from inline radio buttons to a new "Default Column Settings" section alongside `currency_code_column`
**Example data:**
- Added `international_sales` example dataset demonstrating multi-currency transactions across regions
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - Requires screenshot of:
- Currency code column
<img width="1722" height="826" alt="Screenshot 2025-12-01 at 20 29 36" src="https://github.com/user-attachments/assets/df849a38-a08c-4b5f-a5eb-3cab2df3be68" />
- Currency control dropdown with "Auto-detect" option + Chart displaying auto-detected currency formatting
<img width="2402" height="1214" alt="Screenshot 2025-12-01 at 20 19 54 (1)" src="https://github.com/user-attachments/assets/6c9d7ba2-8e58-407b-9f7e-a9ccff58cf6c" />
### TESTING INSTRUCTIONS
1. Open the `international_sales` dataset and set "Currency code column" to `currency_code`
2. Create a new chart (e.g., Big Number Total) using this dataset
3. Select a numeric metric (e.g., `revenue`)
4. In the "Number Format" section, set currency to "Auto-detect"
5. Apply filters to show single-currency data (e.g., filter by `country = USA`)
6. Verify the chart displays USD formatting
7. Change filter to show mixed currencies (e.g., all countries)
8. Verify the chart falls back to neutral number formatting
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [x] Includes DB Migration (follow approval process in [SIP-193](https://github.com/apache/superset/issues/36276))
- [x] Migration is atomic, supports rollback & is backwards-compatible
- [x] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3629788841
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20048695859) for [5b992bb](https://github.com/apache/superset/commit/5b992bb)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3715709984
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! 🎉
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) ·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) ·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2665755385
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -41,6 +45,38 @@ export const getCurrencySymbol = (currency: Partial<Currency>) =>
.formatToParts(1)
.find(x => x.type === 'currency')?.value;
+/** Normalize currency to ISO 4217 format (e.g., "USD"). Returns null if invalid. */
+export function normalizeCurrency(
+ value: string | null | undefined,
+): string | null {
+ if (!value) return null;
+
+ const str = value.toString().trim();
+ if (!str) return null;
+
+ const upper = str.toUpperCase();
+
+ if (/^[A-Z]{3}$/.test(upper)) return upper;
+
+ return null;
+}
+
+/** Check if array contains multiple distinct currencies (after normalization). */
+export function hasMixedCurrencies(
+ currencies: (string | null | undefined)[],
+): boolean {
+ // Filter out null/undefined and normalize
+ const normalized = currencies
+ .map(c => normalizeCurrency(c))
+ .filter((c): c is string => c !== null);
+
+ if (normalized.length === 0) return false;
+
+ // Check if all normalized currencies are the same
+ const first = normalized[0];
+ return !normalized.every(c => c === first);
+}
Review Comment:
Just a suggestion , this function can be imrpoved with an early exit , See if it fits fine
export function hasMixedCurrencies(
currencies: (string | null | undefined)[],
): boolean {
let first: string | null = null;
for (const c of currencies) {
const normalized = normalizeCurrency(c);
if (normalized === null) continue;
if (first === null) {
first = normalized;
} else if (normalized !== first) {
return true;
}
}
return false;
}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2677853301
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/transformProps.ts:
##########
@@ -92,6 +100,10 @@ export default function transformProps(
columnFormats,
metricEntry?.d3format || yAxisFormat,
currencyFormat,
+ undefined,
Review Comment:
this would be a breaking change affecting all callers of getValueFormatter
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3743271186
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20952096861) for [594238e](https://github.com/apache/superset/commit/594238e)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3743645930
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3657632342
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20247344066) for [5b992bb](https://github.com/apache/superset/commit/5b992bb)
• **Environment:** http://52.12.115.249:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3657594224
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20247344066) for [5b992bb](https://github.com/apache/superset/commit/5b992bb)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho merged PR #36416:
URL: https://github.com/apache/superset/pull/36416
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3747086326
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20977162510) for [1e3e4a6](https://github.com/apache/superset/commit/1e3e4a6)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3743438857
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20952096861) for [594238e](https://github.com/apache/superset/commit/594238e)
• **Environment:** http://35.90.39.221:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2685647685
##########
superset/datasets/schemas.py:
##########
@@ -166,6 +166,7 @@ class DatasetPutSchema(Schema):
schema = fields.String(allow_none=True, validate=Length(0, 255))
description = fields.String(allow_none=True)
main_dttm_col = fields.String(allow_none=True)
+ currency_code_column = fields.String(allow_none=True, validate=Length(0, 250))
normalize_columns = fields.Boolean(allow_none=True, dump_default=False)
Review Comment:
nice catch thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3730855383
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! 🎉
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) ·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) ·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3731458463
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20868179636) for [c189974](https://github.com/apache/superset/commit/c189974)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat dynamic currency [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2587183996
##########
tests/unit_tests/datasets/schema_tests.py:
##########
@@ -46,3 +46,47 @@ def test_validate_python_date_format(payload) -> None:
def test_validate_python_date_format_raises(payload) -> None:
with pytest.raises(ValidationError):
validate_python_date_format(payload)
+
+
+def test_dataset_columns_put_schema_includes_currency_code() -> None:
+ """Test that DatasetColumnsPutSchema properly handles is_currency_code field."""
+ from superset.datasets.schemas import DatasetColumnsPutSchema
+
+ schema = DatasetColumnsPutSchema()
+
+ # Column marked as currency code
+ data = {
+ "column_name": "currency",
+ "is_currency_code": True,
+ }
+ result = schema.load(data)
+ assert result["is_currency_code"] is True
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing Schema Field</b></div>
<div id="fix">
The test expects `DatasetColumnsPutSchema` to handle the `is_currency_code` field, but it's not defined in the schema. Marshmallow ignores unknown fields during `load()`, so `result` won't include `is_currency_code`, causing the assertion to fail. Adding the field will fix this and enable API support for marking columns as currency codes.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- uuid = fields.UUID(allow_none=True)
+ uuid = fields.UUID(allow_none=True)
+ is_currency_code = fields.Boolean(allow_none=True)
```
</div>
</details>
</div>
<small><i>Code Review Run #4c2ba0</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/views/core.py:
##########
@@ -169,17 +169,20 @@ def get_raw_results(self, viz_obj: BaseViz) -> FlaskResponse:
payload = viz_obj.get_df_payload()
if viz_obj.has_error(payload):
return json_error_response(payload=payload, status=400)
- return self.json_response(
- {
- "data": payload["df"].to_dict("records")
- if payload["df"] is not None
- else [],
- "colnames": payload.get("colnames"),
- "coltypes": payload.get("coltypes"),
- "rowcount": payload.get("rowcount"),
- "sql_rowcount": payload.get("sql_rowcount"),
- },
- )
+ response = {
+ "data": payload["df"].to_dict("records")
+ if payload["df"] is not None
+ else [],
+ "colnames": payload.get("colnames"),
+ "coltypes": payload.get("coltypes"),
+ "rowcount": payload.get("rowcount"),
+ "sql_rowcount": payload.get("sql_rowcount"),
+ }
+ # Add detected currency for AUTO mode formatting
+ detected_currency = viz_obj._detect_currency() # pylint: disable=protected-access
+ if detected_currency:
+ response["detected_currency"] = detected_currency
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>API Response Inconsistency</b></div>
<div id="fix">
The response format is inconsistent: the new query system always includes 'detected_currency' (even as None), but this code only adds it when truthy. This could break clients expecting a consistent API shape across query paths.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
detected_currency = viz_obj._detect_currency() # pylint: disable=protected-access
response["detected_currency"] = detected_currency
````
</div>
</details>
</div>
<small><i>Code Review Run #4c2ba0</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/types.ts:
##########
@@ -68,6 +68,8 @@ interface PivotTableCustomizeProps {
rowSubTotals: boolean;
valueFormat: string;
currencyFormat: Currency;
+ currencyCodeColumn?: string;
+ detectedCurrency?: string;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Type Mismatch</b></div>
<div id="fix">
The detectedCurrency property type should be string | null to match the backend response type from ChartDataResponseResult, which can return null when multiple currencies are detected.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
detectedCurrency?: string | null;
````
</div>
</details>
</div>
<small><i>Code Review Run #4c2ba0</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/connectors/sqla/models_test.py:
##########
@@ -842,3 +842,42 @@ def test_quoted_name_prevents_double_quoting(mocker: MockerFixture) -> None:
# Should have each part quoted separately:
# GOOD: "MY_DB"."MY_SCHEMA"."MY_TABLE"
assert '"MY_DB"."MY_SCHEMA"."MY_TABLE"' in compiled
+
+
+def test_table_column_is_currency_code_property() -> None:
+ """
+ Test is_currency_code property on TableColumn.
+ """
+ column = TableColumn(
+ column_name="currency",
+ type="VARCHAR(3)",
+ is_currency_code=True,
+ )
+ assert column.is_currency_code is True
+
+
+def test_sqla_table_currency_code_column_property() -> None:
+ """
+ Test currency_code_column property on SqlaTable.
+ """
+ database = Database(database_name="my_db")
+ table = SqlaTable(
+ table_name="sales",
+ database=database,
+ currency_code_column="currency",
+ )
+ assert table.currency_code_column == "currency"
+
+
+def test_table_column_data_includes_currency_code_flag() -> None:
+ """
+ Test that .data property includes is_currency_code flag.
+ """
+ column = TableColumn(
+ column_name="currency",
+ type="VARCHAR(3)",
+ is_currency_code=True,
+ )
+ data = column.data
+ assert "is_currency_code" in data
+ assert data["is_currency_code"] is True
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing serialization attribute</b></div>
<div id="fix">
The test_table_column_data_includes_currency_code_flag test expects the .data property to include 'is_currency_code', but the data method's attrs tuple is missing this attribute. This causes the test to fail since the flag won't be serialized.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- "is_dttm",
- "python_date_format",
+ "is_dttm",
+ "is_currency_code",
+ "python_date_format",
```
</div>
</details>
</div>
<small><i>Code Review Run #4c2ba0</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2695757979
##########
superset/viz.py:
##########
@@ -475,6 +480,56 @@ def cache_key(self, query_obj: QueryObjectDict, **extra: Any) -> str:
json_data = self.json_dumps(cache_dict, sort_keys=True)
return hash_from_str(json_data)
+ @deprecated(deprecated_in="3.0")
Review Comment:
Good point @betodealmeida . Looking at the legacy charts that still use viz.py (NVD3, Deck.GL, geo maps, etc.), none of them would realistically use currency formatting. The charts that actually support currency (Table, Pivot Table, Big Number, Pie, Timeseries) all use the modern Chart Data API path where detection is already implemented in query_actions.py.
I'll remove the _detect_currency method from viz.py since it's not needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3747215606
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2683054304
##########
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:
##########
@@ -276,10 +287,25 @@ const processColumns = memoizeOne(function processColumns(
// percent metrics have a default format
formatter = getNumberFormatter(numberFormat || PERCENT_3_POINT);
} else if (isMetric || (isNumber && (numberFormat || currency))) {
- formatter = currency?.symbol
+ // Resolve AUTO currency when currency column isn't in query results
+ let resolvedCurrency = currency;
Review Comment:
Yes, this is backwards compatible. The new AUTO resolution logic only executes when currency?.symbol === 'AUTO'. For users with previously set static currencies (USD, EUR, etc.), the condition currency?.symbol === 'AUTO' evaluates to false, so the entire resolution block is skipped and the formatter is created exactly as before with their original currency. The only change in their code path is that we use resolvedCurrency (which equals currency unchanged) instead of currency directly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3746713994
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! 🎉
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) ·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) ·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3746925201
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2665779458
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -41,6 +45,38 @@ export const getCurrencySymbol = (currency: Partial<Currency>) =>
.formatToParts(1)
.find(x => x.type === 'currency')?.value;
+/** Normalize currency to ISO 4217 format (e.g., "USD"). Returns null if invalid. */
+export function normalizeCurrency(
Review Comment:
we can do a simplified version
const ISO_4217_REGEX = /^[A-Z]{3}$/;
export function normalizeCurrency(
value: string | null | undefined,
): string | null {
if (isUndefined(value) || isNull(value)) return null; lodash checks
const normalized = value.trim().toUpperCase();
return ISO_4217_REGEX.test(normalized) ? normalized : null;
}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2665765126
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -723,3 +723,133 @@ describe('legend sorting', () => {
]);
});
});
+
+describe('EchartsTimeseries AUTO Mode Currency', () => {
+ it('should detect single currency and resolve AUTO mode', () => {
+ const chartProps = new ChartProps<SqlaFormData>({
+ ...chartPropsConfig,
+ formData: {
+ ...formData,
+ currencyFormat: { symbol: 'AUTO', symbolPosition: 'prefix' },
+ },
+ datasource: {
+ currencyCodeColumn: 'currency_code',
+ columnFormats: {},
+ currencyFormats: {},
+ verboseMap: {},
+ },
+ queriesData: [
+ {
+ data: [
+ {
+ 'San Francisco': 1000,
+ __timestamp: 599616000000,
+ currency_code: 'USD',
+ },
+ {
+ 'San Francisco': 2000,
+ __timestamp: 599916000000,
+ currency_code: 'USD',
+ },
+ ],
+ },
+ ],
+ });
+
+ const transformed = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ // Y-axis formatter should contain $ for USD
+ const yAxisFormatter = (transformed.echartOptions.yAxis as any)?.axisLabel
+ ?.formatter;
+ if (yAxisFormatter) {
+ expect(yAxisFormatter(1000)).toContain('$');
+ }
+ });
+
+ it('should use neutral formatting for mixed currencies in AUTO mode', () => {
+ const chartProps = new ChartProps<SqlaFormData>({
+ ...chartPropsConfig,
+ formData: {
+ ...formData,
+ currencyFormat: { symbol: 'AUTO', symbolPosition: 'prefix' },
+ },
+ datasource: {
+ currencyCodeColumn: 'currency_code',
+ columnFormats: {},
+ currencyFormats: {},
+ verboseMap: {},
+ },
+ queriesData: [
+ {
+ data: [
+ {
+ 'San Francisco': 1000,
+ __timestamp: 599616000000,
+ currency_code: 'USD',
+ },
+ {
+ 'San Francisco': 2000,
+ __timestamp: 599916000000,
+ currency_code: 'EUR',
+ },
+ ],
+ },
+ ],
+ });
+
+ const transformed = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ // With mixed currencies, Y-axis should use neutral formatting
+ const yAxisFormatter = (transformed.echartOptions.yAxis as any)?.axisLabel
Review Comment:
Can we have a better type instead of any , same as above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2665775504
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -41,6 +45,38 @@ export const getCurrencySymbol = (currency: Partial<Currency>) =>
.formatToParts(1)
.find(x => x.type === 'currency')?.value;
+/** Normalize currency to ISO 4217 format (e.g., "USD"). Returns null if invalid. */
+export function normalizeCurrency(
+ value: string | null | undefined,
+): string | null {
+ if (!value) return null;
+
+ const str = value.toString().trim();
Review Comment:
if the value is already string and we have handled a null check do we need to do a toString
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2665805036
##########
superset-frontend/packages/superset-ui-core/src/currency-format/utils.ts:
##########
@@ -76,14 +147,40 @@ export const getValueFormatter = (
d3Format: string | undefined,
currencyFormat: Currency | undefined,
key?: string,
+ data?: Record<string, any>[],
+ currencyCodeColumn?: string,
+ detectedCurrency?: string | null,
) => {
+ let resolvedCurrency: Currency | undefined | null = currencyFormat;
+ if (currencyFormat?.symbol === 'AUTO') {
Review Comment:
constant for AUTO
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3746734666
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20973890833) for [fbd2663](https://github.com/apache/superset/commit/fbd2663)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3756660369
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! 🎉
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) ·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) ·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3756718346
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/21044774894) for [2a9bafe](https://github.com/apache/superset/commit/2a9bafe)
• **Environment:** http://34.216.82.197:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2665759943
##########
superset-frontend/packages/superset-ui-core/src/currency-format/utils.ts:
##########
@@ -25,21 +25,92 @@ import {
QueryFormMetric,
ValueFormatter,
} from '@superset-ui/core';
+import { normalizeCurrency, hasMixedCurrencies } from './CurrencyFormatter';
+
+/**
+ * Detect single currency in data. Returns ISO 4217 code if uniform, null if mixed.
+ */
+export const analyzeCurrencyInData = (
+ data: Record<string, any>[],
Review Comment:
Can we have a better type instead of any
##########
superset-frontend/packages/superset-ui-core/src/currency-format/utils.ts:
##########
@@ -25,21 +25,92 @@ import {
QueryFormMetric,
ValueFormatter,
} from '@superset-ui/core';
+import { normalizeCurrency, hasMixedCurrencies } from './CurrencyFormatter';
+
+/**
+ * Detect single currency in data. Returns ISO 4217 code if uniform, null if mixed.
+ */
+export const analyzeCurrencyInData = (
+ data: Record<string, any>[],
+ currencyColumn: string | undefined,
+): string | null => {
+ if (!currencyColumn || !data || data.length === 0) {
+ return null;
+ }
+
+ const currencies = data
+ .map(row => row[currencyColumn])
+ .filter(val => val !== null && val !== undefined);
+
+ if (currencies.length === 0) {
+ return null;
+ }
+
+ if (hasMixedCurrencies(currencies)) {
+ return null;
+ }
+
+ return normalizeCurrency(currencies[0]);
+};
+
+/** Resolve AUTO currency to detected value, null (mixed), or passthrough original. */
+export const resolveAutoCurrency = (
+ currencyFormat: Currency | undefined,
+ backendDetected: string | null | undefined,
+ data?: Record<string, any>[],
Review Comment:
Can we have a better type instead of any
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2665760409
##########
superset-frontend/packages/superset-ui-core/src/currency-format/utils.ts:
##########
@@ -25,21 +25,92 @@ import {
QueryFormMetric,
ValueFormatter,
} from '@superset-ui/core';
+import { normalizeCurrency, hasMixedCurrencies } from './CurrencyFormatter';
+
+/**
+ * Detect single currency in data. Returns ISO 4217 code if uniform, null if mixed.
+ */
+export const analyzeCurrencyInData = (
+ data: Record<string, any>[],
+ currencyColumn: string | undefined,
+): string | null => {
+ if (!currencyColumn || !data || data.length === 0) {
+ return null;
+ }
+
+ const currencies = data
+ .map(row => row[currencyColumn])
+ .filter(val => val !== null && val !== undefined);
+
+ if (currencies.length === 0) {
+ return null;
+ }
+
+ if (hasMixedCurrencies(currencies)) {
+ return null;
+ }
+
+ return normalizeCurrency(currencies[0]);
+};
+
+/** Resolve AUTO currency to detected value, null (mixed), or passthrough original. */
+export const resolveAutoCurrency = (
+ currencyFormat: Currency | undefined,
+ backendDetected: string | null | undefined,
+ data?: Record<string, any>[],
+ currencyCodeColumn?: string,
+): Currency | undefined | null => {
+ if (currencyFormat?.symbol !== 'AUTO') return currencyFormat;
+
+ const detectedCurrency =
+ backendDetected ??
+ (data && currencyCodeColumn
+ ? analyzeCurrencyInData(data, currencyCodeColumn)
+ : null);
+
+ if (detectedCurrency) {
+ return {
+ symbol: detectedCurrency,
+ symbolPosition: currencyFormat.symbolPosition,
+ };
+ }
+ return null; // Mixed currencies
+};
export const buildCustomFormatters = (
metrics: QueryFormMetric | QueryFormMetric[] | undefined,
savedCurrencyFormats: Record<string, Currency>,
savedColumnFormats: Record<string, string>,
d3Format: string | undefined,
- currencyFormat: Currency | undefined,
+ currencyFormat: Currency | undefined | null,
+ data?: Record<string, any>[],
Review Comment:
Can we have a better type instead of any
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2665809248
##########
superset-frontend/packages/superset-ui-core/src/currency-format/utils.ts:
##########
@@ -76,14 +147,40 @@ export const getValueFormatter = (
d3Format: string | undefined,
currencyFormat: Currency | undefined,
key?: string,
+ data?: Record<string, any>[],
+ currencyCodeColumn?: string,
+ detectedCurrency?: string | null,
) => {
+ let resolvedCurrency: Currency | undefined | null = currencyFormat;
+ if (currencyFormat?.symbol === 'AUTO') {
+ // Use backend-detected currency, or fallback to frontend analysis
+ if (detectedCurrency !== undefined) {
+ resolvedCurrency = detectedCurrency
+ ? {
+ symbol: detectedCurrency,
+ symbolPosition: currencyFormat.symbolPosition,
+ }
+ : null;
+ } else if (data && currencyCodeColumn) {
+ const frontendDetected = analyzeCurrencyInData(data, currencyCodeColumn);
+ resolvedCurrency = frontendDetected
+ ? {
+ symbol: frontendDetected,
+ symbolPosition: currencyFormat.symbolPosition,
+ }
+ : null;
+ } else {
+ resolvedCurrency = null;
+ }
+ }
+
const customFormatter = getCustomFormatter(
buildCustomFormatters(
metrics,
savedCurrencyFormats,
savedColumnFormats,
d3Format,
- currencyFormat,
+ resolvedCurrency,
Review Comment:
can we perhaps name it to resolvedCurrencyFormat , since the variable was previously named currentFormat
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat dynamic currency [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2588596626
##########
tests/unit_tests/connectors/sqla/models_test.py:
##########
@@ -842,3 +842,42 @@ def test_quoted_name_prevents_double_quoting(mocker: MockerFixture) -> None:
# Should have each part quoted separately:
# GOOD: "MY_DB"."MY_SCHEMA"."MY_TABLE"
assert '"MY_DB"."MY_SCHEMA"."MY_TABLE"' in compiled
+
+
+def test_table_column_is_currency_code_property() -> None:
+ """
+ Test is_currency_code property on TableColumn.
+ """
+ column = TableColumn(
+ column_name="currency",
+ type="VARCHAR(3)",
+ is_currency_code=True,
+ )
+ assert column.is_currency_code is True
+
+
+def test_sqla_table_currency_code_column_property() -> None:
+ """
+ Test currency_code_column property on SqlaTable.
+ """
+ database = Database(database_name="my_db")
+ table = SqlaTable(
+ table_name="sales",
+ database=database,
+ currency_code_column="currency",
+ )
+ assert table.currency_code_column == "currency"
+
+
+def test_table_column_data_includes_currency_code_flag() -> None:
+ """
+ Test that .data property includes is_currency_code flag.
+ """
+ column = TableColumn(
+ column_name="currency",
+ type="VARCHAR(3)",
+ is_currency_code=True,
+ )
+ data = column.data
+ assert "is_currency_code" in data
+ assert data["is_currency_code"] is True
Review Comment:
the test was deleted
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3617328385
Taking a look, but just wanted to mention that this is a really big PR... next time it would be better to use [stacked diffs]( https://dineshpandiyan.com/blog/stacked-diffs-with-rebase-onto/) to make the review easier and faster. 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat dynamic currency [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3609650798
CodeAnt AI finished reviewing your PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3644083776
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20149366065) for [5b992bb](https://github.com/apache/superset/commit/5b992bb)
• **Environment:** http://35.90.24.178:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3644046088
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20149366065) for [5b992bb](https://github.com/apache/superset/commit/5b992bb)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2665774191
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -41,6 +45,38 @@ export const getCurrencySymbol = (currency: Partial<Currency>) =>
.formatToParts(1)
.find(x => x.type === 'currency')?.value;
+/** Normalize currency to ISO 4217 format (e.g., "USD"). Returns null if invalid. */
+export function normalizeCurrency(
+ value: string | null | undefined,
+): string | null {
+ if (!value) return null;
+
+ const str = value.toString().trim();
+ if (!str) return null;
+
+ const upper = str.toUpperCase();
+
+ if (/^[A-Z]{3}$/.test(upper)) return upper;
Review Comment:
Can we extract this regex to a variable
const ISO_4217_REGEX = /^[A-Z]{3}$/;
ISO_4217_REGEX.test(normalized)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2677855641
##########
superset-frontend/plugins/legacy-plugin-chart-world-map/src/transformProps.js:
##########
@@ -47,20 +47,29 @@ export default function transformProps(chartProps) {
currencyFormat,
} = formData;
const { r, g, b } = colorPicker;
- const { currencyFormats = {}, columnFormats = {} } = datasource;
+ const {
+ currencyFormats = {},
+ columnFormats = {},
+ currencyCodeColumn,
+ } = datasource;
+ const { data, detected_currency: detectedCurrency } = queriesData[0];
const formatter = getValueFormatter(
metric,
currencyFormats,
columnFormats,
yAxisFormat,
currencyFormat,
+ undefined,
Review Comment:
added a comment to clarify the intentional usage: `// key - not needed for single-metric charts`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2683060975
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx:
##########
@@ -403,15 +430,13 @@ function ColumnCollectionTable({
type: t('Data type'),
groupby: t('Is dimension'),
is_dttm: t('Is temporal'),
- main_dttm_col: t('Default datetime'),
Review Comment:
The main_dttm_col functionality is not lost — it was moved from inline radio buttons in the column table to a dedicated 'Default Column Settings' section at the top of the Columns tab. This groups dataset-level defaults together (datetime column + new currency code column), simplifies the column table, and provides a cleaner UX with a dropdown instead of radio buttons spread across many rows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3739570424
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! 🎉
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) ·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) ·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3746867623
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20973890833) for [fbd2663](https://github.com/apache/superset/commit/fbd2663)
• **Environment:** http://44.251.7.184:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3628866677
CodeAnt AI is running Incremental review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2695159527
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -49,7 +81,9 @@ class CurrencyFormatter extends ExtensibleFunction {
currency: Currency;
constructor(config: CurrencyFormatterConfig) {
- super((value: number) => this.format(value));
+ super((value: number, rowData?: RowData, currencyColumn?: string) =>
+ this.format(value, rowData, currencyColumn),
+ );
Review Comment:
Alternatively:
```suggestion
super(this.format.bind(this));
```
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -41,6 +45,38 @@ export const getCurrencySymbol = (currency: Partial<Currency>) =>
.formatToParts(1)
.find(x => x.type === 'currency')?.value;
+/** Normalize currency to ISO 4217 format (e.g., "USD"). Returns null if invalid. */
+export function normalizeCurrency(
+ value: string | null | undefined,
+): string | null {
+ if (!value) return null;
+
+ const str = value.toString().trim();
+ if (!str) return null;
+
+ const upper = str.toUpperCase();
+
+ if (/^[A-Z]{3}$/.test(upper)) return upper;
+
+ return null;
+}
+
+/** Check if array contains multiple distinct currencies (after normalization). */
+export function hasMixedCurrencies(
+ currencies: (string | null | undefined)[],
+): boolean {
+ // Filter out null/undefined and normalize
+ const normalized = currencies
+ .map(c => normalizeCurrency(c))
+ .filter((c): c is string => c !== null);
+
+ if (normalized.length === 0) return false;
+
+ // Check if all normalized currencies are the same
+ const first = normalized[0];
+ return !normalized.every(c => c === first);
Review Comment:
Small nit, this reads clearer IMHO:
```suggestion
return normalized.some(c => c !== first);
```
##########
superset/examples/international_sales.py:
##########
@@ -0,0 +1,238 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""International sales dataset demonstrating multi-currency transactions."""
+
+import logging
+
+import pandas as pd
+from sqlalchemy import Date, inspect, Integer, Numeric, String
+
+from superset import db
+from superset.connectors.sqla.models import SqlaTable
+from superset.models.core import Database
+from superset.sql.parse import Table
+
+from ..utils.database import get_example_database # noqa: TID252
+from .helpers import get_table_connector_registry
+
+logger = logging.getLogger(__name__)
+
+
+def get_international_sales_data() -> pd.DataFrame:
+ """Generate the international sales dataset with multiple currencies."""
+ # fmt: off
+ data = [
+ # North America - USA (USD)
+ (1, "2024-01-15", "North America", "USA", "Electronics", "Laptop Pro",
+ 50, 1299.99, 64999.50, 45000.00, 19999.50, "USD", "$"),
+ (2, "2024-01-15", "North America", "USA", "Electronics", "Smartphone X",
+ 200, 899.99, 179998.00, 120000.00, 59998.00, "USD", "$"),
+ (3, "2024-01-15", "North America", "USA", "Software", "Office Suite",
+ 500, 149.99, 74995.00, 15000.00, 59995.00, "USD", "$"),
+ (4, "2024-02-15", "North America", "USA", "Electronics", "Laptop Pro",
+ 75, 1299.99, 97499.25, 67500.00, 29999.25, "USD", "$"),
+ (5, "2024-02-15", "North America", "USA", "Electronics", "Smartphone X",
+ 250, 899.99, 224997.50, 150000.00, 74997.50, "USD", "$"),
+ (6, "2024-02-15", "North America", "USA", "Software", "Office Suite",
+ 600, 149.99, 89994.00, 18000.00, 71994.00, "USD", "$"),
+ (7, "2024-03-15", "North America", "USA", "Electronics", "Laptop Pro",
+ 100, 1299.99, 129999.00, 90000.00, 39999.00, "USD", "$"),
+ (8, "2024-03-15", "North America", "USA", "Electronics", "Smartphone X",
+ 300, 899.99, 269997.00, 180000.00, 89997.00, "USD", "$"),
+ # Case normalization test - lowercase 'usd'
+ (9, "2024-03-15", "North America", "USA", "Software", "Office Suite",
+ 700, 149.99, 104993.00, 21000.00, 83993.00, "usd", "$"),
+ # North America - Canada (CAD)
+ (10, "2024-01-15", "North America", "Canada", "Electronics", "Laptop Pro",
+ 30, 1599.99, 47999.70, 35000.00, 12999.70, "CAD", "CA$"),
+ (11, "2024-01-15", "North America", "Canada", "Electronics", "Smartphone X",
+ 100, 1099.99, 109999.00, 75000.00, 34999.00, "CAD", "CA$"),
+ (12, "2024-02-15", "North America", "Canada", "Electronics", "Laptop Pro",
+ 40, 1599.99, 63999.60, 46000.00, 17999.60, "CAD", "CA$"),
+ (13, "2024-02-15", "North America", "Canada", "Software", "Office Suite",
+ 200, 199.99, 39998.00, 8000.00, 31998.00, "CAD", "CA$"),
+ # Case normalization test - mixed case 'Cad'
+ (14, "2024-03-15", "North America", "Canada", "Electronics", "Laptop Pro",
+ 50, 1599.99, 79999.50, 57500.00, 22499.50, "Cad", "CA$"),
+ # Europe - Germany/France (EUR)
+ (15, "2024-01-15", "Europe", "Germany", "Electronics", "Laptop Pro",
+ 40, 1199.99, 47999.60, 32000.00, 15999.60, "EUR", "€"),
+ (16, "2024-01-15", "Europe", "Germany", "Electronics", "Smartphone X",
+ 150, 849.99, 127498.50, 85000.00, 42498.50, "EUR", "€"),
+ (17, "2024-01-15", "Europe", "France", "Software", "Office Suite",
+ 300, 139.99, 41997.00, 9000.00, 32997.00, "EUR", "€"),
+ (18, "2024-02-15", "Europe", "Germany", "Electronics", "Laptop Pro",
+ 55, 1199.99, 65999.45, 44000.00, 21999.45, "EUR", "€"),
+ (19, "2024-02-15", "Europe", "France", "Electronics", "Smartphone X",
+ 180, 849.99, 152998.20, 102000.00, 50998.20, "EUR", "€"),
+ # Case normalization test - lowercase 'eur'
+ (20, "2024-02-15", "Europe", "Germany", "Software", "Office Suite",
+ 350, 139.99, 48996.50, 10500.00, 38496.50, "eur", "€"),
+ # Europe - UK (GBP)
+ (21, "2024-01-15", "Europe", "UK", "Electronics", "Laptop Pro",
+ 35, 999.99, 34999.65, 24500.00, 10499.65, "GBP", "£"),
+ (22, "2024-01-15", "Europe", "UK", "Electronics", "Smartphone X",
+ 120, 749.99, 89998.80, 66000.00, 23998.80, "GBP", "£"),
+ (23, "2024-02-15", "Europe", "UK", "Electronics", "Laptop Pro",
+ 45, 999.99, 44999.55, 31500.00, 13499.55, "GBP", "£"),
+ (24, "2024-02-15", "Europe", "UK", "Software", "Office Suite",
+ 250, 119.99, 29997.50, 7500.00, 22497.50, "GBP", "£"),
+ # Case normalization test - mixed case 'Gbp'
+ (25, "2024-03-15", "Europe", "UK", "Electronics", "Laptop Pro",
+ 60, 999.99, 59999.40, 42000.00, 17999.40, "Gbp", "£"),
+ # Asia - Japan (JPY)
+ (26, "2024-01-15", "Asia", "Japan", "Electronics", "Laptop Pro",
+ 25, 149999.00, 3749975.00, 2625000.00, 1124975.00, "JPY", "¥"),
+ (27, "2024-01-15", "Asia", "Japan", "Electronics", "Smartphone X",
+ 80, 99999.00, 7999920.00, 5600000.00, 2399920.00, "JPY", "¥"),
+ (28, "2024-02-15", "Asia", "Japan", "Electronics", "Laptop Pro",
+ 30, 149999.00, 4499970.00, 3150000.00, 1349970.00, "JPY", "¥"),
+ (29, "2024-03-15", "Asia", "Japan", "Software", "Office Suite",
+ 150, 14999.00, 2249850.00, 450000.00, 1799850.00, "JPY", "¥"),
+ # Asia Pacific - Australia (AUD)
+ (30, "2024-01-15", "Asia Pacific", "Australia", "Electronics", "Laptop Pro",
+ 20, 1899.99, 37999.80, 26000.00, 11999.80, "AUD", "A$"),
+ (31, "2024-02-15", "Asia Pacific", "Australia", "Electronics", "Smartphone X",
+ 60, 1299.99, 77999.40, 48000.00, 29999.40, "AUD", "A$"),
+ (32, "2024-03-15", "Asia Pacific", "Australia", "Software", "Office Suite",
+ 100, 219.99, 21999.00, 6000.00, 15999.00, "AUD", "A$"),
+ # NULL currency tests - Other region
+ (33, "2024-01-15", "Other", "Unknown", "Electronics", "Generic Device",
+ 10, 500.00, 5000.00, 3500.00, 1500.00, None, None),
+ (34, "2024-02-15", "Other", "Unknown", "Software", "Basic App",
+ 50, 50.00, 2500.00, 1000.00, 1500.00, None, None),
+ # Empty string currency test
+ (35, "2024-03-15", "Other", "Unknown", "Electronics", "Unknown Product",
+ 5, 100.00, 500.00, 350.00, 150.00, "", ""),
+ # Additional rows for aggregation tests
+ (36, "2024-01-15", "North America", "USA", "Electronics", "Tablet Pro",
+ 80, 599.99, 47999.20, 32000.00, 15999.20, "USD", "$"),
+ (37, "2024-02-15", "Europe", "Germany", "Electronics", "Tablet Pro",
+ 65, 549.99, 35749.35, 22750.00, 12999.35, "EUR", "€"),
+ (38, "2024-03-15", "Asia", "Japan", "Electronics", "Tablet Pro",
+ 45, 64999.00, 2924955.00, 1575000.00, 1349955.00, "JPY", "¥"),
+ # Euro word/symbol normalization tests
+ (39, "2024-01-15", "Europe", "Spain", "Software", "Cloud Service",
+ 100, 99.99, 9999.00, 5000.00, 4999.00, "euro", "€"),
+ (40, "2024-02-15", "Europe", "Italy", "Software", "Cloud Service",
+ 120, 99.99, 11998.80, 6000.00, 5998.80, "EURO", "€"),
+ (41, "2024-03-15", "Europe", "Portugal", "Software", "Cloud Service",
+ 80, 99.99, 7999.20, 4000.00, 3999.20, "€", "€"),
+ # Invalid currency code fallback test
+ (42, "2024-01-15", "Other", "Unknown", "Electronics", "Mystery Device",
+ 25, 200.00, 5000.00, 3000.00, 2000.00, "XYZ", "?"),
+ ]
+ # fmt: on
+
+ columns = [
+ "id",
+ "transaction_date",
+ "region",
+ "country",
+ "product_category",
+ "product_name",
+ "quantity",
+ "unit_price",
+ "revenue",
+ "cost",
+ "profit",
+ "currency_code",
+ "currency_symbol",
+ ]
+
+ return pd.DataFrame(data, columns=columns)
+
+
+def load_data(tbl_name: str, database: Database) -> None:
+ """Load the international sales data into the database."""
+ pdf = get_international_sales_data()
+ pdf["transaction_date"] = pd.to_datetime(pdf["transaction_date"])
+
+ with database.get_sqla_engine() as engine:
+ schema = inspect(engine).default_schema_name
+
+ pdf.to_sql(
+ tbl_name,
+ engine,
+ schema=schema,
+ if_exists="replace",
+ chunksize=50,
+ dtype={
+ "id": Integer,
+ "transaction_date": Date,
+ "region": String(50),
+ "country": String(50),
+ "product_category": String(50),
+ "product_name": String(100),
+ "quantity": Integer,
+ "unit_price": Numeric(12, 2),
+ "revenue": Numeric(14, 2),
+ "cost": Numeric(14, 2),
+ "profit": Numeric(14, 2),
+ "currency_code": String(10),
+ "currency_symbol": String(10),
+ },
+ method="multi",
+ index=False,
+ )
+ logger.debug("Done loading international sales data!")
+
+
+def load_international_sales(only_metadata: bool = False, force: bool = False) -> None:
Review Comment:
Ideally we'd add this dataset to `superset/examples/configs` as a YAML file, but there's an undocumented trick to get that working so let's keep this for now.
##########
superset/viz.py:
##########
@@ -475,6 +480,56 @@ def cache_key(self, query_obj: QueryObjectDict, **extra: Any) -> str:
json_data = self.json_dumps(cache_dict, sort_keys=True)
return hash_from_str(json_data)
+ @deprecated(deprecated_in="3.0")
Review Comment:
We're in Superset 6.0 already, why are you adding a method that's deprecated in 3.0?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3614649678
CodeAnt AI is running Incremental review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat dynamic currency [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3611493535
CodeAnt AI is running Incremental review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "kasiazjc (via GitHub)" <gi...@apache.org>.
kasiazjc commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3616770527
> Hi @kasiazjc, sorry that screenshot is outdated, I have already updated the label to "Auto-detect" all good, thanks! :)
perfect, thank you! 🫶
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "kasiazjc (via GitHub)" <gi...@apache.org>.
kasiazjc commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3616752708
@richardfogaca let's change the label in the dropdown from "Auto-detect from dataset" to "Auto-detect" so that it fits nicely :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3628391572
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20037419699) for [f0165e5](https://github.com/apache/superset/commit/f0165e5)
• **Environment:** http://44.255.20.214:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3628359526
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20037419699) for [f0165e5](https://github.com/apache/superset/commit/f0165e5)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat dynamic currency [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3609650217
## Nitpicks 🔍
<table>
<tr><td>🔒 <strong>No security issues identified</strong></td></tr>
<tr><td>⚡ <strong>Recommended areas for review</strong><br><br>
- [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-fda2a3496e8791d837d7a931dbb9d00e4fdcaf6e88447ed9a2c10d383d5c7fb5R141-R143'><strong>Intl.RangeError risk</strong></a><br>Calls to `getCurrencySymbol(this.currency)` use the `currency.symbol` value directly as the Intl `currency` code. If that value is invalid (not a valid ISO 4217 code) `Intl.NumberFormat` may throw a RangeError. The non-AUTO path has no try/catch or validation and can crash at runtime.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-fda2a3496e8791d837d7a931dbb9d00e4fdcaf6e88447ed9a2c10d383d5c7fb5R33-R37'><strong>Null/undefined value handling</strong></a><br>The formatter interface allows `value` to be null/undefined but the implementation (`format`) expects a `number` and immediately passes it to a number formatter. If callers pass null/undefined the formatter may throw or produce invalid output. The implementation should guard/coerce `value` before formatting.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-d9ffcff287c482c6212389c0c2674187f9dd37ab1b5392ae197b41e3d2a373c2R218-R224'><strong>Detected currency validation</strong></a><br>`detectedCurrency` is used if truthy, but backend could return unexpected values (arrays, 'MIXED', null, lowercase, etc.). The frontend should validate the detected value (e.g., ensure it's a single ISO currency code) and fall back to neutral formatting for mixed/invalid results to avoid incorrect currency formatting.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-d9ffcff287c482c6212389c0c2674187f9dd37ab1b5392ae197b41e3d2a373c2R286-R306'><strong>Currency object shape</strong></a><br>The code sets `resolvedCurrency = { ...currency, symbol: detectedCurrency }`. Ensure that `CurrencyFormatter` accepts `symbol` being an ISO currency code string here; if the `Currency` type expects other keys (e.g., `code`, `locale`, or `symbol` with different semantics), this can produce incorrect formatting or runtime errors. Confirm the type/shape and mapping between detected value and CurrencyFormatter expectations.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-2672bc046f1a56f67dd661b1ace7c639da51ad78c4e442b96445c60924621c4eR281-R313'><strong>CurrencyFormatter type mismatch</strong></a><br>The new code passes `resolvedCurrency` into `CurrencyFormatter` (as `currency`) and into `buildCustomFormatters`. Verify `resolvedCurrency` shape matches what these APIs expect (string currency code vs. currency object with symbol/code). If shapes differ, formatting could be incorrect or runtime errors may appear. Also ensure `resolveAutoCurrency` returns consistent structure for primary and secondary series.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-2672bc046f1a56f67dd661b1ace7c639da51ad78c4e442b96445c60924621c4eR314-R331'><strong>buildCustomFormatters signature change</strong></a><br>`buildCustomFormatters` is now called with extra params (`resolvedCurrency`, `data1`, `currencyCodeColumn`). Ensure the function signature and behaviour have been updated accordingly and that these are used only when available to avoid undefined propagation.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-d9ffcff287c482c6212389c0c2674187f9dd37ab1b5392ae197b41e3d2a373c2R200-R320'><strong>Memoization staleness</strong></a><br>processColumns is memoized with a custom equality function (`isEqualColumns`). New inputs (dataset `currencyCodeColumn` and the backend `detected_currency`) affect the returned column formatters but may not be considered by the current memoization comparator, causing stale formatter/column outputs when the detected currency changes. Verify the memoization comparator covers these new fields or remove/adjust it so changes to detection trigger recomputation.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-f81d43cccc89e454b69515e1485808fb3f0406ef4ec085bebd438ced87a6fb2fR260-R328'><strong>Formatting behavior / assumption</strong></a><br>The AUTO currency handling depends on `currencyFormat` having `symbol === 'AUTO'` and the aggregator exposing currencies per-cell. Verify the UI/backend contract: ensure that when `currencyFormat.symbol` is `'AUTO'` the dataset actually provides `currencyCodeColumn`, that aggregators are instrumented to collect currencies, and that `detectedCurrency` fallback logic uses the intended server-provided value. Confirm behavior for mixed/null currency values.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-00b3a3d5ef7bab1dac800b90eeae358d199d6b34c6390fab7ef6aaa6303403f8R75-R76'><strong>Possible Bug</strong></a><br>The query object uses the key `"filter"` but Superset's Query object typically expects `"filters"` (plural). If the underlying `datasource.query` expects `"filters"`, the query will ignore provided filters and may return wrong results (or a full table scan). Confirm the expected query object schema and make the key consistent with consumers.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-359940b6f6470257ed49fa39b82361ccb50a826407072043843a36a0ed6ce4e9R490-R494'><strong>Assumption about `currency_format` shape</strong></a><br>`_detect_currency` expects `self.form_data["currency_format"]` to be a dict with a `symbol` key. If `currency_format` has a different shape (e.g. a string or a legacy format), the AUTO check may silently skip detection. Validate or normalize `currency_format` shape before checking `symbol` to avoid missed detection or unexpected behavior.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-359940b6f6470257ed49fa39b82361ccb50a826407072043843a36a0ed6ce4e9R425-R448'><strong>Cache inconsistency</strong></a><br>The detected currency is added to the payload but is not stored in the data cache (`get_df_payload` stores only `{"df": df, "query": self.query}`). When serving from cache, `detected_currency` is not part of cache_value and may be recomputed (causing extra queries) or differ from what the cached df represents. The detection result should be cached alongside the dataframe (or factored into the cache key) to avoid re-detection and ensure consistency between cached df and reported detected currency.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-359940b6f6470257ed49fa39b82361ccb50a826407072043843a36a0ed6ce4e9R479-R504'><strong>Performance / Duplicate Query</strong></a><br>The newly added `_detect_currency` helper likely issues an additional query to the datasource (via the shared `detect_currency` utility) even though `get_payload()` already obtains the dataframe for the same query. This can double the query cost and add latency for charts using AUTO currency. Prefer detecting currency from the already-fetched `df` when possible and only fall back to `detect_currency()` when the dataframe does not contain the configured currency column or the detection requires a separate query.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-12adcc0dab4417a345f763565aee3cd0f657eece6142615492172abe69cfd3bdR748-R852'><strong>Currency auto-detect context missing</strong></a><br>The change appears to support passing the aggregator to `format`, likely to enable row-level currency auto-detection. However, there is no row/row-data context being passed (only `agg`), so formatters that need the original row object or a currency column value may still lack the necessary context. Investigate how row-level information should be provided to the formatter (e.g., supplying raw row data or keys) so AUTO currency detection can be accurate.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-64ed1780b1eae73eaa259863ef625b5527333ea9f11a114220f3fe7f6d229f20R57-R67'><strong>Formatter argument ordering</strong></a><br>`getValueFormatter` is now called with additional args (`undefined, data, currencyCodeColumn, detectedCurrency`). Ensure this matches the expected parameter order for all call sites; a mismatch can produce incorrect formatting or silent failures.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-12adcc0dab4417a345f763565aee3cd0f657eece6142615492172abe69cfd3bdR748-R852'><strong>Possible Format Signature Mismatch</strong></a><br>The new calls pass the aggregator object as a second argument to `agg.format(aggValue, agg)`. Verify all aggregator implementations (including fallback aggregator returned by `getAggregator`) accept a second argument. If any aggregator's `format` implementation ignores extra args or expects a different shape, this may cause inconsistent formatting or unexpected behavior. Also ensure any custom aggregators written elsewhere in the codebase are updated to accept the new second argument if they rely on it.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-f7b502ad4943c53bd516a12f9b54ecbcaf35a966833cb6fcdedde3f1cc954cf3R215-R475'><strong>Memory / duplicate growth</strong></a><br>Each aggregator now pushes seen currency codes into plain arrays (e.g. `this.currencies.push(...)`) without deduplication or bounds. For large datasets or high-cardinality result sets these arrays will grow unbounded and contain many duplicates, increasing memory usage and slowing get/dedup operations later.<br>
</td></tr>
</table>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat dynamic currency [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3609652875
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/36416?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `31.03448%` with `80 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 67.34%. Comparing base ([`3eec441`](https://app.codecov.io/gh/apache/superset/commit/3eec441abe2d5e385fe82925cea39c7a267ffc1c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`54dac48`](https://app.codecov.io/gh/apache/superset/commit/54dac48dfab5d6bd2b3654c1d762c2cbb01236dc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/36416?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/examples/international\_sales.py](https://app.codecov.io/gh/apache/superset/pull/36416?src=pr&el=tree&filepath=superset%2Fexamples%2Finternational_sales.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZXhhbXBsZXMvaW50ZXJuYXRpb25hbF9zYWxlcy5weQ==) | 0.00% | [43 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/36416?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/utils/currency.py](https://app.codecov.io/gh/apache/superset/pull/36416?src=pr&el=tree&filepath=superset%2Futils%2Fcurrency.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvY3VycmVuY3kucHk=) | 36.00% | [15 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/superset/pull/36416?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/common/query\_context\_factory.py](https://app.codecov.io/gh/apache/superset/pull/36416?src=pr&el=tree&filepath=superset%2Fcommon%2Fquery_context_factory.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X2NvbnRleHRfZmFjdG9yeS5weQ==) | 55.55% | [7 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/superset/pull/36416?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/views/core.py](https://app.codecov.io/gh/apache/superset/pull/36416?src=pr&el=tree&filepath=superset%2Fviews%2Fcore.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/36416?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/viz.py](https://app.codecov.io/gh/apache/superset/pull/36416?src=pr&el=tree&filepath=superset%2Fviz.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdml6LnB5) | 50.00% | [3 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/superset/pull/36416?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/cli/examples.py](https://app.codecov.io/gh/apache/superset/pull/36416?src=pr&el=tree&filepath=superset%2Fcli%2Fexamples.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY2xpL2V4YW1wbGVzLnB5) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/36416?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/examples/data\_loading.py](https://app.codecov.io/gh/apache/superset/pull/36416?src=pr&el=tree&filepath=superset%2Fexamples%2Fdata_loading.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZXhhbXBsZXMvZGF0YV9sb2FkaW5nLnB5) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/36416?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #36416 +/- ##
===========================================
+ Coverage 0 67.34% +67.34%
===========================================
Files 0 636 +636
Lines 0 46842 +46842
Branches 0 5085 +5085
===========================================
+ Hits 0 31546 +31546
- Misses 0 14012 +14012
- Partials 0 1284 +1284
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/36416/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/36416/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `43.64% <17.24%> (?)` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/36416/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `66.98% <31.03%> (?)` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/36416/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `47.30% <26.72%> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/36416/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `67.31% <31.03%> (?)` | |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/36416/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `66.70% <31.03%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/36416?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3715918336
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3716247411
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3746771159
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20973733699) for [776ab40](https://github.com/apache/superset/commit/776ab40)
• **Environment:** http://16.144.54.174:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3746717107
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20973733699) for [776ab40](https://github.com/apache/superset/commit/776ab40)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3730857045
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20867991390) for [8abf362](https://github.com/apache/superset/commit/8abf362)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3731524112
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20868179636) for [c189974](https://github.com/apache/superset/commit/c189974)
• **Environment:** http://34.216.197.209:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2681489144
##########
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:
##########
@@ -276,10 +287,25 @@ const processColumns = memoizeOne(function processColumns(
// percent metrics have a default format
formatter = getNumberFormatter(numberFormat || PERCENT_3_POINT);
} else if (isMetric || (isNumber && (numberFormat || currency))) {
- formatter = currency?.symbol
+ // Resolve AUTO currency when currency column isn't in query results
+ let resolvedCurrency = currency;
Review Comment:
Is this backwards compatible? Meaning if someone who doesn't have a currency_column in their data keeps their previously set currency?
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx:
##########
@@ -403,15 +430,13 @@ function ColumnCollectionTable({
type: t('Data type'),
groupby: t('Is dimension'),
is_dttm: t('Is temporal'),
- main_dttm_col: t('Default datetime'),
Review Comment:
Is losing this significant?
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx:
##########
@@ -157,6 +159,47 @@ const StyledTableTabWrapper = styled.div`
}
`;
+const DefaultColumnSettingsContainer = styled.div`
+ ${({ theme }) => css`
+ margin-bottom: ${theme.sizeUnit * 4}px;
+ `}
+`;
+
+const DefaultColumnSettingsTitle = styled.h4`
+ ${({ theme }) => css`
+ margin: 0 0 ${theme.sizeUnit * 2}px 0;
+ font-size: ${theme.fontSizeLG}px;
+ font-weight: ${theme.fontWeightMedium};
+ color: ${theme.colorText};
+ `}
+`;
+
+const DefaultColumnSettingsFields = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-direction: column;
+ gap: ${theme.sizeUnit * 3}px;
+ `}
+`;
+
+const FieldWrapper = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-direction: column;
+ gap: ${theme.sizeUnit}px;
+ `}
+`;
+
+const FieldLabelWithTooltip = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ align-items: center;
+ gap: ${theme.sizeUnit}px;
+ font-size: ${theme.fontSizeSM}px;
+ color: ${theme.colorTextLabel};
+ `}
+`;
Review Comment:
Seems like a good place to use our Flex component?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3756662832
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/21044774894) for [2a9bafe](https://github.com/apache/superset/commit/2a9bafe)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2695723588
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -49,7 +81,9 @@ class CurrencyFormatter extends ExtensibleFunction {
currency: Currency;
constructor(config: CurrencyFormatterConfig) {
- super((value: number) => this.format(value));
+ super((value: number, rowData?: RowData, currencyColumn?: string) =>
+ this.format(value, rowData, currencyColumn),
+ );
Review Comment:
Thanks for the suggestion! Unfortunately, these aren't quite equivalent due to timing. When super() is called, this.format doesn't exist yet since super() must complete before this is accessible. Using this.format.bind(this) would evaluate this.format immediately (resulting in undefined), whereas the arrow function defers the lookup until the function is actually invoked, by which point the instance is fully constructed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3762717289
Looks great! Thanks @richardfogaca!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3618476107
Hi @betodealmeida, I didn't know about that. It does seem quite helpful for these cases, I'll make sure I use it next time. Thanks for the tip
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3617805999
CodeAnt AI is running Incremental review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2665765599
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -723,3 +723,133 @@ describe('legend sorting', () => {
]);
});
});
+
+describe('EchartsTimeseries AUTO Mode Currency', () => {
+ it('should detect single currency and resolve AUTO mode', () => {
+ const chartProps = new ChartProps<SqlaFormData>({
+ ...chartPropsConfig,
+ formData: {
+ ...formData,
+ currencyFormat: { symbol: 'AUTO', symbolPosition: 'prefix' },
+ },
+ datasource: {
+ currencyCodeColumn: 'currency_code',
+ columnFormats: {},
+ currencyFormats: {},
+ verboseMap: {},
+ },
+ queriesData: [
+ {
+ data: [
+ {
+ 'San Francisco': 1000,
+ __timestamp: 599616000000,
+ currency_code: 'USD',
+ },
+ {
+ 'San Francisco': 2000,
+ __timestamp: 599916000000,
+ currency_code: 'USD',
+ },
+ ],
+ },
+ ],
+ });
+
+ const transformed = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ // Y-axis formatter should contain $ for USD
+ const yAxisFormatter = (transformed.echartOptions.yAxis as any)?.axisLabel
+ ?.formatter;
+ if (yAxisFormatter) {
+ expect(yAxisFormatter(1000)).toContain('$');
+ }
+ });
+
+ it('should use neutral formatting for mixed currencies in AUTO mode', () => {
+ const chartProps = new ChartProps<SqlaFormData>({
+ ...chartPropsConfig,
+ formData: {
+ ...formData,
+ currencyFormat: { symbol: 'AUTO', symbolPosition: 'prefix' },
+ },
+ datasource: {
+ currencyCodeColumn: 'currency_code',
+ columnFormats: {},
+ currencyFormats: {},
+ verboseMap: {},
+ },
+ queriesData: [
+ {
+ data: [
+ {
+ 'San Francisco': 1000,
+ __timestamp: 599616000000,
+ currency_code: 'USD',
+ },
+ {
+ 'San Francisco': 2000,
+ __timestamp: 599916000000,
+ currency_code: 'EUR',
+ },
+ ],
+ },
+ ],
+ });
+
+ const transformed = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ // With mixed currencies, Y-axis should use neutral formatting
+ const yAxisFormatter = (transformed.echartOptions.yAxis as any)?.axisLabel
+ ?.formatter;
+ if (yAxisFormatter) {
+ const formatted = yAxisFormatter(1000);
+ expect(formatted).not.toContain('$');
+ expect(formatted).not.toContain('€');
+ }
+ });
+
+ it('should preserve static currency format regardless of data', () => {
+ const chartProps = new ChartProps<SqlaFormData>({
+ ...chartPropsConfig,
+ formData: {
+ ...formData,
+ currencyFormat: { symbol: 'GBP', symbolPosition: 'prefix' },
+ },
+ datasource: {
+ currencyCodeColumn: 'currency_code',
+ columnFormats: {},
+ currencyFormats: {},
+ verboseMap: {},
+ },
+ queriesData: [
+ {
+ data: [
+ {
+ 'San Francisco': 1000,
+ __timestamp: 599616000000,
+ currency_code: 'USD',
+ },
+ {
+ 'San Francisco': 2000,
+ __timestamp: 599916000000,
+ currency_code: 'EUR',
+ },
+ ],
+ },
+ ],
+ });
+
+ const transformed = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ // Static mode should always show £
+ const yAxisFormatter = (transformed.echartOptions.yAxis as any)?.axisLabel
Review Comment:
Can we have a better type instead of any , same as above
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2665748384
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -30,7 +30,11 @@ interface CurrencyFormatterConfig {
}
interface CurrencyFormatter {
- (value: number | null | undefined): string;
+ (
+ value: number | null | undefined,
+ rowData?: Record<string, any>,
Review Comment:
type RowData = Record<string, string | number | boolean | null | undefined>;
Can we do something like this instead of any
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat dynamic currency [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3609624873
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3730890489
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20868179636) for [c189974](https://github.com/apache/superset/commit/c189974)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3746837458
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20973890833) for [fbd2663](https://github.com/apache/superset/commit/fbd2663)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2683269485
##########
superset/datasets/schemas.py:
##########
@@ -166,6 +166,7 @@ class DatasetPutSchema(Schema):
schema = fields.String(allow_none=True, validate=Length(0, 255))
description = fields.String(allow_none=True)
main_dttm_col = fields.String(allow_none=True)
+ currency_code_column = fields.String(allow_none=True, validate=Length(0, 250))
normalize_columns = fields.Boolean(allow_none=True, dump_default=False)
Review Comment:
The ImportV1DataSetSchema must also include this column otherwise importing to a new environment will suffer from a failure , Please check for Import Export Scenario ,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2665762001
##########
superset-frontend/packages/superset-ui-core/src/currency-format/utils.ts:
##########
@@ -76,14 +147,40 @@ export const getValueFormatter = (
d3Format: string | undefined,
currencyFormat: Currency | undefined,
key?: string,
+ data?: Record<string, any>[],
Review Comment:
Can we have a better type instead of any
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2677851037
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/types.ts:
##########
@@ -31,7 +31,7 @@ import { ColorFormatters } from '@superset-ui/chart-controls';
import { BaseChartProps, Refs } from '../types';
export interface BigNumberDatum {
- [key: string]: number | null;
+ [key: string]: number | string | null;
Review Comment:
The string type is because the query response data can include currency code columns (e.g. currency_code: 'USD'), which are strings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3731052320
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3629788839
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20048702130) for [5b992bb](https://github.com/apache/superset/commit/5b992bb)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2665802038
##########
superset-frontend/packages/superset-ui-core/src/currency-format/utils.ts:
##########
@@ -25,21 +25,92 @@ import {
QueryFormMetric,
ValueFormatter,
} from '@superset-ui/core';
+import { normalizeCurrency, hasMixedCurrencies } from './CurrencyFormatter';
+
+/**
+ * Detect single currency in data. Returns ISO 4217 code if uniform, null if mixed.
+ */
+export const analyzeCurrencyInData = (
+ data: Record<string, any>[],
+ currencyColumn: string | undefined,
+): string | null => {
+ if (!currencyColumn || !data || data.length === 0) {
+ return null;
+ }
+
+ const currencies = data
+ .map(row => row[currencyColumn])
+ .filter(val => val !== null && val !== undefined);
+
+ if (currencies.length === 0) {
+ return null;
+ }
+
+ if (hasMixedCurrencies(currencies)) {
+ return null;
+ }
+
+ return normalizeCurrency(currencies[0]);
+};
+
+/** Resolve AUTO currency to detected value, null (mixed), or passthrough original. */
+export const resolveAutoCurrency = (
+ currencyFormat: Currency | undefined,
+ backendDetected: string | null | undefined,
+ data?: Record<string, any>[],
+ currencyCodeColumn?: string,
+): Currency | undefined | null => {
+ if (currencyFormat?.symbol !== 'AUTO') return currencyFormat;
+
+ const detectedCurrency =
+ backendDetected ??
+ (data && currencyCodeColumn
+ ? analyzeCurrencyInData(data, currencyCodeColumn)
+ : null);
+
+ if (detectedCurrency) {
+ return {
+ symbol: detectedCurrency,
+ symbolPosition: currencyFormat.symbolPosition,
+ };
+ }
+ return null; // Mixed currencies
+};
export const buildCustomFormatters = (
metrics: QueryFormMetric | QueryFormMetric[] | undefined,
savedCurrencyFormats: Record<string, Currency>,
savedColumnFormats: Record<string, string>,
d3Format: string | undefined,
- currencyFormat: Currency | undefined,
+ currencyFormat: Currency | undefined | null,
+ data?: Record<string, any>[],
+ currencyCodeColumn?: string,
) => {
const metricsArray = ensureIsArray(metrics);
+
+ let resolvedCurrency = currencyFormat;
+ if (currencyFormat?.symbol === 'AUTO' && data && currencyCodeColumn) {
+ const detectedCurrency = analyzeCurrencyInData(data, currencyCodeColumn);
+ if (detectedCurrency) {
+ resolvedCurrency = {
+ symbol: detectedCurrency,
+ symbolPosition: currencyFormat.symbolPosition,
+ };
+ } else {
+ resolvedCurrency = null;
+ }
+ }
+
return metricsArray.reduce((acc, metric) => {
if (isSavedMetric(metric)) {
const actualD3Format = d3Format ?? savedColumnFormats[metric];
- const actualCurrencyFormat = currencyFormat?.symbol
- ? currencyFormat
- : savedCurrencyFormats[metric];
+ // null means explicitly no currency (from AUTO mixed detection)
+ const actualCurrencyFormat =
+ resolvedCurrency === null
+ ? undefined
+ : resolvedCurrency?.symbol
+ ? resolvedCurrency
+ : savedCurrencyFormats[metric];
Review Comment:
too many ternary , impacts readability , can we extract it to a function
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3723144115
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20812558276) for [832bd59](https://github.com/apache/superset/commit/832bd59)
• **Environment:** http://16.148.18.37:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2665799047
##########
superset-frontend/packages/superset-ui-core/src/currency-format/utils.ts:
##########
@@ -25,21 +25,92 @@ import {
QueryFormMetric,
ValueFormatter,
} from '@superset-ui/core';
+import { normalizeCurrency, hasMixedCurrencies } from './CurrencyFormatter';
+
+/**
+ * Detect single currency in data. Returns ISO 4217 code if uniform, null if mixed.
+ */
+export const analyzeCurrencyInData = (
+ data: Record<string, any>[],
+ currencyColumn: string | undefined,
+): string | null => {
+ if (!currencyColumn || !data || data.length === 0) {
+ return null;
+ }
+
+ const currencies = data
+ .map(row => row[currencyColumn])
+ .filter(val => val !== null && val !== undefined);
+
+ if (currencies.length === 0) {
+ return null;
+ }
+
+ if (hasMixedCurrencies(currencies)) {
+ return null;
+ }
+
+ return normalizeCurrency(currencies[0]);
+};
+
+/** Resolve AUTO currency to detected value, null (mixed), or passthrough original. */
+export const resolveAutoCurrency = (
+ currencyFormat: Currency | undefined,
+ backendDetected: string | null | undefined,
+ data?: Record<string, any>[],
+ currencyCodeColumn?: string,
+): Currency | undefined | null => {
+ if (currencyFormat?.symbol !== 'AUTO') return currencyFormat;
+
+ const detectedCurrency =
+ backendDetected ??
+ (data && currencyCodeColumn
+ ? analyzeCurrencyInData(data, currencyCodeColumn)
+ : null);
+
+ if (detectedCurrency) {
+ return {
+ symbol: detectedCurrency,
+ symbolPosition: currencyFormat.symbolPosition,
+ };
+ }
+ return null; // Mixed currencies
+};
export const buildCustomFormatters = (
metrics: QueryFormMetric | QueryFormMetric[] | undefined,
savedCurrencyFormats: Record<string, Currency>,
savedColumnFormats: Record<string, string>,
d3Format: string | undefined,
- currencyFormat: Currency | undefined,
+ currencyFormat: Currency | undefined | null,
+ data?: Record<string, any>[],
+ currencyCodeColumn?: string,
) => {
const metricsArray = ensureIsArray(metrics);
+
+ let resolvedCurrency = currencyFormat;
+ if (currencyFormat?.symbol === 'AUTO' && data && currencyCodeColumn) {
Review Comment:
use constant for AUTO
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3715719924
### <span aria-hidden="true">👷</span> Deploy Preview for *superset-docs-preview* processing.
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | ab03cf9594158b1a009941384c337e4497b36003 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/695d4cf094c2c700081947f6 |
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3743266185
CodeAnt AI is running Incremental review
---
### Thanks for using CodeAnt! 🎉
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) ·
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) ·
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3747105686
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20977330697) for [dabc514](https://github.com/apache/superset/commit/dabc514)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3758871150
hey @rusackas, the SIP has been closed and the PR is approved, can we lift the hold label?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3756707185
CodeAnt AI Incremental review completed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3629815945
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20048702130) for [5b992bb](https://github.com/apache/superset/commit/5b992bb)
• **Environment:** http://35.162.7.251:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2665743004
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -49,7 +85,10 @@ class CurrencyFormatter extends ExtensibleFunction {
currency: Currency;
constructor(config: CurrencyFormatterConfig) {
- super((value: number) => this.format(value));
+ super(
+ (value: number, rowData?: Record<string, any>, currencyColumn?: string) =>
Review Comment:
Can we have something other than any
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2665764634
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -723,3 +723,133 @@ describe('legend sorting', () => {
]);
});
});
+
+describe('EchartsTimeseries AUTO Mode Currency', () => {
+ it('should detect single currency and resolve AUTO mode', () => {
+ const chartProps = new ChartProps<SqlaFormData>({
+ ...chartPropsConfig,
+ formData: {
+ ...formData,
+ currencyFormat: { symbol: 'AUTO', symbolPosition: 'prefix' },
+ },
+ datasource: {
+ currencyCodeColumn: 'currency_code',
+ columnFormats: {},
+ currencyFormats: {},
+ verboseMap: {},
+ },
+ queriesData: [
+ {
+ data: [
+ {
+ 'San Francisco': 1000,
+ __timestamp: 599616000000,
+ currency_code: 'USD',
+ },
+ {
+ 'San Francisco': 2000,
+ __timestamp: 599916000000,
+ currency_code: 'USD',
+ },
+ ],
+ },
+ ],
+ });
+
+ const transformed = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ // Y-axis formatter should contain $ for USD
+ const yAxisFormatter = (transformed.echartOptions.yAxis as any)?.axisLabel
Review Comment:
Can we have a better type instead of any , we can extend the interface of EchartsTimeSeriesChartProps in order to accomodate yAxis
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "amaannawab923 (via GitHub)" <gi...@apache.org>.
amaannawab923 commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2665796556
##########
superset-frontend/packages/superset-ui-core/src/currency-format/utils.ts:
##########
@@ -25,21 +25,92 @@ import {
QueryFormMetric,
ValueFormatter,
} from '@superset-ui/core';
+import { normalizeCurrency, hasMixedCurrencies } from './CurrencyFormatter';
+
+/**
+ * Detect single currency in data. Returns ISO 4217 code if uniform, null if mixed.
+ */
+export const analyzeCurrencyInData = (
+ data: Record<string, any>[],
+ currencyColumn: string | undefined,
+): string | null => {
+ if (!currencyColumn || !data || data.length === 0) {
+ return null;
+ }
+
+ const currencies = data
+ .map(row => row[currencyColumn])
+ .filter(val => val !== null && val !== undefined);
+
+ if (currencies.length === 0) {
+ return null;
+ }
+
+ if (hasMixedCurrencies(currencies)) {
+ return null;
+ }
+
+ return normalizeCurrency(currencies[0]);
+};
+
+/** Resolve AUTO currency to detected value, null (mixed), or passthrough original. */
+export const resolveAutoCurrency = (
+ currencyFormat: Currency | undefined,
+ backendDetected: string | null | undefined,
+ data?: Record<string, any>[],
+ currencyCodeColumn?: string,
+): Currency | undefined | null => {
+ if (currencyFormat?.symbol !== 'AUTO') return currencyFormat;
Review Comment:
Can we extract this to a constant 'AUTO'
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3723081035
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20812558276) for [832bd59](https://github.com/apache/superset/commit/832bd59)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat dynamic currency [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3609625892
CodeAnt AI is reviewing your PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat dynamic currency [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2588539307
##########
tests/unit_tests/datasets/schema_tests.py:
##########
@@ -46,3 +46,47 @@ def test_validate_python_date_format(payload) -> None:
def test_validate_python_date_format_raises(payload) -> None:
with pytest.raises(ValidationError):
validate_python_date_format(payload)
+
+
+def test_dataset_columns_put_schema_includes_currency_code() -> None:
+ """Test that DatasetColumnsPutSchema properly handles is_currency_code field."""
+ from superset.datasets.schemas import DatasetColumnsPutSchema
+
+ schema = DatasetColumnsPutSchema()
+
+ # Column marked as currency code
+ data = {
+ "column_name": "currency",
+ "is_currency_code": True,
+ }
+ result = schema.load(data)
+ assert result["is_currency_code"] is True
Review Comment:
is_currency_code is not needed, so this column has been removed from stale tests and types
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3615909842
CodeAnt AI is running Incremental review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3756885112
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2675507676
##########
superset-frontend/packages/superset-ui-core/test/currency-format/normalizeCurrency.test.ts:
##########
Review Comment:
Should we merge these tests in a single utils.test file?
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -63,18 +97,47 @@ class CurrencyFormatter extends ExtensibleFunction {
return this.d3Format.replace(/\$|%/g, '');
}
- format(value: number) {
+ format(value: number, rowData?: RowData, currencyColumn?: string): string {
const formattedValue = getNumberFormatter(this.getNormalizedD3Format())(
value,
);
- if (!this.hasValidCurrency()) {
+
+ const isAutoMode = this.currency?.symbol === AUTO_CURRENCY_SYMBOL;
+
+ if (!this.hasValidCurrency() && !isAutoMode) {
return formattedValue as string;
}
- if (this.currency.symbolPosition === 'prefix') {
- return `${getCurrencySymbol(this.currency)} ${formattedValue}`;
+ if (isAutoMode) {
+ if (rowData && currencyColumn && rowData[currencyColumn]) {
+ const rawCurrency = rowData[currencyColumn];
+ const normalizedCurrency = normalizeCurrency(rawCurrency);
+
+ if (normalizedCurrency) {
+ try {
+ const symbol = getCurrencySymbol({ symbol: normalizedCurrency });
+ if (symbol) {
+ return this.currency.symbolPosition === 'prefix'
+ ? `${symbol} ${formattedValue}`
+ : `${formattedValue} ${symbol}`;
+ }
+ } catch {
+ // Invalid currency code - fall through to neutral format
+ }
Review Comment:
Or even return the formattedValue without falling through?
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -63,18 +97,47 @@ class CurrencyFormatter extends ExtensibleFunction {
return this.d3Format.replace(/\$|%/g, '');
}
- format(value: number) {
+ format(value: number, rowData?: RowData, currencyColumn?: string): string {
const formattedValue = getNumberFormatter(this.getNormalizedD3Format())(
value,
);
- if (!this.hasValidCurrency()) {
+
+ const isAutoMode = this.currency?.symbol === AUTO_CURRENCY_SYMBOL;
+
+ if (!this.hasValidCurrency() && !isAutoMode) {
return formattedValue as string;
}
- if (this.currency.symbolPosition === 'prefix') {
- return `${getCurrencySymbol(this.currency)} ${formattedValue}`;
+ if (isAutoMode) {
+ if (rowData && currencyColumn && rowData[currencyColumn]) {
+ const rawCurrency = rowData[currencyColumn];
+ const normalizedCurrency = normalizeCurrency(rawCurrency);
+
+ if (normalizedCurrency) {
+ try {
+ const symbol = getCurrencySymbol({ symbol: normalizedCurrency });
+ if (symbol) {
+ return this.currency.symbolPosition === 'prefix'
+ ? `${symbol} ${formattedValue}`
+ : `${formattedValue} ${symbol}`;
+ }
+ } catch {
+ // Invalid currency code - fall through to neutral format
+ }
Review Comment:
Can we add a log for this. This just swallows the error now right?
##########
superset-frontend/plugins/legacy-plugin-chart-world-map/src/transformProps.js:
##########
@@ -47,20 +47,29 @@ export default function transformProps(chartProps) {
currencyFormat,
} = formData;
const { r, g, b } = colorPicker;
- const { currencyFormats = {}, columnFormats = {} } = datasource;
+ const {
+ currencyFormats = {},
+ columnFormats = {},
+ currencyCodeColumn,
+ } = datasource;
+ const { data, detected_currency: detectedCurrency } = queriesData[0];
const formatter = getValueFormatter(
metric,
currencyFormats,
columnFormats,
yAxisFormat,
currencyFormat,
+ undefined,
Review Comment:
Are we intending to pass undefined by default for key?
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/types.ts:
##########
@@ -31,7 +31,7 @@ import { ColorFormatters } from '@superset-ui/chart-controls';
import { BaseChartProps, Refs } from '../types';
export interface BigNumberDatum {
- [key: string]: number | null;
+ [key: string]: number | string | null;
Review Comment:
Why do we accept a string as well now?
##########
superset-frontend/packages/superset-ui-core/src/currency-format/CurrencyFormatter.ts:
##########
@@ -63,18 +97,47 @@ class CurrencyFormatter extends ExtensibleFunction {
return this.d3Format.replace(/\$|%/g, '');
}
- format(value: number) {
+ format(value: number, rowData?: RowData, currencyColumn?: string): string {
const formattedValue = getNumberFormatter(this.getNormalizedD3Format())(
value,
);
- if (!this.hasValidCurrency()) {
+
+ const isAutoMode = this.currency?.symbol === AUTO_CURRENCY_SYMBOL;
+
+ if (!this.hasValidCurrency() && !isAutoMode) {
return formattedValue as string;
}
- if (this.currency.symbolPosition === 'prefix') {
- return `${getCurrencySymbol(this.currency)} ${formattedValue}`;
+ if (isAutoMode) {
+ if (rowData && currencyColumn && rowData[currencyColumn]) {
+ const rawCurrency = rowData[currencyColumn];
+ const normalizedCurrency = normalizeCurrency(rawCurrency);
+
+ if (normalizedCurrency) {
+ try {
+ const symbol = getCurrencySymbol({ symbol: normalizedCurrency });
+ if (symbol) {
+ return this.currency.symbolPosition === 'prefix'
Review Comment:
symbolPosition is string right? So if it's not 'prefix' we are treating it as suffix?
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/transformProps.ts:
##########
@@ -92,6 +100,10 @@ export default function transformProps(
columnFormats,
metricEntry?.d3format || yAxisFormat,
currencyFormat,
+ undefined,
Review Comment:
Maybe just move the key argument to last?
##########
superset-frontend/plugins/plugin-chart-echarts/test/BigNumber/transformProps.test.ts:
##########
@@ -492,3 +492,61 @@ describe('BigNumberWithTrendline - Aggregation Tests', () => {
expect(transformed.bigNumber).toStrictEqual(10);
});
});
+
+describe('BigNumberWithTrendline - AUTO Mode Currency', () => {
Review Comment:
Let's not add immediate tech debt here and use a flat testing structure
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -723,3 +737,129 @@ describe('legend sorting', () => {
]);
});
});
+
+describe('EchartsTimeseries AUTO Mode Currency', () => {
Review Comment:
ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3747134790
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20977330697) for [dabc514](https://github.com/apache/superset/commit/dabc514)
• **Environment:** http://44.244.135.162:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3739705912
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20928462252) for [b941688](https://github.com/apache/superset/commit/b941688)
• **Environment:** http://16.147.27.92:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3730937024
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/20867991390) for [8abf362](https://github.com/apache/superset/commit/8abf362)
• **Environment:** http://44.243.198.140:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3739631488
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/20928462252) for [b941688](https://github.com/apache/superset/commit/b941688)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36416:
URL: https://github.com/apache/superset/pull/36416#issuecomment-3739817752
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Dynamic currency [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on code in PR #36416:
URL: https://github.com/apache/superset/pull/36416#discussion_r2683103861
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx:
##########
@@ -157,6 +159,47 @@ const StyledTableTabWrapper = styled.div`
}
`;
+const DefaultColumnSettingsContainer = styled.div`
+ ${({ theme }) => css`
+ margin-bottom: ${theme.sizeUnit * 4}px;
+ `}
+`;
+
+const DefaultColumnSettingsTitle = styled.h4`
+ ${({ theme }) => css`
+ margin: 0 0 ${theme.sizeUnit * 2}px 0;
+ font-size: ${theme.fontSizeLG}px;
+ font-weight: ${theme.fontWeightMedium};
+ color: ${theme.colorText};
+ `}
+`;
+
+const DefaultColumnSettingsFields = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-direction: column;
+ gap: ${theme.sizeUnit * 3}px;
+ `}
+`;
+
+const FieldWrapper = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-direction: column;
+ gap: ${theme.sizeUnit}px;
+ `}
+`;
+
+const FieldLabelWithTooltip = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ align-items: center;
+ gap: ${theme.sizeUnit}px;
+ font-size: ${theme.fontSizeSM}px;
+ color: ${theme.colorTextLabel};
+ `}
+`;
Review Comment:
Good catch! I'll update it to use the Flex component for the pure layout containers (DefaultColumnSettingsFields and FieldWrapper). Keeping FieldLabelWithTooltip as a styled component since it has font styling beyond flex layout
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org