Skip to content

Conversation

@wardsi
Copy link
Contributor

@wardsi wardsi commented Jan 27, 2025

Check List

  • Tests have been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Issue Reference this PR resolves
In the dremio driver (query) the syntax for DATE_ADD and DATE_SUB are incorrect. The interval function requires a cast and the positional arguments need correcting.

@wardsi wardsi requested a review from a team as a code owner January 27, 2025 21:29
@github-actions github-actions bot added the pr:community Contribution from Cube.js community members. label Jan 27, 2025
@KSDaemon KSDaemon self-assigned this Jan 27, 2025

subtractInterval(date, interval) {
return `DATE_SUB(${date}, INTERVAL ${interval})`;
const intervalParts = interval.trim().split(' ');
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, intervals are a bit more complicated. You can get here interval values like:

  • 1 hour 2 minutes
  • 3 month
  • 3 months 24 days 15 minutes
  • 2 years 6 months
  • Etc....

Please, have a look at some examples of interval processing in different dialects that might give you an idea how to process it here:

Have a look at formatInterval() implementations in these query dialects.
Hope this helps!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Many thanks for the info. I have sent a further change. It only supports year,qtr,month and day for now. I've taken examples from the other drivers as per suggestions above.

…cube into dremio-date-interval-fix

Sync branch before push to origin. Need to correct lint failures
/**
* The input interval with (possible) plural units, like "1 hour 2 minutes", "2 year", "3 months", "4 weeks", "5 days", "3 months 24 days 15 minutes", ...
* will be converted to Dremio dialect.
* @see https://docs.dremio.com/24.3.x/reference/sql/sql-functions/functions/DATE_ADD/
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the docs, I see that Dremio supports more intervals for timestamps.

DATE_ADD(timestamp_expression string, time_interval interval) → timestamp
timestamp_expression: A string-formatted timestamp in the format 'YYYY-MM-DD HH24:MI:SS'.
time_interval: A CAST of a number to one of these intervals: SECOND, MINUTE, HOUR, DAY, MONTH, YEAR.

So i think they should be supported here too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, we also need to support a week method as well in the same way the quarter is calculated.

Copy link
Member

@KSDaemon KSDaemon left a comment

Choose a reason for hiding this comment

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

Please add support for SECOND, MINUTE, HOUR

@KSDaemon
Copy link
Member

There are these functions defined in base class that you can override:

  /**
   * @param {string} timestamp
   * @param {string} interval
   * @returns {string}
   */
  addTimestampInterval(timestamp, interval) {
    return this.addInterval(timestamp, interval);
  }

  /**
   * @param {string} timestamp
   * @param {string} interval
   * @returns {string}
   */
  subtractTimestampInterval(timestamp, interval) {
    return this.subtractInterval(timestamp, interval);
  }

@KSDaemon
Copy link
Member

Another thing I missed: you should also provide the template for INTERVAL that might be used by SQL API.
Have a look at sqlTemplates() function in other Query classes as example (PostgresQuery, BigqueryQuery, etc):

    templates.expressions.interval = 'INTERVAL \'{{ interval }}\'';

@KSDaemon KSDaemon changed the title Correct dremio date interval functions fix(dremio-driver): Correct dremio date interval functions Feb 3, 2025
@KSDaemon
Copy link
Member

KSDaemon commented Feb 3, 2025

@wardsi Thnx for the updates! I think this looks good now. Have you tested your changes at least locally?

@wardsi
Copy link
Contributor Author

wardsi commented Feb 3, 2025

Hi - yes. I am using the entire change on a uat env. Testing against superset and metabase integrations with cubejs and synmetrix

@KSDaemon
Copy link
Member

KSDaemon commented Feb 6, 2025

Closing because it is superseded by #9193

@KSDaemon KSDaemon closed this Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:community Contribution from Cube.js community members.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants