Copilot commented on code in PR #42432:
URL: https://github.com/apache/superset/pull/42432#discussion_r3662584466
##########
tests/unit_tests/db_engine_specs/test_sqlite.py:
##########
@@ -131,3 +131,34 @@ def test_time_grain_expressions(dttm: str, grain: str,
expected: str) -> None:
with engine.connect() as connection:
result = connection.execute(text(sql)).scalar()
assert result == expected
+
+
[email protected](
+ "year,expected",
+ [
+ (2013, "2013-01-01 00:00:00"),
+ (2013.0, "2013-01-01 00:00:00"),
+ (None, None),
+ ],
+)
+def test_year_pdf_time_grain(year: Optional[float], expected: Optional[str])
-> None:
Review Comment:
The parametrized inputs include both `2013` (an `int`) and `2013.0` (a
`float`), but the test signature annotates `year` as `Optional[float]`. If this
repo runs type checking on tests, this will be flagged. Consider widening the
annotation to `Optional[float | int]` (or `Optional[Union[int, float]]`) to
reflect actual inputs.
##########
superset/db_engine_specs/sqlite.py:
##########
@@ -126,6 +126,18 @@ class SqliteEngineSpec(BaseEngineSpec):
def epoch_to_dttm(cls) -> str:
return "datetime({col}, 'unixepoch')"
+ @classmethod
+ def year_to_dttm(cls) -> str:
+ # SQLite's date functions parse a 'YYYY-01-01' string just fine, but
won't
+ # accept a bare integer/real year (it's read as a Julian day number
instead).
+ # The CASE guard is needed because printf() treats a NULL argument as
0,
+ # which would otherwise turn a missing year into '0000-01-01' rather
than
+ # propagating the NULL.
+ return (
+ "CASE WHEN {col} IS NULL THEN NULL "
+ "ELSE printf('%04d-01-01', CAST({col} AS INTEGER)) END"
+ )
Review Comment:
The base `year_to_dttm()` docstring says this hook should return an
expression producing the Jan 1 *datetime* of the year, but the SQLite
implementation returns a date string (`YYYY-01-01`) rather than an explicit
datetime. To match the contract (and ensure consistent behavior when no
time-grain truncation is applied), consider returning a
`datetime(...)`/`DATETIME(...)` expression around the constructed string (while
preserving the current `NULL` propagation).
##########
superset/db_engine_specs/base.py:
##########
@@ -1110,6 +1110,14 @@ def get_timestamp_expr(
time_expr = time_expr.replace("{col}", cls.epoch_to_dttm())
elif pdf == "epoch_ms":
time_expr = time_expr.replace("{col}", cls.epoch_ms_to_dttm())
+ elif pdf == "%Y":
+ # a bare four-digit year (e.g. the `year` column on the
`video_game_sales`
+ # example dataset) has no native date type to lean on; without
this the
+ # column value is passed straight into the grain function below,
which
+ # every engine interprets as something other than a calendar year
(SQLite
+ # reads a bare integer as a Julian day number, for instance),
silently
+ # producing NULL for every row.
+ time_expr = time_expr.replace("{col}", cls.year_to_dttm())
Review Comment:
This introduces a cross-engine behavior change for any dataset using
`python_date_format == \"%Y\"`: for engine specs that don’t override
`year_to_dttm()`, `get_timestamp_expr()` will now raise `NotImplementedError`
where it previously produced an expression (even if potentially incorrect for
some engines). To avoid breaking existing non-SQLite configs, consider either
(a) providing a reasonable default `year_to_dttm()` implementation in
`BaseEngineSpec` that works for most SQL dialects, or (b) catching
`NotImplementedError` around `cls.year_to_dttm()` and falling back to the prior
behavior (or only applying this path when `time_grain` is set).
--
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]
