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

fix(api-gateway): Allow querying time dimensions with custom granularity #9068

Merged

Conversation

LFischerstrom
Copy link
Contributor

@LFischerstrom LFischerstrom commented Jan 6, 2025

Check List

  • Tests has 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

It's possible to query time dimensions with granularity as a normal dimension but queries with quarter were rejected.

@github-actions github-actions bot added the pr:community Contribution from Cube.js community members. label Jan 6, 2025
@LFischerstrom LFischerstrom marked this pull request as ready for review January 6, 2025 00:08
@LFischerstrom LFischerstrom requested a review from a team as a code owner January 6, 2025 00:08
@KSDaemon KSDaemon changed the title fix: allow dimensions with quarter granularity fix(api-gateway): Allow querying dimensions with quarter granularity Jan 8, 2025
@igorlukanin
Copy link
Member

Thanks @LFischerstrom 🙌

Honestly, I didn't even know this is possible to query dimensions like this:
Screenshot 2025-01-08 at 10 21 50

I feel like we'd at least need a test for this.

@LFischerstrom
Copy link
Contributor Author

@igorlukanin Ok, it's documented here https://cube.dev/docs/product/apis-integrations/rest-api/query-format

It also claims to support custom granularities which I think would rejected by the input validation.

@igorlukanin
Copy link
Member

Yeah, you're right:

In the case of a dimension of the time type, a granularity could be optionally added to the name, in the following format: cube_name.time_dimension_name.granularity_name, e.g., stories.time.week. It can be one of the default granularities (e.g., year or week) or a custom granularity.

And I think you're also right wrt custom granularities. I wonder what is the best fix to that. A wildcard match? I bet @KSDaemon definitely has an opinion on this.

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.

@LFischerstrom Let's update it like this. So it will support all granularities :)

It would be awesome if you can add a test for querying time dimension with some granulaties. It should not be hard. Have a look at https://github.com/cube-js/cube/blob/master/packages/cubejs-api-gateway/test/index.test.ts#L306-L339 as example. We just need to check that query is allowed by api qateway.

@LFischerstrom
Copy link
Contributor Author

Thanks @KSDaemon, will look into adding a tests

@LFischerstrom
Copy link
Contributor Author

@KSDaemon I'm trying to run the tests locally but it's not working. Are there any instructions that I should follow for setting up the repo to be able to run the tests?

@KSDaemon
Copy link
Member

@LFischerstrom Hi! Hm.... Nothing special is required. What tests are you trying to run? What package?
Btw, I see lint fails, you can fix lint errors with yarn run lint:fix in root.

@KSDaemon
Copy link
Member

KSDaemon commented Jan 15, 2025

@LFischerstrom Try to follow the commands described in https://github.com/cube-js/cube/blob/master/CONTRIBUTING.md#cube-server

Probably not all required, but at least:

  • yarn install
  • yarn build
  • yarn tsc
    To build/transpile all the code

@LFischerstrom
Copy link
Contributor Author

Thanks, I tried to running yarn build instead of yarn tsc. After using tsc it worked.

@LFischerstrom
Copy link
Contributor Author

@KSDaemon Alright, the tests pass for me so if you think the new test looks good I think this is ready.

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.

👍🏻 I think this looks good! @LFischerstrom Thank you for adding the tests!

@KSDaemon KSDaemon changed the title fix(api-gateway): Allow querying dimensions with quarter granularity fix(api-gateway): Allow querying time dimensions with custom granularity Jan 15, 2025
@KSDaemon KSDaemon merged commit 80f10ce into cube-js:master Jan 15, 2025
24 checks passed
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.

3 participants