DataJunction / dj

A metrics platform.
http://datajunction.io
MIT License
35 stars 15 forks source link

Add register_view as another way to create Source nodes. #1153

Closed agorajek closed 2 months ago

agorajek commented 2 months ago

Summary

This adds the ability for DJ to create Source nodes based on database views, but with the extra option of creating the view first. This does not mean that DJ will maintain that view. Only that we use the query param verbatim to build a view and if it is successful we create a source node on top of it. DJ will also store the view SQL but will not do anything with it for now. The feature of allowing this query SQL to be updated could be a future feature.

Test Plan

Unit tests.

Deployment Plan

n/a

netlify[bot] commented 2 months ago

Deploy Preview for thriving-cassata-78ae72 canceled.

Name Link
Latest commit 3d89ba9325ff5a2b839a0c438c6b46506eea7eeb
Latest deploy log https://app.netlify.com/sites/thriving-cassata-78ae72/deploys/66e0b6f7ee7c7a00082be158
samredai commented 2 months ago

@agorajek, this is AWESOME! I have thoughts on the location of the view registering logic though. Wouldn't it be better if that logic is all encapsulated within the query service?

The two main reasons I'm advocating for this is 1) It's a single call to the query service instead of two calls and 2) if someone wants to customize how exactly they register views at their company, they don't need to add code to the OSS project and can instead just customize the query service.

agorajek commented 2 months ago

@samredai your suggestion is good. I'll try to update the code.

shangyian commented 2 months ago

Yeah, +1 to moving he "create view" part of this to the query service. It'll make it easier to adjust for any database engines that don't create views in the standard way.

agorajek commented 2 months ago

@shangyian . I am going to move the view creation to the query service in a separate PR because @samredai is currently refactoring the query service.

agorajek commented 2 months ago

@samredai and @shangyian this PR is ready if we want to hold on with moving the query generation to query service. @samredai let me know if you care either way.

samredai commented 2 months ago

@agorajek I just opened the DJQS refactor PR #1168 but I'll leave it up to you if you'd rather merge this first than migrate the register view logic over to the refactored query service.

agorajek commented 2 months ago

I'll merge this and move the CREATE VIEW sql to djqs later.