You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "ege-st (via GitHub)" <gi...@apache.org> on 2023/08/10 01:01:30 UTC
[GitHub] [superset] ege-st opened a new pull request, #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
ege-st opened a new pull request, #24942:
URL: https://github.com/apache/superset/pull/24942
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
This fixes two bugs in the Apache Pinot DB Engine spec:
1. 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 `{}`
2. 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
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
1. 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.
2. 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
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1293814884
##########
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,
Review Comment:
BTW, before making this change, also please check the latest Pinot that whether or not fix it in Pinot side.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ege-st commented on pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on PR #24942:
URL: https://github.com/apache/superset/pull/24942#issuecomment-1688709690
@nytai could you take a look at this PR, I believe your review is the last one required.
Thanks!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ege-st commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1290718365
##########
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,
Review Comment:
@zhaoyongjie do we just update this code to use the driver at https://github.com/BurdaForward/pinot-dbapi/tree/bf_release and that will allow ad-hoc expressions to work with Pinot? Or do I change the existing driver to follow the code snippet you provided above?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ege-st commented on pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on PR #24942:
URL: https://github.com/apache/superset/pull/24942#issuecomment-1694696260
@zhaoyongjie @john-bodley Is the PR good to merge? Thanks!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1297475185
##########
superset/db_engine_specs/pinot.py:
##########
@@ -30,93 +29,34 @@ class PinotEngineSpec(BaseEngineSpec): # pylint: disable=abstract-method
allows_alias_in_select = False
allows_alias_in_orderby = False
- # Pinot does its own conversion below
+ # https://docs.pinot.apache.org/users/user-guide-query/supported-transformations#datetime-functions
_time_grain_expressions = {
- TimeGrain.SECOND: "1:SECONDS",
- TimeGrain.MINUTE: "1:MINUTES",
- TimeGrain.FIVE_MINUTES: "5:MINUTES",
- TimeGrain.TEN_MINUTES: "10:MINUTES",
- TimeGrain.FIFTEEN_MINUTES: "15:MINUTES",
- TimeGrain.THIRTY_MINUTES: "30:MINUTES",
- TimeGrain.HOUR: "1:HOURS",
- TimeGrain.DAY: "1:DAYS",
- TimeGrain.WEEK: "week",
- TimeGrain.MONTH: "month",
- TimeGrain.QUARTER: "quarter",
- TimeGrain.YEAR: "year",
- }
-
- _python_to_java_time_patterns: dict[str, str] = {
- "%Y": "yyyy",
- "%m": "MM",
- "%d": "dd",
- "%H": "HH",
- "%M": "mm",
- "%S": "ss",
- }
-
- _use_date_trunc_function: dict[str, bool] = {
- TimeGrain.SECOND: False,
- TimeGrain.MINUTE: False,
- TimeGrain.FIVE_MINUTES: False,
- TimeGrain.TEN_MINUTES: False,
- TimeGrain.FIFTEEN_MINUTES: False,
- TimeGrain.THIRTY_MINUTES: False,
- TimeGrain.HOUR: False,
- TimeGrain.DAY: False,
- TimeGrain.WEEK: True,
- TimeGrain.MONTH: True,
- TimeGrain.QUARTER: True,
- TimeGrain.YEAR: True,
+ None: "{col}",
+ "PT1S": "CAST(DATE_TRUNC('second', CAST({col} AS TIMESTAMP)) AS TIMESTAMP)",
Review Comment:
Thank you!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1295153284
##########
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,
Review Comment:
@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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr` [superset]
Posted by "martin-raymond (via GitHub)" <gi...@apache.org>.
martin-raymond commented on PR #24942:
URL: https://github.com/apache/superset/pull/24942#issuecomment-1785046832
@ege-st do you have any news ?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ege-st commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1290658459
##########
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,
Review Comment:
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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1290668187
##########
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,
Review Comment:
@ege-st
The design of current Pinot DB spec is completely incorrect. Maintaining our own Pinot driver and db_spec should solve your issue.
```python
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)
@staticmethod
def alias_in_select_mutator(label: str) -> str:
return f"{label}__"
@classmethod
def get_timezone_expressions(
cls,
col: ColumnClause,
timezone: typing.Optional[str],
) -> ColumnClause:
if BF_EUROPE_BERLIN_TIMEZONE_COLUMN_SUFFIX in col.name:
return col
tpl = f"""
CAST(
FROMDATETIME(
DATETIMECONVERT(
{col},
'1:MILLISECONDS:EPOCH',
'1:MILLISECONDS:SIMPLE_DATE_FORMAT:yyyy-MM-dd HH:mm:ss.SSS tz({timezone})',
'1:MILLISECONDS'
),
'yyyy-MM-dd HH:mm:ss.SSS'
) AS TIMESTAMP
)""".strip()
return literal_column(tpl)
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie merged pull request #24942: refactor(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie merged PR #24942:
URL: https://github.com/apache/superset/pull/24942
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1295153284
##########
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,
Review Comment:
@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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1296224860
##########
superset/db_engine_specs/pinot.py:
##########
@@ -30,93 +29,34 @@ class PinotEngineSpec(BaseEngineSpec): # pylint: disable=abstract-method
allows_alias_in_select = False
allows_alias_in_orderby = False
- # Pinot does its own conversion below
+ # https://docs.pinot.apache.org/users/user-guide-query/supported-transformations#datetime-functions
_time_grain_expressions = {
- TimeGrain.SECOND: "1:SECONDS",
- TimeGrain.MINUTE: "1:MINUTES",
- TimeGrain.FIVE_MINUTES: "5:MINUTES",
- TimeGrain.TEN_MINUTES: "10:MINUTES",
- TimeGrain.FIFTEEN_MINUTES: "15:MINUTES",
- TimeGrain.THIRTY_MINUTES: "30:MINUTES",
- TimeGrain.HOUR: "1:HOURS",
- TimeGrain.DAY: "1:DAYS",
- TimeGrain.WEEK: "week",
- TimeGrain.MONTH: "month",
- TimeGrain.QUARTER: "quarter",
- TimeGrain.YEAR: "year",
- }
-
- _python_to_java_time_patterns: dict[str, str] = {
- "%Y": "yyyy",
- "%m": "MM",
- "%d": "dd",
- "%H": "HH",
- "%M": "mm",
- "%S": "ss",
- }
-
- _use_date_trunc_function: dict[str, bool] = {
- TimeGrain.SECOND: False,
- TimeGrain.MINUTE: False,
- TimeGrain.FIVE_MINUTES: False,
- TimeGrain.TEN_MINUTES: False,
- TimeGrain.FIFTEEN_MINUTES: False,
- TimeGrain.THIRTY_MINUTES: False,
- TimeGrain.HOUR: False,
- TimeGrain.DAY: False,
- TimeGrain.WEEK: True,
- TimeGrain.MONTH: True,
- TimeGrain.QUARTER: True,
- TimeGrain.YEAR: True,
+ None: "{col}",
+ "PT1S": "CAST(DATE_TRUNC('second', CAST({col} AS TIMESTAMP)) AS TIMESTAMP)",
Review Comment:
Can we keep the enums here, eg, `TimeGrain.SECOND` instead of `PT1S`? It makes it much easier to read.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr` [superset]
Posted by "martin-raymond (via GitHub)" <gi...@apache.org>.
martin-raymond commented on PR #24942:
URL: https://github.com/apache/superset/pull/24942#issuecomment-1779294826
Hi @ege-st ! what do you think of https://github.com/apache/superset/issues/25749 ? we think it might be related to your fix
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1290663501
##########
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,
Review Comment:
@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 but not used in Adhoc expression, so we shouldn't make this change.
<img width="895" alt="image" src="https://github.com/apache/superset/assets/2016594/4b6547bb-796e-4dd7-a2d5-07fcaff8b0a5">
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1290712357
##########
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,
Review Comment:
The Pinot provided time grain function so we just define `_time_grain_expressions` and apply `column_datatype_to_string` is enough.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on PR #24942:
URL: https://github.com/apache/superset/pull/24942#issuecomment-1680135706
@ege-st when you have time, could you fix fix the unit test and code style? If you need helping, feel free to ping me at GitHub or Slack DM.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #24942: refactor(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on PR #24942:
URL: https://github.com/apache/superset/pull/24942#issuecomment-1694711972
@ege-st Thanks for tackling this problem!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ege-st commented on pull request #24942: refactor(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on PR #24942:
URL: https://github.com/apache/superset/pull/24942#issuecomment-1694708330
@zhaoyongjie Done.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ege-st commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1291316703
##########
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,
Review Comment:
@zhaoyongjie Thanks, I will test out your recommendation today and update the PR accordingly.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1290452448
##########
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,
Review Comment:
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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1290426308
##########
superset/connectors/sqla/models.py:
##########
@@ -990,7 +990,9 @@ def adhoc_column_to_sqla( # pylint: disable=too-many-locals
time_grain = col.get("timeGrain")
has_timegrain = col.get("columnType") == "BASE_AXIS" and time_grain
is_dttm = False
+ pdf = None
if col_in_metadata := self.get_column(expression):
+ pdf = col_in_metadata.python_date_format
Review Comment:
Given that you're changing how the Python date format logic is used, would you mind adding a unit test for this?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1294628722
##########
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,
Review Comment:
@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
https://github.com/apache/superset/blob/cacad56172225351f3c22a488aac839575f688dc/superset/models/core.py#L965-L979
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on PR #24942:
URL: https://github.com/apache/superset/pull/24942#issuecomment-1694706995
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
> <https://github.com/apache/superset/pull/24942#issuecomment-1694696260>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAPMKURVVPL3EOQXF4GHMRTXXNRUVANCNFSM6AAAAAA3KYBQUU>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
Best regards,
Yongjie
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1290452448
##########
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,
Review Comment:
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 https://github.com/apache/superset/pull/21163 and thus you probably have the most context as to why we historically weren't defining the `pdf` variable.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1290426308
##########
superset/connectors/sqla/models.py:
##########
@@ -990,7 +990,9 @@ def adhoc_column_to_sqla( # pylint: disable=too-many-locals
time_grain = col.get("timeGrain")
has_timegrain = col.get("columnType") == "BASE_AXIS" and time_grain
is_dttm = False
+ pdf = None
if col_in_metadata := self.get_column(expression):
+ pdf = col_in_metadata.python_date_format
Review Comment:
Given that you're changing how the Python date format logic is used, would you mind adding a unit test for this?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ege-st commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1293815319
##########
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,
Review Comment:
@zhaoyongjie agreed, I started looking to see if this is a bug in Pinot or in Calcite but I also want to see if I can get the fix done entirely within Superset because upgrading the Superset version will be easier than upgrading the Pinot deployment and, just in case, the fix in Calcite or Pinot takes a while to get done.
I'll take a look at the Superset changes you suggested.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1290668187
##########
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,
Review Comment:
@ege-st
The design of current Pinot DB spec is completely incorrect. Maintaining our own Pinot driver and db_spec should solve your issue.
```python
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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1294628722
##########
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,
Review Comment:
@ege-st the git-diffs are from Superset 2.1.0 branch. There aren't many changes, so you should apply the changes manually 🖨️🖨️🖨️
##########
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,
Review Comment:
@ege-st the git-diffs are from Superset 2.1.0 branch. There aren't many changes, so you should apply the changes manually 🖨️🖨️🖨️
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ege-st commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1290653433
##########
superset/db_engine_specs/pinot.py:
##########
@@ -99,12 +101,13 @@ def get_timestamp_expr(
else:
seconds_or_ms = "MILLISECONDS" if pdf == "epoch_ms" else "SECONDS"
tf = f"1:{seconds_or_ms}:EPOCH"
+
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)
Review Comment:
Sure, not a problem.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1290691643
##########
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,
Review Comment:
@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](https://github.com/apache/superset/blob/master/superset/db_engine_specs/pinot.py) a Pino DB engine spec, but maybe only adding the `column_datatype_to_string` method is required.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1291133076
##########
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,
Review Comment:
Hi @ege-st I think you only replace content of the `pinot.py` with codes snippet above which is to resolve this PR issue. The owner custom driver is for other issue, e.g. connection timeout and so on.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1291133076
##########
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,
Review Comment:
Hi @ege-st I think you only replace content of the `pinot.py` with codes snippet above which is to resolve this PR issue. The owner custom driver is for other issues, e.g. connection timeout and so on.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ege-st commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1293973636
##########
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,
Review Comment:
@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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ege-st commented on pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on PR #24942:
URL: https://github.com/apache/superset/pull/24942#issuecomment-1679006741
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
> <https://github.com/apache/superset/pull/24942#discussion_r1294628722>:
>
> > @@ -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
> <https://github.com/apache/superset/pull/24942#discussion_r1294628722>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BAASDJYAFUBWSB6AAIG4XETXVN5SDANCNFSM6AAAAAA3KYBQUU>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1290452448
##########
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,
Review Comment:
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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1290426308
##########
superset/connectors/sqla/models.py:
##########
@@ -990,7 +990,9 @@ def adhoc_column_to_sqla( # pylint: disable=too-many-locals
time_grain = col.get("timeGrain")
has_timegrain = col.get("columnType") == "BASE_AXIS" and time_grain
is_dttm = False
+ pdf = None
if col_in_metadata := self.get_column(expression):
+ pdf = col_in_metadata.python_date_format
Review Comment:
Given that you're changing how the Python date format logic is used, would you mind adding a unit test for this.
##########
superset/db_engine_specs/pinot.py:
##########
@@ -99,12 +101,13 @@ def get_timestamp_expr(
else:
seconds_or_ms = "MILLISECONDS" if pdf == "epoch_ms" else "SECONDS"
tf = f"1:{seconds_or_ms}:EPOCH"
+
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)
Review Comment:
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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on PR #24942:
URL: https://github.com/apache/superset/pull/24942#issuecomment-1682449976
@ege-st please run following script in your local terminal, it should be similar as CI in GitHub, when all of passed, ping me.
```bash
make pre-commit
pre-commit run --all-files
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ege-st commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1295132219
##########
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,
Review Comment:
@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?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1293923299
##########
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,
Review Comment:
@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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ege-st commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1296277773
##########
superset/db_engine_specs/pinot.py:
##########
@@ -30,93 +29,34 @@ class PinotEngineSpec(BaseEngineSpec): # pylint: disable=abstract-method
allows_alias_in_select = False
allows_alias_in_orderby = False
- # Pinot does its own conversion below
+ # https://docs.pinot.apache.org/users/user-guide-query/supported-transformations#datetime-functions
_time_grain_expressions = {
- TimeGrain.SECOND: "1:SECONDS",
- TimeGrain.MINUTE: "1:MINUTES",
- TimeGrain.FIVE_MINUTES: "5:MINUTES",
- TimeGrain.TEN_MINUTES: "10:MINUTES",
- TimeGrain.FIFTEEN_MINUTES: "15:MINUTES",
- TimeGrain.THIRTY_MINUTES: "30:MINUTES",
- TimeGrain.HOUR: "1:HOURS",
- TimeGrain.DAY: "1:DAYS",
- TimeGrain.WEEK: "week",
- TimeGrain.MONTH: "month",
- TimeGrain.QUARTER: "quarter",
- TimeGrain.YEAR: "year",
- }
-
- _python_to_java_time_patterns: dict[str, str] = {
- "%Y": "yyyy",
- "%m": "MM",
- "%d": "dd",
- "%H": "HH",
- "%M": "mm",
- "%S": "ss",
- }
-
- _use_date_trunc_function: dict[str, bool] = {
- TimeGrain.SECOND: False,
- TimeGrain.MINUTE: False,
- TimeGrain.FIVE_MINUTES: False,
- TimeGrain.TEN_MINUTES: False,
- TimeGrain.FIFTEEN_MINUTES: False,
- TimeGrain.THIRTY_MINUTES: False,
- TimeGrain.HOUR: False,
- TimeGrain.DAY: False,
- TimeGrain.WEEK: True,
- TimeGrain.MONTH: True,
- TimeGrain.QUARTER: True,
- TimeGrain.YEAR: True,
+ None: "{col}",
+ "PT1S": "CAST(DATE_TRUNC('second', CAST({col} AS TIMESTAMP)) AS TIMESTAMP)",
Review Comment:
Done.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr` [superset]
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on PR #24942:
URL: https://github.com/apache/superset/pull/24942#issuecomment-1779518082
Hi @martin-raymond, I'll take a look today and let you know.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1290668187
##########
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,
Review Comment:
@ege-st
The design of current Pinot DB spec is completely incorrect. Maintaining our own Pinot driver and db_spec should solve your issue.
```python
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)
@staticmethod
def alias_in_select_mutator(label: str) -> str:
return f"{label}__"
@classmethod
def get_timezone_expressions(
cls,
col: ColumnClause,
timezone: typing.Optional[str],
) -> ColumnClause:
if BF_EUROPE_BERLIN_TIMEZONE_COLUMN_SUFFIX in col.name:
return col
tpl = f"""
CAST(
FROMDATETIME(
DATETIMECONVERT(
{col},
'1:MILLISECONDS:EPOCH',
'1:MILLISECONDS:SIMPLE_DATE_FORMAT:yyyy-MM-dd HH:mm:ss.SSS tz({timezone})',
'1:MILLISECONDS'
),
'yyyy-MM-dd HH:mm:ss.SSS'
) AS TIMESTAMP
)""".strip()
return literal_column(tpl)
```
driver at: https://github.com/BurdaForward/pinot-dbapi/tree/bf_release
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1290663501
##########
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,
Review Comment:
@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.
<img width="895" alt="image" src="https://github.com/apache/superset/assets/2016594/4b6547bb-796e-4dd7-a2d5-07fcaff8b0a5">
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1290712357
##########
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,
Review Comment:
The Pinot provided time grain functions so we just define `_time_grain_expressions` and apply `column_datatype_to_string` is enough.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ege-st commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1293501272
##########
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,
Review Comment:
@zhaoyongjie after some testing there's one problem that's preventing your suggestion from working with Pinot. When using the code above, it will generate a query that looks like this:
```
SELECT CAST(DATE_TRUNC('day', CAST(created_date_ts AS TIMESTAMP)) AS TIMESTAMP) as created_date_ts,
COUNT(*) AS "count"
FROM "default".test_data
GROUP BY CAST(DATE_TRUNC('day', CAST(created_date_ts AS TIMESTAMP)) AS TIMESTAMP)
ORDER BY COUNT(*) DESC
LIMIT 10000;
```
Unfortunately, if you alias the expression as the name of an already existing column, Pinot will not properly analyze the `GROUP BY` clause. The above will generate an error.
However, if I change the query to:
```
SELECT CAST(DATE_TRUNC('day', CAST(created_date_ts AS TIMESTAMP)) AS TIMESTAMP) as created_date_ts_2,
COUNT(*) AS "count"
FROM "default".test_data
GROUP BY CAST(DATE_TRUNC('day', CAST(created_date_ts AS TIMESTAMP)) AS TIMESTAMP)
ORDER BY COUNT(*) DESC
LIMIT 10000;
```
or if I use:
```
SELECT CAST(DATE_TRUNC('day', CAST(created_date_ts AS TIMESTAMP)) AS TIMESTAMP) as created_date_ts,
COUNT(*) AS "count"
FROM "default".test_data
GROUP BY created_date_ts
ORDER BY COUNT(*) DESC
LIMIT 10000;
```
It will work. Is there a way that I can change what alias Superset uses for the computed timestamp?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1293810653
##########
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,
Review Comment:
@ege-st It might be a potential bug in Apache Calcite that was the SQL parser in Pinot. It was not too hard to fix it in Pinot codebase, I remember this issue was existing some DB which is used Calcite as parser.
Another option is add changes in Superset codes.
```python
diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py
index 5c5f66853..1fcdff8e5 100644
--- a/superset/connectors/sqla/models.py
+++ b/superset/connectors/sqla/models.py
@@ -1081,7 +1081,7 @@ class SqlaTable(Model, BaseDatasource): # pylint: disable=too-many-public-metho
# add quotes to tables
if db_engine_spec.allows_alias_in_select:
label = db_engine_spec.make_label_compatible(label_expected)
- sqla_col = sqla_col.label(label)
+ sqla_col = sqla_col.label(db_engine_spec.alias_in_select_mutator(label))
sqla_col.key = label_expected
return sqla_col
diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py
index df0552e8a..1d8e91482 100644
--- a/superset/db_engine_specs/base.py
+++ b/superset/db_engine_specs/base.py
@@ -644,6 +644,10 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods
)
)
+ @staticmethod
+ def alias_in_select_mutator(label: str) -> str:
+ return label
+
@classmethod
def fetch_data(
cls, cursor: Any, limit: Optional[int] = None
diff --git a/superset/db_engine_specs/pinot.py b/superset/db_engine_specs/pinot.py
index d88fdf36f..d82bf8604 100644
--- a/superset/db_engine_specs/pinot.py
+++ b/superset/db_engine_specs/pinot.py
@@ -26,7 +26,7 @@ class PinotEngineSpec(BaseEngineSpec): # pylint: disable=abstract-method
engine_name = "Apache Pinot"
allows_subqueries = False
allows_joins = False
- allows_alias_in_select = False
+ allows_alias_in_select = True
allows_alias_in_orderby = False
# https://docs.pinot.apache.org/users/user-guide-query/supported-transformations#datetime-functions
@@ -56,3 +56,7 @@ class PinotEngineSpec(BaseEngineSpec): # pylint: disable=abstract-method
return sqla_column_type.compile().upper()
else:
return super().column_datatype_to_string(sqla_column_type, dialect)
+
+ @staticmethod
+ def alias_in_select_mutator(label: str) -> str:
+ return f"{label}__"
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1293810653
##########
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,
Review Comment:
@ege-st I might be a potential bug in Apache Calcite that was the SQL parser in Pinot. It was not too hard to fix it in Pinot codebase, I remember this issue was existing some DB which is used Calcite as parser.
Another option is add changes in Superset codes.
```python
diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py
index 5c5f66853..1fcdff8e5 100644
--- a/superset/connectors/sqla/models.py
+++ b/superset/connectors/sqla/models.py
@@ -1081,7 +1081,7 @@ class SqlaTable(Model, BaseDatasource): # pylint: disable=too-many-public-metho
# add quotes to tables
if db_engine_spec.allows_alias_in_select:
label = db_engine_spec.make_label_compatible(label_expected)
- sqla_col = sqla_col.label(label)
+ sqla_col = sqla_col.label(db_engine_spec.alias_in_select_mutator(label))
sqla_col.key = label_expected
return sqla_col
diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py
index df0552e8a..1d8e91482 100644
--- a/superset/db_engine_specs/base.py
+++ b/superset/db_engine_specs/base.py
@@ -644,6 +644,10 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods
)
)
+ @staticmethod
+ def alias_in_select_mutator(label: str) -> str:
+ return label
+
@classmethod
def fetch_data(
cls, cursor: Any, limit: Optional[int] = None
diff --git a/superset/db_engine_specs/pinot.py b/superset/db_engine_specs/pinot.py
index d88fdf36f..d82bf8604 100644
--- a/superset/db_engine_specs/pinot.py
+++ b/superset/db_engine_specs/pinot.py
@@ -26,7 +26,7 @@ class PinotEngineSpec(BaseEngineSpec): # pylint: disable=abstract-method
engine_name = "Apache Pinot"
allows_subqueries = False
allows_joins = False
- allows_alias_in_select = False
+ allows_alias_in_select = True
allows_alias_in_orderby = False
# https://docs.pinot.apache.org/users/user-guide-query/supported-transformations#datetime-functions
@@ -56,3 +56,7 @@ class PinotEngineSpec(BaseEngineSpec): # pylint: disable=abstract-method
return sqla_column_type.compile().upper()
else:
return super().column_datatype_to_string(sqla_column_type, dialect)
+
+ @staticmethod
+ def alias_in_select_mutator(label: str) -> str:
+ return f"{label}__"
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on code in PR #24942:
URL: https://github.com/apache/superset/pull/24942#discussion_r1295153284
##########
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,
Review Comment:
@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 rather than calculate in client.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #24942: fix(pinot): The `python_date_format` for a temporal column was not being passed to `get_timestamp_expr`
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on PR #24942:
URL: https://github.com/apache/superset/pull/24942#issuecomment-1694706447
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
> <https://github.com/apache/superset/pull/24942#issuecomment-1694696260>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAPMKURVVPL3EOQXF4GHMRTXXNRUVANCNFSM6AAAAAA3KYBQUU>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
Best regards,
Yongjie
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org