Skip to content
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

Merged
merged 7 commits into from
Jan 7, 2025

Conversation

bcmeireles
Copy link
Contributor

@bcmeireles bcmeireles commented Dec 19, 2024

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

  • Reflections are now handled through the Rest API, allowing non-admin users to also create them
  • It is now possible to set a custom name for reflections
  • If a reflection already exists in the dataset with the same name defined in the model, it will be updated instead of creating a new one
  • New date_dimensions parameter was added to the reflection materialization, to set fields that have a DATE granularity
  • Added distribution fields under distribute_by
  • Added partition transformations under partition_transform
    • Defaults to Original/Identity if not defined
    • year/month/day/hour/bucket(n), truncate(n)
  • Computations default to SUM, COUNT if mapped measure is numeric, COUNT if not
  • reflections_enabled adapter option has been renamed to reflections_metadata_enabled (requires user privileges to run in dremio)
  • CI now creates and formats a Samples source as it is needed for the reflection tests

@bcmeireles bcmeireles marked this pull request as ready for review January 3, 2025 12:10
@bcmeireles bcmeireles requested review from howareyouman and simonpannek and removed request for howareyouman January 3, 2025 12:11
Copy link
Contributor

@howareyouman howareyouman left a 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?

.github/scripts/create_and_format_sources.sh Show resolved Hide resolved
dbt/adapters/dremio/connections.py Outdated Show resolved Hide resolved
dbt/adapters/dremio/connections.py Outdated Show resolved Hide resolved
dbt/adapters/dremio/connections.py Outdated Show resolved Hide resolved
dbt/adapters/dremio/connections.py Show resolved Hide resolved
@howareyouman
Copy link
Contributor

@bcmeireles we should transfer the PR description somewhere in the docs and changelog as well!

@bcmeireles
Copy link
Contributor Author

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?

@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.
Regarding the waiting for a reflection, that has been requested in #184 and is not addressed in this PR.

@bcmeireles bcmeireles linked an issue Jan 3, 2025 that may be closed by this pull request
@bcmeireles bcmeireles changed the title Move reflections to rest API calls Move Reflections to Rest API Calls and Extend Implementation Jan 7, 2025
howareyouman
howareyouman previously approved these changes Jan 7, 2025
Copy link
Contributor

@howareyouman howareyouman left a 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!

dbt/adapters/dremio/connections.py Outdated Show resolved Hide resolved
@bcmeireles
Copy link
Contributor Author

A link to the Reflections API docs has also been added in the entity

simonpannek
simonpannek previously approved these changes Jan 7, 2025
@bcmeireles bcmeireles requested a review from simonpannek January 7, 2025 13:25
@bcmeireles bcmeireles merged commit 8ad45ec into dremio:main Jan 7, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Reflections: dimensions_by_day not parsed correctly
3 participants