Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4.1.0rc1 Druid queries with CURRENT_TIMETAMP fails #29729

Closed
3 tasks done
iercan opened this issue Jul 28, 2024 · 9 comments
Closed
3 tasks done

4.1.0rc1 Druid queries with CURRENT_TIMETAMP fails #29729

iercan opened this issue Jul 28, 2024 · 9 comments
Labels
data:connect:druid Related to Druid validation:required A committer should validate the issue

Comments

@iercan
Copy link
Contributor

iercan commented Jul 28, 2024

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

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

Screenshots/recordings

How metric looks like
image

How superset sends query
image

Given error
image

Superset version

master / latest-dev

Python version

3.9

Node version

16

Browser

Chrome

Additional context

No response

Checklist

  • I have searched Superset docs and Slack and didn't find a solution to my problem.
  • I have searched the GitHub issue tracker and didn't find a similar bug report.
  • 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 dosubot bot added the data:connect:druid Related to Druid label Jul 28, 2024
Copy link

dosubot bot commented Jul 28, 2024

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 rusackas added the validation:required A committer should validate the issue label Jul 29, 2024
@rusackas
Copy link
Member

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

@betodealmeida
@sujiplr
@nytai
@villebro

@sadpandajoe
Copy link
Member

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
Copy link
Member

mistercrunch commented Aug 5, 2024

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
Copy link
Member

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
Copy link
Member

eschutho commented Aug 7, 2024

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

@mistercrunch
Copy link
Member

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

@iercan
Copy link
Contributor Author

iercan commented Aug 16, 2024

@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.

@iercan
Copy link
Contributor Author

iercan commented Oct 17, 2024

Error has gone on 4.1.0rc3

@iercan iercan closed this as completed Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:connect:druid Related to Druid validation:required A committer should validate the issue
Projects
None yet
Development

No branches or pull requests

5 participants