rusackas commented on code in PR #45772:
URL: https://github.com/apache/superset/pull/45772#discussion_r4393914632
##########
superset/db_engine_specs/pinot.py:
##########
@@ -46,8 +42,10 @@ class PinotEngineSpec(BaseEngineSpec):
],
"pypi_packages": ["pinotdb"],
"version_requirements": (
- "The Pinot extra requires pinotdb[sqlalchemy]>=8.0.0,<10.0.0."
- " Earlier releases declare SQLAlchemy below 2 in their SQLAlchemy
extra."
+ "The Pinot extra requires pinotdb[sqlalchemy]>=9.2.1,<10.0.0."
Review Comment:
Good catch, bumped the advertised floor to `>=9.2.2` to match the extra,
with 9.2.1 kept as the version that fixed the TIMESTAMP compile.
##########
tests/unit_tests/db_engine_specs/test_pinot.py:
##########
@@ -85,6 +86,29 @@ def test_timegrain_expressions(time_grain: str,
expected_result: str) -> None:
assert actual == expected_result
+def test_column_datatype_to_string_timestamp() -> None:
+ """
+ DB Eng Specs (pinot): TIMESTAMP columns compile as TIMESTAMP, not NUMERIC.
+
+ Regression test for startreedata/pinot-dbapi#224: pinotdb < 9.2.1 aliased
+ its TypeCompiler's ``visit_TIMESTAMP`` to ``visit_NUMERIC``, so Superset
+ special-cased TIMESTAMP in ``column_datatype_to_string``. The underlying
+ driver bug is fixed as of pinotdb 9.2.1 (see the ``pinot`` extra's floor
+ in pyproject.toml), so ``PinotEngineSpec`` no longer overrides this
+ method at all; this test guards against the installed pinotdb dialect
+ regressing back to the old, wrong behavior on a future bump.
+ """
+ pytest.importorskip("pinotdb.sqlalchemy")
Review Comment:
No, none that I can find. `requirements/development.in` has no `pinot`
extra, so this skips in CI and only runs for anyone with `pinotdb` installed
locally. @sadpandajoe I would rather not widen the dev lockfile for one
assertion, so I am treating it as a local guard. Want me to add the extra
anyway?
--
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]
