-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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 risingwave.py in db engine specs #22123
Conversation
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.
Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️
We hope to see you in our Slack community too!
Codecov Report
@@ Coverage Diff @@
## master #22123 +/- ##
==========================================
+ Coverage 66.74% 66.96% +0.22%
==========================================
Files 1841 1834 -7
Lines 70220 69896 -324
Branches 7672 7570 -102
==========================================
- Hits 46865 46805 -60
+ Misses 21371 21133 -238
+ Partials 1984 1958 -26
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Hi @BowenXiao1999 , thanks for the contribution and always happy to see more databases added to Superset! A few comments:
|
Thanks! I will provide more context based on your suggestion. |
The reason that these entry is not merged is expected I think, cuz the raw records is chosen and no group by or agg is done on. After switch to aggregation, they are correctly differetiate. BTW, we have a demo link: https://risingwave-labs.notion.site/Tutorial-Building-streaming-dashboards-with-Superset-ae1c667aa6734bffa38f0d260734a470, but maybe we should not put it in doc seems it evolve fast. @villebro PTAL again and feel free to give comments, thanks! |
Ah, makes sense 👍 @BowenXiao1999 are you able to add a note to the docs as requested in my previous review? We want to make sure others will also know that RisingWave is also supported! |
Sorry my bad, forgot to push. Already added, pls check. |
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
SUMMARY
Fixes #22122
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Can see the time grain option now.
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION