Repository navigation
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| if isinstance(sqla_type, (Date, DateTime)): | ||
| try: | ||
| df[column_name] = pd.to_datetime(df[column_name]) | ||
| df[column_name] = pd.to_datetime(df[column_name]).dt.as_unit("ns") |
There was a problem hiding this comment.
Suggestion: Mixed-offset timestamps can make pd.to_datetime return object dtype; .dt.as_unit then raises AttributeError, rejecting otherwise valid date imports.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Type error
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dataset/importers/v1/utils.py
**Line:** 773:773
**Comment:**
*Type Error: Mixed-offset timestamps can make `pd.to_datetime` return object dtype; `.dt.as_unit` then raises `AttributeError`, rejecting otherwise valid date imports.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 34048a8.
The code checks that pd.to_datetime returned a datetime dtype before calling .dt.as_unit. Other results fall through to per-value conversion instead of raising on .dt.
If that's not right, unresolve this thread and CodeAnt will leave it open.
| # dropna=False preserves restored all-NaN metric rows that would otherwise | ||
| # be silently dropped by stack's default dropna=True behavior. | ||
| df = df.stack(level=0, dropna=False).unstack() | ||
| df = df.stack(level=0, future_stack=True).unstack() |
There was a problem hiding this comment.
Suggestion: When drop_missing_columns=True leaves only observed metric/category pairs, stacking and unstacking recreates absent pairs as all-NaN columns, defeating the option.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Logic error
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/pandas_postprocessing/pivot.py
**Line:** 407:407
**Comment:**
*Logic Error: When `drop_missing_columns=True` leaves only observed metric/category pairs, stacking and unstacking recreates absent pairs as all-NaN columns, defeating the option.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 34048a8.
The columns are reshaped by reordering and sorting the MultiIndex levels, rather than stacking and unstacking, which would recreate absent pairs.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 6b5b198.
The column reshape now uses reorder_levels and sort_index instead of stacking and unstacking, preserving only the observed metric/category pairs.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of fdeb866.
The column reshape now uses reorder_levels and sort_index instead of stacking and unstacking, preserving only the observed metric/category pairs.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 176ea8d.
The column reshape now uses reorder_levels and sort_index instead of stacking and unstacking, preserving only the observed metric/category pairs.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 8b34fed.
The column reshape now uses reorder_levels and sort_index instead of stacking and unstacking, preserving only the observed metric/category pairs.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of bd9c46a.
The column reshape now uses reorder_levels and sort_index instead of stacking and unstacking, preserving only the observed metric/category pairs.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of bc0bfdf.
The reshape now reorders the existing column levels with reorder_levels and sorts them, rather than stacking and unstacking; this preserves only the metric/category pairs already present.
If that's not right, unresolve this thread and CodeAnt will leave it open.
| for column in metrics: | ||
| if is_object_dtype(df[column]): | ||
| df.loc[:, column] = to_numeric(df[column], errors="coerce") | ||
| if is_object_dtype(df[column]) or is_string_dtype(df[column]): | ||
| df[column] = to_numeric(df[column], errors="coerce") |
There was a problem hiding this comment.
Suggestion: Malformed values in string/object metrics become NaN, so nan-aware statistics silently omit them and return misleading boxplot results.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Logic error
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/pandas_postprocessing/boxplot.py
**Line:** 137:139
**Comment:**
*Logic Error: Malformed values in string/object metrics become NaN, so nan-aware statistics silently omit them and return misleading boxplot results.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 34048a8.
String/object metric columns are converted with to_numeric(errors="raise"); malformed values now raise InvalidPostProcessingError instead of being silently turned into NaN.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 6b5b198.
Object and string metric columns are converted with to_numeric(errors="raise"); malformed values now raise InvalidPostProcessingError instead of becoming NaN.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of fdeb866.
Object and string metric columns are converted with to_numeric(errors="raise"); malformed values now raise InvalidPostProcessingError instead of becoming NaN.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 176ea8d.
Object and string metric columns are converted with to_numeric(errors="raise"); malformed values now raise InvalidPostProcessingError instead of becoming NaN.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 8b34fed.
Object and string metric columns are converted with to_numeric(errors="raise"); malformed values now raise InvalidPostProcessingError instead of becoming NaN.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of bd9c46a.
Object and string metric columns are converted with to_numeric(errors="raise"); malformed values now raise InvalidPostProcessingError instead of becoming NaN.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of bc0bfdf.
Non-numeric metric columns are converted with to_numeric(errors="raise"). If conversion fails, the code raises InvalidPostProcessingError instead of silently turning malformed values into NaN.
If that's not right, unresolve this thread and CodeAnt will leave it open.
|
|
||
| assert json_schema == { | ||
| "type": "object", | ||
| "additionalProperties": False, |
There was a problem hiding this comment.
These schema assertions now expect additionalProperties: False (and additionalProperties: {} for dict fields), but requirements/base.txt still pins apispec==6.6.1. On 6.6.1 the marshmallow converter only emits additionalProperties when Meta.unknown is set; a plain Schema with a Dict field renders as {'type': 'object', ...} there. So these equality asserts (and the matching ones in postgres_tests.py, databases/api_test.py and test_available) would fail on the existing Python 3.11 lockfile baseline and only pass with the unpinned resolve used for 3.15. Should the expected shape be derived from the installed apispec, or should the apispec bound move so both baselines produce the same schema?
|
|
||
| RUN --mount=type=cache,target=${SUPERSET_HOME}/.cache/uv \ | ||
| /app/docker/pip-install.sh --requires-build-essential -r requirements/base.txt | ||
| --mount=type=bind,source=.,target=/src \ |
There was a problem hiding this comment.
Bind-mounting the whole build context (source=.) into the dependency-install RUN ties that layer's cache key to every non-ignored file, so any source edit reruns the requirements/base.txt install even on the default Python 3.11, where the script never reads /src. That undoes the ordering the comment above describes (install dependencies before copying source). Could the mount be limited to what the 3.15 path needs (e.g. pyproject.toml and superset-core), or the 3.15 branch moved into its own RUN?
| SOURCE_ROOT="${BUILD_ROOT}" | ||
| fi | ||
| /app/docker/pip-install.sh --requires-build-essential \ | ||
| --no-sources --no-editable "${SOURCE_ROOT}/superset-core" "${SOURCE_ROOT}" |
There was a problem hiding this comment.
--no-editable is not accepted by every uv the Dockerfile allows: it installs uv>=0.11.0, and uv pip install --no-editable fails with unexpected argument '--no-editable' found on 0.11.3. A 3.15 image build with a cached or pinned older uv would fail at this step. Both targets are plain paths and --no-sources already skips the editable source entries, so is the flag needed, or should the uv floor be raised?
| @@ -0,0 +1,17 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
This new script and tests/unit_tests/docker_app_dependencies_test.py have no ASF license header, and neither path is covered by .rat-excludes, so the Apache RAT license check will flag them; sibling docker/pip-install.sh and the new test_lint_metadata.py both carry it. Can the standard header be added to both files?
| SQLite migrations passed locally on Windows for all five interpreter versions, | ||
| including CPython 3.15.0 final. | ||
|
|
||
| A local Podman container running Debian 13 with CPython 3.15.2 also passed |
There was a problem hiding this comment.
From here the page reads as a private test log (Podman/Hyper-V host notes, counts like 32,821 passed, an unnamed "raw-cursor join truncation fix", Playwright case counts) that readers cannot reproduce from the linked workflow, which runs a much narrower set, and the numbers go stale on the next test change. Should the run-specific evidence live in the PR description, leaving this page with the install command, the support policy and the known limitations?
| with app.app_context(): | ||
| raise SystemExit(pytest.main([ | ||
| '--noconftest', '-o', 'addopts=', | ||
| 'tests/unit_tests/db_engine_specs/test_lint_metadata.py', |
There was a problem hiding this comment.
This is the only job that runs on 3.15/pandas 3, but the PR also changes pandas-sensitive code in superset/commands/dataset/importers/v1/utils.py, superset/models/helpers.py and the metadb cursor wrapper, whose tests (tests/unit_tests/commands/importers/v1/utils_test.py, tests/unit_tests/models/helpers_test.py, test_superset_joins_with_limit_drops_fanout_matches in tests/unit_tests/extensions/test_sqlalchemy.py) are not in this selection and otherwise run only on 3.11. The paths: filter also omits Dockerfile and docker/**. A later change to those files would never run against pandas 3.1. Should those tests and paths be added so the experimental 3.15 claim stays checked in CI?
| _df = _df.fillna(fill_value) | ||
| elif method == "linear": | ||
| _df = df.resample(rule).interpolate() | ||
| _df = df.resample(rule).asfreq() |
There was a problem hiding this comment.
linear now interpolates only numeric columns, so a datetime column that used to be filled between rows comes back as NaT. On pandas 2.3.3, with index 2024-01-01, 2024-01-03 and a column 2024-02-01, 2024-02-03, daily Resampler.interpolate() gives 2024-02-02 for the middle row and this path gives NaT. Should datetime columns stay in the interpolated set?
| # 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'", |
There was a problem hiding this comment.
Moving 3.15 to pandas 3 makes string columns arrive as the str dtype, but df_columns_to_num in superset/common/utils/dataframe_utils.py only converts columns whose dtype.type == np.object_. A driver that returns metrics as strings (the ClickHouse SUM() case that helper exists for) now keeps ['10', '20'] as strings instead of [10.0, 20.0], and the next numeric step (e.g. the timeseries pivot mean) raises TypeError. The new boxplot code already handles this with is_string_dtype; should df_columns_to_num accept string dtypes too, with a test that builds the frame with str columns?
| # 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'", |
There was a problem hiding this 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?
| """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): |
There was a problem hiding this 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?
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: "#/components/schemas/SelectStarResponseSchema" |
There was a problem hiding this 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?
|
|
||
| # convert to numeric, coercing errors to NaN | ||
| df.loc[:, column] = to_numeric(df[column], errors="coerce") | ||
| df[column] = to_numeric(df[column], errors="coerce") |
There was a problem hiding this 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]]?
| df[column_name] = converted | ||
| converted = pd.to_datetime(df[column_name]) | ||
| if pd.api.types.is_datetime64_any_dtype(converted): | ||
| df[column_name] = converted.dt.as_unit("ns") |
There was a problem hiding this comment.
Forcing as_unit("ns") here (and in the per-value path below) turns a valid far-future date such as 3118-01-01 into NaT: the nanosecond cast overflows, the ValueError is swallowed, the fallback overflows again, and load_data() then writes NULL for that cell. The neighbouring test_far_future_date_strings_convert_successfully asserts the timestamp survives with no warning, so that test should fail against this code. Could the conversion keep the coarser resolution pandas already returned instead of casting to ns?
| reader.file_to_dataframe(create_columnar_file(COLUMNAR_DATA)) | ||
| assert ( | ||
| str(ex.value) | ||
| str(ex.value).replace(": large_string", ": string") |
There was a problem hiding this comment.
The actual message is normalised with .replace(": large_string", ": string"), but the expected literal below still contains : large_string for Name, City and Birth. After the replace the left side can never contain large_string, so this equality fails whichever Arrow type name the reader reports, including when it correctly raises DatabaseUploadFailed for xpto. Should the same normalisation be applied to the expected string (or the assertion pinned to the stable No match for FieldRef.Name(xpto) prefix)?
| raise | ||
| converted = pd.to_datetime( | ||
| df[col.col_label], | ||
| utc=True, |
There was a problem hiding this comment.
On the Mixed timezones fallback the column is converted to UTC, and _coerce_temporal_join_series later strips the tz, so the join compares UTC wall clocks. A comparison series with a single offset keeps its local wall clock through the same path. For a %z column spanning a DST change (noon at -05:00 and noon at -04:00), the source becomes 17:00/16:00 while the single-offset side stays 12:00, so rows that matched by local time no longer join. Is UTC the intended basis for these joins, or should each value keep its own local wall clock?
| The source checkout includes experimental Python 3.15 compatibility. Its package | ||
| requirements select NumPy `>=2.5.4,<2.6` and pandas `>=3.1.0,<3.2` on Python 3.15 | ||
| and newer. Python 3.11 through 3.14 retain NumPy `>=1.23.5,<2.5` and pandas | ||
| `>=2.3.3,<2.4`; the minimum Python version has not changed. These source changes |
There was a problem hiding this comment.
This says Python 3.11-3.14 keep pandas >=2.3.3,<2.4, but pyproject.toml now requires pandas[excel]>=3.0.6,<3.1 for python_version < '3.15', and the generated requirements pin 3.0.6. Someone constraining pandas to the documented range gets an unsatisfiable install. Can this read >=3.0.6,<3.1?
| 'tests/unit_tests/result_set_test.py', | ||
| 'tests/unit_tests/common/test_dataframe_utils.py', | ||
| 'tests/unit_tests/commands/importers/v1/utils_test.py', | ||
| 'tests/unit_tests/models/helpers_test.py', |
There was a problem hiding this comment.
tests/unit_tests/utils/test_date_parsing.py is not in this pytest target list, so the new mixed-offset regression for normalize_dttm_col (the Berlin repeated-hour case, where both timestamps must normalise to local 02:30 without raising) never runs on the Python 3.15 / pandas 3.1 combination this job exists to cover. Regular unit CI runs on 3.11 and cannot exercise it. Could that file be added here and to the paths: triggers?
SUMMARY
Replaces #45821 with a reviewable, line-ending-clean diff. Related to #45019.
The previous PR included repository-wide CRLF/LF churn. This branch is rebuilt from the same original base and preserves its substantive changes without that churn: 49 files changed, 680 insertions, 135 deletions, in one commit. The original branch has not been rewritten.
>=2.5.4,<2.6and pandas>=3.1.0,<3.2on Python 3.15 using dependency markers; preserve the existing ranges for Python 3.11-3.14.richdirectly for CLI imports and document installation, validation and known limitations.The frontend Node pin remains
v24.16.0with LF line endings, matching the original base. This avoids carrying the CRLF-corrupted version string that blocked the previous Netlify docs preview. No Netlify configuration or Node version change is needed.Beyond removing line-ending churn, this replacement adds an explicit union type annotation in the container regression test and the shell script's final newline, as required by local checks.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable. No intentional UI changes.
TESTING INSTRUCTIONS
Create an isolated Python 3.15 environment and install from package metadata, not the generated Python 3.11 requirement files:
Run the new compatibility workflow and the container regression tests:
Validation of this replacement commit:
type: ignoreatsuperset/db_engine_specs/mysql.py:680. The same finding was reproduced on the unchanged original base with the same selection of modified existing files. The new test's MyPy error was corrected before this commit.Remaining limits: this is experimental compatibility, not production certification. The complete official image build remains unverified because the reference host ran out of memory during frontend minimization, although the isolated Python 3.15 dependency stage passed. The documented Gunicorn gthread shutdown limitation remains unresolved. The new Netlify preview and PR CI still need to run;
/healthis liveness, not dependency readiness.ADDITIONAL INFORMATION