Skip to content

Conversation

@paco-valdez
Copy link
Member

@paco-valdez paco-valdez commented Oct 28, 2025

Check List

  • Tests have been run in packages where changes have been 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

Changes Made:

  1. Added test (packages/cubejs-client-core/test/drill-down.test.ts:405-520):
    - Created a test case that replicates scenario with a 5-minute custom granularity
    - The test verifies that drilling down returns the correct interval range
  2. Fixed the bug (packages/cubejs-client-core/src/ResultSet.ts):
    - Modified the snapTo() method to detect custom granularities using isPredefinedGranularity()
    - For custom granularities:
    • Retrieves the granularity metadata (interval, origin, offset) from annotations
    • Parses the interval string (e.g., "5 minutes")
    • Calculates the correct end time by adding the interval to the start time
    • Returns the proper date range: [intervalStart, intervalStart + interval - 1ms]
      - Predefined granularities continue to use the existing snapTo() logic

@paco-valdez paco-valdez requested a review from a team as a code owner October 28, 2025 17:25
@github-actions github-actions bot added client:core Issues relating to the JavaScript client SDK javascript Pull requests that update Javascript code labels Oct 28, 2025
@ovr ovr requested a review from vasilev-alex October 28, 2025 18:14

if (granularity !== undefined) {
const range = dayRange(value, value).snapTo(granularity);
let range: { start: dayjs.Dayjs; end: dayjs.Dayjs };
Copy link
Member

Choose a reason for hiding this comment

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

I assume that we need to tweak the dayRange function and pass timeDimensionAnnotation, because it's used in time-series, and without that, custom time dimension logic will not work.

Copy link
Member

Choose a reason for hiding this comment

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

friendly ping to @vasilev-alex for advices here :)

@paco-valdez paco-valdez requested a review from ovr October 28, 2025 19:30
@paco-valdez
Copy link
Member Author

@ovr You are absolutely right! 😉 , I moved the logic to snapTo func, and also Im using now the annotations.

@paco-valdez paco-valdez changed the title Fix drilldown() for custom granularities feat(cubejs-client-core): add support for custom granularities to drilldown() Oct 31, 2025
@paco-valdez paco-valdez changed the title feat(cubejs-client-core): add support for custom granularities to drilldown() feat(cubejs-client-core): add custom granularities support to drilldown() Oct 31, 2025
// If custom granularity has an origin, align to it
if (customGranularity.origin) {
let origin = internalDayjs(customGranularity.origin);
if (customGranularity.offset) {
Copy link
Member

Choose a reason for hiding this comment

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

offset and origin are mutually exclusive


while (start.startOf(value).isBefore(end) || start.isSame(end)) {
results.push(start);
start = start.add(1, value);
Copy link
Member

Choose a reason for hiding this comment

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

How would this work for custom intervals like 3 days or 2 days 3 hours 4 minutes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client:core Issues relating to the JavaScript client SDK javascript Pull requests that update Javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants