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

Improve interval expression #4168

Closed
killme2008 opened this issue Jun 18, 2024 · 7 comments
Closed

Improve interval expression #4168

killme2008 opened this issue Jun 18, 2024 · 7 comments
Assignees
Labels
C-enhancement Category Enhancements good first issue Good for newcomers help wanted Extra attention is needed

Comments

@killme2008
Copy link
Contributor

killme2008 commented Jun 18, 2024

What type of enhancement is this?

API improvement, User experience

What does the enhancement do?

GreptimeDB supports sql-compatible intervals like:

 select interval '1 hour';

That is great. But in many time-series query scenarios, the 1 hour is shortened to 1h. It is great to support it too:

mysql> select interval '1h';
ERROR 1815 (HY000): Failed to plan SQL: Not yet implemented: Unsupported Interval Expression with value "1h seconds"

Other units such as:

  • y for years
  • m for minutes
  • s for seconds
  • w for weeks
  • d for days

etc.

Implementation challenges

No response

@killme2008 killme2008 added C-enhancement Category Enhancements good first issue Good for newcomers help wanted Extra attention is needed labels Jun 18, 2024
@etolbakov
Copy link
Collaborator

Hi Dennis @killme2008
Do I get it right that the exception comes from the DataFusion?

If that's the case is there any established way of "intercepting" the AST before sending it to plan?
Thank you!

@killme2008
Copy link
Contributor Author

Hi Dennis @killme2008 Do I get it right that the exception comes from the DataFusion?

If that's the case is there any established way of "intercepting" the AST before sending it to plan? Thank you!

I think we can transform the Expr just like type alias:

https://github.com/GreptimeTeam/greptimedb/blob/main/src/sql/src/statements/transform/type_alias.rs

Adds a new transform rule and add it to

static ref RULES: Vec<Arc<dyn TransformRule>> = vec![

@killme2008
Copy link
Contributor Author

@etolbakov Looks like compound signed number case not processed properly:

mysql> SELECT INTERVAL '-1h5m';
+----------------------------------------------+
| IntervalMonthDayNano("18446740773709551616") |
+----------------------------------------------+
| P0Y0M0DT0H-55M0S                             |
+----------------------------------------------+

mysql> SELECT INTERVAL '-1h-5m';
ERROR 1815 (HY000): Failed to plan SQL: Parser error: Invalid input syntax for type interval: "-1 h- 5 minutes"

Would you like to fix it?

@killme2008 killme2008 reopened this Jun 24, 2024
@etolbakov
Copy link
Collaborator

ohhh....my bad, yeah, let me give it a go

@killme2008
Copy link
Contributor Author

I have crafted a document detailing the interval type. Would you kindly provide feedback upon review?

GreptimeTeam/docs#1021

@killme2008
Copy link
Contributor Author

killme2008 commented Jun 25, 2024

@etolbakov I have an idea. Maybe we can apply this transform rule to the cast(string as interval) expression too? Then it can be applied to the interval in the form of '3y2mon'::interval. What do you think about it?

https://docs.rs/datafusion/latest/datafusion/logical_expr/enum.Expr.html#variant.Cast

https://docs.rs/datafusion/latest/datafusion/logical_expr/struct.TryCast.html

@etolbakov
Copy link
Collaborator

It's a great idea! Let me make an assessment (and reopen the ticket 😁)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category Enhancements good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants