sadpandajoe commented on code in PR #45823:
URL: https://github.com/apache/superset/pull/45823#discussion_r4414650055
##########
superset/models/helpers.py:
##########
@@ -376,6 +376,8 @@ def _parse_temporal_join_values(series: pd.Series,
column_name: str) -> pd.Serie
"""Parse working temporal values and wrap pandas parser-policy errors."""
if pd.api.types.is_datetime64_any_dtype(series):
return series
+ if _has_multiple_utc_offsets(series):
Review Comment:
This guard only covers the join-value parser. The normal chart path in
`normalize_dttm_col` (`superset/utils/core.py`) still calls
`pd.to_datetime(..., utc=False, format="%Y-%m-%dT%H:%M:%S%z",
errors="coerce")`, and pandas 3 raises `ValueError: Mixed timezones detected`
there for values like `2024-10-27T02:30:00+02:00` and
`2024-10-27T02:30:00+01:00` instead of keeping both instants as 2.3 does. Is a
string temporal column with differing offsets expected to keep working on 3.15,
and if so should that path get the same handling?
##########
pyproject.toml:
##########
@@ -84,11 +84,13 @@ dependencies = [
"marshmallow-union>=0.1.15.post1",
"msgpack>=1.2.3, <1.3",
"nh3>=0.3.7, <0.4",
- "numpy>=1.23.5, <2.5",
+ "numpy>=1.23.5, <2.5; python_version < '3.15'",
+ "numpy>=2.5.4, <2.6; python_version >= '3.15'",
"packaging",
# --------------------------
# pandas and related (wanting pandas[performance] without numba as it's
100+MB and not needed)
- "pandas[excel]>=2.3.3, <2.4",
+ "pandas[excel]>=2.3.3, <2.4; python_version < '3.15'",
+ "pandas[excel]>=3.1.0, <3.2; python_version >= '3.15'",
Review Comment:
Selecting pandas 3 here means the resample rules the chart control still
offers (`1T`, `1H`, `1M`, `1AS`, `1A` in `advancedAnalytics.tsx`) are rejected
on 3.15. `resample()` passes `rule` straight to `to_offset()`, so a chart with
`resample_rule="1M"` and `resample_method="sum"` that returns monthly totals on
3.11-3.14 now fails with `Invalid resample rule: 1M`, saved charts included.
Should the legacy aliases be normalized to the pandas 3 spellings (`min`, `h`,
`ME`, `YS`, `YE`) before validation and resampling, with a test that covers
them?
##########
superset/utils/pandas_postprocessing/histogram.py:
##########
@@ -75,7 +75,7 @@ def histogram(
return df
# convert to numeric, coercing errors to NaN
- df.loc[:, column] = to_numeric(df[column], errors="coerce")
+ df[column] = to_numeric(df[column], errors="coerce")
Review Comment:
This assignment is what makes numeric conversion work for pandas string
columns, but none of the histogram tests build a string-dtype column, and the
mixed-string test makes no assertion when the call succeeds. Reverting to the
old `.loc[:, column]` assignment would pass the suite while failing on pandas
3. Could a test call `histogram(..., bins=2)` on `Series(["1", "2", "3", "4"],
dtype="string")` (and `"str"`) and assert the counts `[[2, 2]]`?
##########
superset/databases/api.py:
##########
@@ -1240,6 +1250,34 @@ def table_extra_metadata(self, pk: int) -> FlaskResponse:
return self.response(200, **payload)
@expose("/<int:pk>/select_star/<path:table_name>/", methods=("GET",))
+ @protect()
+ @permission_name("read")
+ def select_star_without_schema(self, pk: int, table_name: str) ->
FlaskResponse:
+ """Get a select star statement using the database's default schema.
+ ---
+ get:
+ summary: Get database select star for table in the default schema
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ description: The database id
+ - in: path
+ schema:
+ type: string
+ name: table_name
+ description: Table name
+ responses:
+ 200:
+ description: SQL statement for a select star for table
+ content:
+ application/json:
+ schema:
+ $ref: "#/components/schemas/SelectStarResponseSchema"
Review Comment:
The new schema-free route documents only the 200 response, so
`/api/v1/database/{pk}/select_star/{table_name}/` no longer advertises the
400/401/404/422/500 responses it had before, even though it delegates to the
same handler and still returns them (a missing table still gives a 404 with
`Table not found on the database`). Clients validating against the OpenAPI
document would now reject those errors. Can the wrapper docstring keep the same
error responses so the regenerated spec matches?
--
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]
