-
Notifications
You must be signed in to change notification settings - Fork 28
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: use well-defined dimensions instead of strings #284
Conversation
Signed-off-by: Shrivardhan Rao <Shrivardhan_Rao@intuit.com>
Codecov Report
@@ Coverage Diff @@
## main #284 +/- ##
==========================================
- Coverage 93.29% 93.26% -0.03%
==========================================
Files 66 66
Lines 2909 2911 +2
Branches 233 234 +1
==========================================
+ Hits 2714 2715 +1
Misses 156 156
- Partials 39 40 +1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT. Thank you for fixing this @cosmic-chichu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comment
use generator Co-authored-by: Avik Basu <3485425+ab93@users.noreply.github.com> Signed-off-by: shrivardhan <shrivardhan92@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There is no unit test for this particular feature. Is this way of setting dimensions manually tested?
Using dimensions as a list of strings causes ambiguity in other druid clients. Well-defined dimensions makes use of the utility in pydruid to create dimensions as DimensionSpec. This helps with deterministic serialization and de-serialization of the query.