4.1.0rc1 Druid queries with CURRENT_TIMETAMP fails

iercan commented 1 month ago

Bug description

I'm testing 4.1.0rc1 and encountering an error with Druid datasets that use the CURRENT_TIMESTAMP function. It seems to be adding parentheses at the end, which is causing the failure, even though we don't define any in the metric.

How to reproduce the bug

Create a metric that use CURRENT_TIMESTAMP on druid dataset
Use metric in a chart

Screenshots/recordings

How metric looks like

How superset sends query

Given error

Superset version

master / latest-dev

Python version

3.9

Node version

16

Browser

Chrome

Additional context

No response

Checklist

[X] I have searched Superset docs and Slack and didn't find a solution to my problem.
[X] I have searched the GitHub issue tracker and didn't find a similar bug report.
[X] I have checked Superset's logs for errors and if I found a relevant Python stacktrace, I included it here as text in the "additional context" section.

dosubot[bot] commented 1 month ago

To resolve the issue with the CURRENT_TIMESTAMP function adding parentheses in Druid datasets on Superset version 4.1.0rc1, you need to modify the convert_dttm method in the DruidEngineSpec class to handle the CURRENT_TIMESTAMP function correctly.

Here is the relevant part of the code that needs to be adjusted:

@classmethod
def convert_dttm(
    cls, target_type: str, dttm: datetime, db_extra: dict[str, Any] | None = None
) -> str | None:
    sqla_type = cls.get_sqla_column_type(target_type)

    if isinstance(sqla_type, types.Date):
        return f"CAST(TIME_PARSE('{dttm.date().isoformat()}') AS DATE)"
    if isinstance(sqla_type, (types.DateTime, types.TIMESTAMP)):
        return f"""TIME_PARSE('{dttm.isoformat(timespec="seconds")}')"""
    if target_type.upper() == "CURRENT_TIMESTAMP":
        return "CURRENT_TIMESTAMP"  # Ensure no parentheses are added
    return None

By adding the condition to check if the target_type is CURRENT_TIMESTAMP and returning it directly without parentheses, you can avoid the issue causing the query to fail ^[1].

To continue talking to Dosu, mention @dosu.

rusackas commented 1 month ago

Pinging some folks from the rolodex that might be able to help repro/test/fix.

@betodealmeida @sujiplr @nytai @villebro

sadpandajoe commented 1 month ago

I don't see anything that has changed in the DruidEngineSpec files, but I did see we did bump pydruid. @betodealmeida @eschutho do you think that could have affect this?

mistercrunch commented 1 month ago

I tried to recreate using sqlparse.format but couldn't, got GPT to write this, but wasn't conclusive

import sqlparse

sql = "SELECT case when __time > TIMESTAMPADD(DAY, -7, CURRENT_DATE) AND __time < TIMESTAMPADD(DAY, -7, CURRENT_DATE) FROM t;"
formatted_sql = sqlparse.format(sql, reindent=True, keyword_case='upper', strip_comments=True)

print(formatted_sql)

it doesn't add parenthesis...

mistercrunch commented 1 month ago

Also reviewed https://github.com/apache/superset/blob/master/superset/db_engine_specs/druid.py for SQL mutation logic that would be Druid specific and couldn't find anything...

eschutho commented 1 month ago

@iercan can you add the version of your druid db and the driver?

mistercrunch commented 1 month ago

@betodealmeida might have some intuition as to what might be adding those () after CURRENT_TIMESTAMP (?)

iercan commented 3 weeks ago

@iercan can you add the version of your druid db and the driver?

Apologies for the late reply. Druid version is 30.0.0 and pydruid is 0.6.9.

apache / superset