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

feat: Add timestamp arithmetic functionality #6901

Merged
merged 9 commits into from
Mar 4, 2021

Conversation

jzaralim
Copy link
Contributor

@jzaralim jzaralim commented Jan 26, 2021

Description

This PR adds the functions timestampadd and timestampsub. Example usages are:

TIMESTAMPADD(MINUTES, 40, time)
TIMESTAMPSUB(HOURS, 10, time)

In order to do this, a new "type" was introduced, INTERVAL, that represents an interval of time. Users define them using an IntervalExpression, which consists of numeric expression and a WindowUnit. INTERVAL can only be used within a function and cannot be used as an actual data type (similar to intervals in MySQL).

Testing done

Unit/QTT tests for each of the functions

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@jzaralim jzaralim requested review from JimGalasyn and a team as code owners January 26, 2021 20:23
Copy link
Member

@JimGalasyn JimGalasyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with a few suggestions.

@blueedgenick
Copy link
Contributor

Hey @jzaralim - apologies if this was already debated, perhaps in the klip, and I simply missed it - I'm thinking that it might be easier for our users, and more in line with other common systems, to have a datediff function rather than specific add/subtract-to-timestamp functions ? In combination with the neat new interval building block you have here it can be composed into many interesting permutations. It's pretty common to use this kind of function (and supply a negative interval value to go back in time, positive to go forwards) and of course the naming suggests it may one day work for any kind of date/time data (we can be flexible or not on that as the capabilities get richer).

@spena
Copy link
Member

spena commented Jan 27, 2021

@blueedgenick Doesn't datediff give a different result than timestampadd for instance? The idea of the timestampadd is to add units to a timestamp and return a timestamp. But datediff returns an integer instead. That seems different, doesn't it?

@jzaralim
Copy link
Contributor Author

@blueedgenick If you're thinking of a function that generically adds/subtracts an interval of time from a timestamp, then currently there isn't anything stopping an interval from having a negative value, so timestampadd(date, -1 day) would actually return the same thing as timestampsub(date, 1 day). I haven't seen any MySQL/Snowflake etc function names that imply this though.

@jzaralim jzaralim changed the title feat: add timestamp arithmetic functionality feat: Add timestamp arithmetic functionality Feb 18, 2021
Copy link
Member

@spena spena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks @jzaralim. I left some comments. Btw, I'm not sure if using interval as another type. Is there another way to avoid that? Can we specify the right syntax in SqlBase.g4 instead?

Btw, I looked at other DBs, and they use the INTERVAL keyword in their interval parameters. For instance, date_add(time, INTERVAL '5 hours 5 minutes'). Snowflake, Postgres and Mysql support it that way. Do you think we should do it too?

docs/developer-guide/ksqldb-reference/scalar-functions.md Outdated Show resolved Hide resolved
docs/developer-guide/ksqldb-reference/scalar-functions.md Outdated Show resolved Hide resolved
docs/developer-guide/ksqldb-reference/scalar-functions.md Outdated Show resolved Hide resolved
docs/developer-guide/ksqldb-reference/scalar-functions.md Outdated Show resolved Hide resolved
);

// Then:
assertThat(e.getMessage(), containsString("no viable alternative at input 'TIMESTAMPADD(5 HOURS"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to change this error message? no viable alternative at ... does not look helpful for users. If the problem is generic for other functions, then we can ignore it for now.

);

// Then:
assertThat(e.getMessage(), containsString("mismatched input '5' expecting {"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any way to change the error message? Users may get confused about which 5 parameter is.


@Override
public String toString() {
return "INTERVAL";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this INTERVAL string printed somewhere? I wonder if you were adding an INTERVAL keyword, but I didn't not see it.

@jzaralim
Copy link
Contributor Author

Cool, thanks @jzaralim. I left some comments. Btw, I'm not sure if using interval as another type. Is there another way to avoid that? Can we specify the right syntax in SqlBase.g4 instead?

Btw, I looked at other DBs, and they use the INTERVAL keyword in their interval parameters. For instance, date_add(time, INTERVAL '5 hours 5 minutes'). Snowflake, Postgres and Mysql support it that way. Do you think we should do it too?

The main reason interval is its own type is because we need the UDFs to be able to differentiate between regular integers and intervals. SqlLambda exists now though, so it makes sense to refactor intervals to follow the same pattern.

I'm not too sure what purpose the INTERVAL keyword would have, but since it is a well known syntax, let's do that too. It should be a simple change anyway.

@jzaralim
Copy link
Contributor Author

jzaralim commented Mar 2, 2021

@spena I've changed the grammar to use a separate time unit instead of the interval type from before. I will be rebasing this after #7103 merges though, since it touches a lot of the same code.

Copy link
Member

@spena spena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @jzaralim

@jzaralim jzaralim merged commit e2c06dc into confluentinc:master Mar 4, 2021
@jzaralim jzaralim deleted the math branch March 4, 2021 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants