#views users for created dashboards on profile page #1667
Conversation
|
should we add views and users to slices as well? if not i think we should align the |
|
not too familiar with sqlalchemy indexes but otherwise LGTM. @mistercrunch @bkyryliuk any comments on the indexing code here? |
There was a problem hiding this comment.
I'm a bit worried about how this can introduce lag in loading simple dashbaord or slice lists. How does it perform on production with millions of entries in the logs table?
If we're going to refer to views per dashboard commonly I think we should maintain a counter in the dashboard. That would mean a database migration script to add the field and prepolutate it, and and incrementing the view counter on the dashboard endpoint.
| ) | ||
| .join(Log) |
There was a problem hiding this comment.
outerjoin
also note that the query may perform better as a join to a subquery on the Log table that aggregates by dashboard id
| .order_by( | ||
| Dash.changed_on.desc() | ||
| ) | ||
| .group_by(Dash.id) |
There was a problem hiding this comment.
I'm not sure but it seems like this should be .group_by(Dash) or individual fields should be specified in the select and group_by clauses
| json = Column(Text) | ||
| user = relationship('User', backref='logs', foreign_keys=[user_id]) | ||
| dttm = Column(DateTime, default=datetime.utcnow) | ||
| dt = Column(Date, default=date.today()) | ||
| views = Index(dashboard_id, user_id, action, dttm) |
There was a problem hiding this comment.
I think the common way is more something like this:
__table_args__ = (
Index(foo),
Index(bar),
)
|
|
||
|
|
||
| def upgrade(): | ||
| op.create_index( |
There was a problem hiding this comment.
just curious, was this auto-generated or did you copy/paste it from some place?
There was a problem hiding this comment.
this is auto-generated and modified
|
I reverted the original query appraoch, added the views column to dashboards and slices, pre populated them with Log data in the migration file. |
vera-liu
force-pushed
the
vliu_dashboard_view
branch
from
54361dc to
a4aa9fe
Compare
vera-liu
changed the title
|
LGMT |
vera-liu
force-pushed
the
vliu_dashboard_view
branch
2 times, most recently
from
868c2d8 to
f7b8271
Compare
them with Log data
fix multiple heads
vera-liu
force-pushed
the
vliu_dashboard_view
branch
from
3f8a115 to
795dc5b
Compare
* Add #views and #distinct users to created dashboard on profile page * Added index on logs to speed up query * Added #views and #users for slice table * Add a views column to dashboards and slices, prepopulate them with Log data * Remove index on Log model * Remove unused index * Update 1b2c3f7c96f9_.py fix multiple heads * Exclude postgres in prepopulating views column
mistercrunch
added
🏷️ bot
I'm not sure if this is the standard way to add index. Any suggestions are appreciated.
Done:
Note:
@mistercrunch @bkyryliuk