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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
