-
Notifications
You must be signed in to change notification settings - Fork 25
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
Move Reflections to Rest API Calls and Extend Implementation #256
Conversation
2a02b00
to
e6ece19
Compare
e6ece19
to
133108c
Compare
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.
Great PR!
I have some not huge blocking changes requests.
The main part is documentation. We should update changelog, we should create a ticket for docs team for sure about this feature in our dbt connector.
I remember that we were discussing the waiting part of reflection to be created. Will we provide it in the next PR? Should we create a ticket for it?
dbt/include/dremio/macros/materializations/reflection/create_reflection.sql
Show resolved
Hide resolved
@bcmeireles we should transfer the PR description somewhere in the docs and changelog as well! |
I'll be copying the PR description into the Changelog. Once merged I will also be updating this repo's wiki with the new functionalities and changes. |
dbt/include/dremio/macros/materializations/reflection/create_reflection.sql
Outdated
Show resolved
Hide resolved
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.
Could you please add somewhere links about REST API of reflections in Dremio?
The rest is great, thank you!
A link to the Reflections API docs has also been added in the entity |
0c8a09b
to
40d40b2
Compare
Summary
Reflections were being created through the SQL Runner and that was the cause for some issues we were facing.
Description
The current reflections implementation requires sys.reflections access, which is only available to superusers (admins). When creating the reflection with a regular user, it will error. To fix this issue, the creation of the reflections has been moved from queries being ran inside the SQL Runner to Dremio's Rest API calls.
The current logic of writing reflections through a custom materialization remains to make sure reflections created under the previous approach still work.
On top of this, the behaviour where all reflections would be dropped and then recreated when running a dbt project has been changed to instead first check if a reflection with the same name is already present in the same dataset and, if that's the case, it will update the already existing reflection instead.
Test Results
A total of 11 tests have been added, approaching different situations that may occur when dealing with reflections
Changelog
date_dimensions
parameter was added to the reflection materialization, to set fields that have aDATE
granularitydistribute_by
partition_transform
SUM, COUNT
if mapped measure is numeric,COUNT
if notreflections_enabled
adapter option has been renamed toreflections_metadata_enabled
(requires user privileges to run in dremio)Samples
source as it is needed for the reflection tests