refactor(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr` #24942

ege-st · 2023-08-10T01:01:28Z

SUMMARY

Refactoring the Pinot plugin for Superset to bring it inline with how plugins operate in the latest version of Superset.

This refactoring is also addressing two bugs in the Apache Pinot DB Engine spec:

If a column was marked as temporal and no time grain was provided then the query would be constructed with illegal {} around the column name causing Pinot to reject the query as syntactically invalid. The code has been updated to remove the incorrect {}
In some cases, get_timestamp_expr will be called by models.adhoc_column_to_sqla and None is explicitly passed for the pdf parameter. This would cause the Pinot get_timestamp_expr to fault. To fix this, the models.adhoc_column_to_sqla method is updated to get the python_date_format for a column (if that data is available). (Without this fix, queries created for charts simply do not use the date format a user sets for a temporal column).

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Connected to a Pinot data set and marked a long column as epoch_ms and tested creating a Bar Chart V2 to confirm that the query would be correctly constructed (this tested the issue where None is passed for the pdf parameter.
Setting a String column as temporal and using %Y-%m-%d as the PDF value and confirming that when a chart is created the correct Date Time conversion function is used to parse the text date time.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

… passed to get_timestamp_expr

github-actions

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

john-bodley · 2023-08-10T16:58:54Z

superset/db_engine_specs/pinot.py

        if time_grain:
            granularity = cls.get_time_grain_expressions().get(time_grain)
            if not granularity:
                raise NotImplementedError(f"No pinot grain spec for '{time_grain}'")
        else:
-            return TimestampExpression("{{col}}", col)
+            return TimestampExpression("{col}", col)


Would you also mind adding some unit tests for Pinot which cover the get_timestamp_expr function. You can find many other examples of this in the other DB engine specs.

Sure, not a problem.

john-bodley · 2023-08-10T17:20:26Z

superset/connectors/sqla/models.py

@@ -1011,7 +1013,7 @@ def adhoc_column_to_sqla(  # pylint: disable=too-many-locals
        if is_dttm and has_timegrain:
            sqla_column = self.db_engine_spec.get_timestamp_expr(
                col=sqla_column,
-                pdf=None,
+                pdf=pdf,


I'm concerned that this change may actually break existing logic—given it was explicitly set to None. Would you mind adding a unit test for this which helps not just to provide code coverage, but also helps reviewers et al. grok the consequence of the change.

@zhaoyongjie it seems like you added this logic in #21163 and thus you probably have the most context as to why we historically weren't defining the pdf variable.

Unfortunately, in my testing whenever I tried to create a chart or use a dashboard, if a column was marked as temporal it would always call get_timestamp_expr via adhoc_column_to_sqla which means that the user defined date format is never passed to the DB Engine Spec.

It's possible that the root cause of the issue is that get_timestamp_expr is being called through adhoc_column_to_sqla which it should be getting called via TableColumn.get_timestamp_expression (the only other call path to get_timestamp_expr I could find. But all my tests pointed to adhoc_column_to_sqla being the root cause.

@john-bodley the "pdf" is a shortcut for "date format (seconds or milliseconds)", this code was existing in many years, the "pdf" only used in Calculated Column and Columns from database, but not used in Adhoc expression, so we shouldn't make this change.

@ege-st

The design of current Pinot DB spec is completely incorrect. Maintaining our own Pinot driver and db_spec should solve your issue.

class PinotEngineSpec(BaseEngineSpec): # pylint: disable=abstract-method engine = "pinot" engine_name = "Apache Pinot" allows_subqueries = False allows_joins = False allows_alias_in_select = True allows_alias_in_orderby = False # https://docs.pinot.apache.org/users/user-guide-query/supported-transformations#datetime-functions _time_grain_expressions = { None: "{col}", "PT1S": "CAST(DATE_TRUNC('second', CAST({col} AS TIMESTAMP)) AS TIMESTAMP)", "PT1M": "CAST(DATE_TRUNC('minute', CAST({col} AS TIMESTAMP)) AS TIMESTAMP)", "PT5M": "CAST(ROUND(DATE_TRUNC('minute', CAST({col} AS TIMESTAMP)), 300000) as TIMESTAMP)", "PT10M": "CAST(ROUND(DATE_TRUNC('minute', CAST({col} AS TIMESTAMP)), 600000) as TIMESTAMP)", "PT15M": "CAST(ROUND(DATE_TRUNC('minute', CAST({col} AS TIMESTAMP)), 900000) as TIMESTAMP)", "PT30M": "CAST(ROUND(DATE_TRUNC('minute', CAST({col} AS TIMESTAMP)), 1800000) as TIMESTAMP)", "PT1H": "CAST(DATE_TRUNC('hour', CAST({col} AS TIMESTAMP)) AS TIMESTAMP)", "P1D": "CAST(DATE_TRUNC('day', CAST({col} AS TIMESTAMP)) AS TIMESTAMP)", "P1W": "CAST(DATE_TRUNC('week', CAST({col} AS TIMESTAMP)) AS TIMESTAMP)", "P1M": "CAST(DATE_TRUNC('month', CAST({col} AS TIMESTAMP)) AS TIMESTAMP)", "P3M": "CAST(DATE_TRUNC('quarter', CAST({col} AS TIMESTAMP)) AS TIMESTAMP)", "P1Y": "CAST(DATE_TRUNC('year', CAST({col} AS TIMESTAMP)) AS TIMESTAMP)", } @classmethod def column_datatype_to_string( cls, sqla_column_type: TypeEngine, dialect: Dialect ) -> str: # Pinot driver infers TIMESTAMP column as LONG, so make the quick fix. # When the Pinot driver fix this bug, current method could be removed. if isinstance(sqla_column_type, types.TIMESTAMP): return sqla_column_type.compile().upper() else: return super().column_datatype_to_string(sqla_column_type, dialect)

driver at: https://github.com/BurdaForward/pinot-dbapi/tree/bf_release

@zhaoyongjie I'm aware of what the Python Date Format (PDF) represents, though thanks for clarifying that this shouldn't be used for ad-hoc expressions.

Note we do already have a Pino DB engine spec, but maybe only adding the column_datatype_to_string method is required.

@ege-st I also wondered if this was an underlying issue with the Pino SQLAlchemy dialect. You might want to look into the visit_label method.

@zhaoyongjie I've confirmed that this error happens with the latest versions of Pinot: so Superset can't alias a projection to the same name as a column that already exists. I looked at the diff you provided but it appears to be diffing a version of models.py that is not the same as the one in the master branch.

@john-bodley could you provide some more detail? Is SQL Alchemy generating the alias name used in the projection? If so, then it could be an issue with the dialect, but if Superset generates the alias label then I'm not sure how the dialect can address this.

@ege-st the git-diffs are from Superset 2.1.0 branch. There aren't many changes, so you should apply the changes manually 🖨️🖨️🖨️

You should change this part of code on Master branch

superset/superset/models/core.py

Lines 965 to 979 in cacad56

def make_sqla_column_compatible(

self, sqla_col: ColumnElement, label: str | None = None

) -> ColumnElement:

"""Takes a sqlalchemy column object and adds label info if supported by engine.

:param sqla_col: sqlalchemy column instance

:param label: alias/label that column is expected to have

:return: either a sql alchemy column or label instance if supported by engine

"""

label_expected = label or sqla_col.name

# add quotes to tables

if self.db_engine_spec.allows_alias_in_select:

label = self.db_engine_spec.make_label_compatible(label_expected)

sqla_col = sqla_col.label(label)

sqla_col.key = label_expected

return sqla_col

@zhaoyongjie so I believe I figured out a workaround for the alias issue. If I just set allows_alias_in_select = False then the query generated by Superset does not use an alias and the query is then compatible with Pinot. So, I don't think any of the additional changes you kindly suggested are necessary.

One question that I have is: what is the purpose of the pdf that gets defined in the dataset configuration? Since it isn't passed into the engine spec when creating a chart, it can't be used in the query generation, so it doesn't seem to serve a purpose?

@ege-st

so I believe I figured out a workaround for the alias issue. If I just set allows_alias_in_select = False then the query generated by Superset does not use an alias and the query is then compatible with Pinot. So, I don't think any of the additional changes you kindly suggested are necessary.

Sounds good! It should be worked.

One question that I have is: what is the purpose of the pdf that gets defined in the dataset configuration? Since it isn't passed into the engine spec when creating a chart, it can't be used in the query generation, so it doesn't seem to serve a purpose?

I think the original design of "pdf" is a hard-code for getting a timestamp from a string, but a type conversion expression is more graceful, --- should push down the function and run in DB rather than calculate in client.

ege-st · 2023-08-15T14:15:02Z

Thanks, I will test the recommended changes.

…

On Tue, Aug 15, 2023 at 9:54 AM Yongjie Zhao ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In superset/connectors/sqla/models.py <#24942 (comment)>: > @@ -1011,7 +1013,7 @@ def adhoc_column_to_sqla( # pylint: disable=too-many-locals if is_dttm and has_timegrain: sqla_column = self.db_engine_spec.get_timestamp_expr( col=sqla_column, - pdf=None, + pdf=pdf, @ege-st <https://github.com/ege-st> the git-diffs are from Superset 2.1.0 branch. There aren't many changes, so you should apply the changes manually 🖨️🖨️🖨️ — Reply to this email directly, view it on GitHub <#24942 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BAASDJYAFUBWSB6AAIG4XETXVN5SDANCNFSM6AAAAAA3KYBQUU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

zhaoyongjie · 2023-08-16T07:54:22Z

@ege-st when you have time, could you fix the unit test and code style? If you need helping, feel free to ping me at GitHub or Slack DM.

zhaoyongjie

The code change LGTM, waiting for CI.

betodealmeida · 2023-08-16T17:25:49Z

superset/db_engine_specs/pinot.py

-        TimeGrain.QUARTER: True,
-        TimeGrain.YEAR: True,
+        None: "{col}",
+        "PT1S": "CAST(DATE_TRUNC('second', CAST({col} AS TIMESTAMP)) AS TIMESTAMP)",


Can we keep the enums here, eg, TimeGrain.SECOND instead of PT1S? It makes it much easier to read.

ege-st · 2023-08-22T18:30:33Z

@nytai could you take a look at this PR, I believe your review is the last one required.

Thanks!

ege-st · 2023-08-27T15:29:02Z

@zhaoyongjie @john-bodley Is the PR good to merge? Thanks!

zhaoyongjie · 2023-08-27T16:18:36Z

I’ll check it out tonight and let you know.

On Sun 27. Aug 2023 at 17:29, Erich ***@***.***> wrote: @zhaoyongjie <https://github.com/zhaoyongjie> @john-bodley <https://github.com/john-bodley> Is the PR good to merge? Thanks! — Reply to this email directly, view it on GitHub <#24942 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPMKURVVPL3EOQXF4GHMRTXXNRUVANCNFSM6AAAAAA3KYBQUU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Best regards, Yongjie

zhaoyongjie · 2023-08-27T16:21:21Z

Could you mind to change subtitle of merge request? This mr is refacoring of the Pinot spec rather than fix it.

On Sun 27. Aug 2023 at 17:29, Erich ***@***.***> wrote: @zhaoyongjie <https://github.com/zhaoyongjie> @john-bodley <https://github.com/john-bodley> Is the PR good to merge? Thanks! — Reply to this email directly, view it on GitHub <#24942 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPMKURVVPL3EOQXF4GHMRTXXNRUVANCNFSM6AAAAAA3KYBQUU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Best regards, Yongjie

ege-st · 2023-08-27T16:28:34Z

@zhaoyongjie Done.

zhaoyongjie · 2023-08-27T16:47:18Z

@ege-st Thanks for tackling this problem!

…ot being passed to `get_timestamp_expr` (#24942) (cherry picked from commit c2a21d2)

martin-raymond · 2023-10-25T13:36:39Z

Hi @ege-st ! what do you think of #25749 ? we think it might be related to your fix

ege-st · 2023-10-25T15:18:35Z

Hi @martin-raymond, I'll take a look today and let you know.

martin-raymond · 2023-10-30T12:06:59Z

@ege-st do you have any news ?

…ot being passed to `get_timestamp_expr` (apache#24942)

ege-st added 8 commits August 8, 2023 10:51

implementation notes

d60873e

Initial work to fix bug

2367233

clean up code

78d43a9

Code cleanup

0fd9b9e

Code cleanup

1301e22

Add comment

ccdfe8d

Simpler code

c83dc2b

Fixed a bug where the date format for a temporal column was not being…

b7d494a

… passed to get_timestamp_expr

pull-request-size bot added the size/XS label Aug 10, 2023

github-actions bot reviewed Aug 10, 2023

View reviewed changes

john-bodley reviewed Aug 10, 2023

View reviewed changes

john-bodley requested a review from betodealmeida August 10, 2023 16:59

rusackas requested a review from nytai August 10, 2023 17:01

john-bodley reviewed Aug 10, 2023

View reviewed changes

john-bodley requested a review from zhaoyongjie August 10, 2023 17:25

ege-st added 2 commits August 14, 2023 09:30

Apply recommendations from PR feedback

912fc24

Merge branch 'master' into pinot-bug-fix

61ddf51

ege-st added 3 commits August 15, 2023 19:31

Do not use alias on select

01a041c

undo unneeded changes

52268a1

undo unneeded changes

f9ef8e4

pull-request-size bot added size/L and removed size/XS labels Aug 15, 2023

add tests and fix linting errors

8fa3524

zhaoyongjie approved these changes Aug 16, 2023

View reviewed changes

ege-st added 3 commits August 16, 2023 10:52

working on unit tests

e1c2a75

Fixing unit tests

31d36cf

Remove unused import

8a9ed76

betodealmeida requested changes Aug 16, 2023

View reviewed changes

fixed formatting

399abf7

betodealmeida approved these changes Aug 17, 2023

View reviewed changes

ege-st added 4 commits August 19, 2023 09:41

fix integration test

15fc982

formatting

1e5cfc9

fixing integration test

4c6af11

Fix integration tests

88f3f23

ege-st added 2 commits August 24, 2023 09:09

Fixing linter errors

45d5c60

Fixing linter errors

6c779ee

ege-st changed the title ~~fix(pinot): The python_date_format for a temporal column was not being passed to get_timestamp_expr~~ refactor(pinot): The python_date_format for a temporal column was not being passed to get_timestamp_expr Aug 27, 2023

zhaoyongjie merged commit c2a21d2 into apache:master Aug 27, 2023
33 checks passed

michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Aug 29, 2023

michael-s-molina pushed a commit that referenced this pull request Aug 30, 2023

refactor(pinot): The python_date_format for a temporal column was n…

76da1b5

…ot being passed to `get_timestamp_expr` (#24942) (cherry picked from commit c2a21d2)

martin-raymond mentioned this pull request Oct 24, 2023

refactor(pinot) regression on area chart (DB engine error) #25749

Closed

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024

refactor(pinot): The python_date_format for a temporal column was n…

b1b686d

…ot being passed to `get_timestamp_expr` (apache#24942)

mistercrunch added 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr` #24942

refactor(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr` #24942

ege-st commented Aug 10, 2023 •

edited

Loading

github-actions bot left a comment

john-bodley Aug 10, 2023

ege-st Aug 10, 2023

john-bodley Aug 10, 2023 •

edited

Loading

ege-st Aug 10, 2023

zhaoyongjie Aug 10, 2023 •

edited

Loading

zhaoyongjie Aug 10, 2023 •

edited

Loading

john-bodley Aug 10, 2023

john-bodley Aug 14, 2023

ege-st Aug 14, 2023

zhaoyongjie Aug 15, 2023 •

edited

Loading

ege-st Aug 15, 2023

zhaoyongjie Aug 15, 2023 •

edited

Loading

ege-st commented Aug 15, 2023 via email

zhaoyongjie commented Aug 16, 2023 •

edited

Loading

zhaoyongjie left a comment

betodealmeida Aug 16, 2023

ege-st Aug 16, 2023

betodealmeida Aug 17, 2023

ege-st commented Aug 22, 2023

ege-st commented Aug 27, 2023

zhaoyongjie commented Aug 27, 2023 via email

zhaoyongjie commented Aug 27, 2023 via email

ege-st commented Aug 27, 2023

zhaoyongjie commented Aug 27, 2023

martin-raymond commented Oct 25, 2023

ege-st commented Oct 25, 2023

martin-raymond commented Oct 30, 2023

	def make_sqla_column_compatible(
	self, sqla_col: ColumnElement, label: str \| None = None
	) -> ColumnElement:
	"""Takes a sqlalchemy column object and adds label info if supported by engine.
	:param sqla_col: sqlalchemy column instance
	:param label: alias/label that column is expected to have
	:return: either a sql alchemy column or label instance if supported by engine
	"""
	label_expected = label or sqla_col.name
	# add quotes to tables
	if self.db_engine_spec.allows_alias_in_select:
	label = self.db_engine_spec.make_label_compatible(label_expected)
	sqla_col = sqla_col.label(label)
	sqla_col.key = label_expected
	return sqla_col

refactor(pinot): The python_date_format for a temporal column was not being passed to get_timestamp_expr #24942

refactor(pinot): The python_date_format for a temporal column was not being passed to get_timestamp_expr #24942

Conversation

ege-st commented Aug 10, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

github-actions bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaoyongjie Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

zhaoyongjie Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaoyongjie Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaoyongjie Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

ege-st commented Aug 15, 2023 via email

zhaoyongjie commented Aug 16, 2023 • edited Loading

zhaoyongjie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ege-st commented Aug 22, 2023

ege-st commented Aug 27, 2023

zhaoyongjie commented Aug 27, 2023 via email

zhaoyongjie commented Aug 27, 2023 via email

ege-st commented Aug 27, 2023

zhaoyongjie commented Aug 27, 2023

martin-raymond commented Oct 25, 2023

ege-st commented Oct 25, 2023

martin-raymond commented Oct 30, 2023

refactor(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr` #24942

refactor(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr` #24942

ege-st commented Aug 10, 2023 •

edited

Loading

john-bodley Aug 10, 2023 •

edited

Loading

zhaoyongjie Aug 10, 2023 •

edited

Loading

zhaoyongjie Aug 10, 2023 •

edited

Loading

zhaoyongjie Aug 15, 2023 •

edited

Loading

zhaoyongjie Aug 15, 2023 •

edited

Loading

zhaoyongjie commented Aug 16, 2023 •

edited

Loading