feat: add new granularities to gooddata-dbt and gooddata-pipelines - #1714
Open
okrcmaGD wants to merge 1 commit into
Open
feat: add new granularities to gooddata-dbt and gooddata-pipelines#1714okrcmaGD wants to merge 1 commit into
okrcmaGD wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis change adds optional second-based date granularities to LDM extension and dbt workflows. The option is disabled by default and requires the organization feature flag. Documentation and tests cover both modes. ChangesSecond-based date granularities
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LdmExtensionManager
participant LdmExtensionDataProcessor
participant DateDatasetBuilder
LdmExtensionManager->>LdmExtensionDataProcessor: Pass enable_second_granularities
LdmExtensionDataProcessor->>DateDatasetBuilder: Use configured date granularities
DateDatasetBuilder-->>LdmExtensionDataProcessor: Build date datasets
sequenceDiagram
participant deploy_ldm
participant DbtModelTables
participant TimestampDateDataset
deploy_ldm->>DbtModelTables: Pass CLI option
DbtModelTables->>TimestampDateDataset: Add second timestamp granularities
TimestampDateDataset-->>DbtModelTables: Return generated dataset
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/gooddata-pipelines/src/gooddata_pipelines/ldm_extension/input_processor.py`:
- Line 64: Restore the public LdmExtensionDataProcessor.DATE_GRANULARITIES class
attribute as a deprecated alias referencing _DATE_GRANULARITIES, preserving
existing caller access without duplicating the granularity values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e731f3c6-6548-4baf-bbfc-5d9c6522845b
📒 Files selected for processing (10)
docs/content/en/latest/pipelines/ldm_extension/_index.mdpackages/gooddata-dbt/README.mdpackages/gooddata-dbt/src/gooddata_dbt/args.pypackages/gooddata-dbt/src/gooddata_dbt/dbt/base.pypackages/gooddata-dbt/src/gooddata_dbt/dbt/tables.pypackages/gooddata-dbt/src/gooddata_dbt/dbt_plugin.pypackages/gooddata-dbt/tests/test_tables.pypackages/gooddata-pipelines/src/gooddata_pipelines/ldm_extension/input_processor.pypackages/gooddata-pipelines/src/gooddata_pipelines/ldm_extension/ldm_extension_manager.pypackages/gooddata-pipelines/tests/test_ldm_extension/test_input_processor.py
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1714 +/- ##
==========================================
+ Coverage 78.30% 79.00% +0.70%
==========================================
Files 271 271
Lines 18689 18779 +90
==========================================
+ Hits 14634 14837 +203
+ Misses 4055 3942 -113 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/gooddata-pipelines/src/gooddata_pipelines/ldm_extension/input_processor.py (1)
85-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid sharing mutable granularity lists.
CatalogDeclarativeDateDatasetretains the supplied list, and_date_from_fieldpasses the same list to every dataset. Copy_date_granularitiesin__init__and when constructing each dataset. AddClassVarannotations if Ruff RUF012 is enabled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/gooddata-pipelines/src/gooddata_pipelines/ldm_extension/input_processor.py` around lines 85 - 99, Prevent shared mutable granularity lists by copying _date_granularities in CatalogDeclarativeDateDataset.__init__ and copying it again when _date_from_field constructs each dataset. Annotate class-level mutable granularity constants with ClassVar if required by Ruff RUF012.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/gooddata-pipelines/src/gooddata_pipelines/ldm_extension/input_processor.py`:
- Around line 85-99: Prevent shared mutable granularity lists by copying
_date_granularities in CatalogDeclarativeDateDataset.__init__ and copying it
again when _date_from_field constructs each dataset. Annotate class-level
mutable granularity constants with ClassVar if required by Ruff RUF012.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54f40155-b3eb-4148-bdd9-7d4e0953a987
📒 Files selected for processing (10)
docs/content/en/latest/pipelines/ldm_extension/_index.mdpackages/gooddata-dbt/README.mdpackages/gooddata-dbt/src/gooddata_dbt/args.pypackages/gooddata-dbt/src/gooddata_dbt/dbt/base.pypackages/gooddata-dbt/src/gooddata_dbt/dbt/tables.pypackages/gooddata-dbt/src/gooddata_dbt/dbt_plugin.pypackages/gooddata-dbt/tests/test_tables.pypackages/gooddata-pipelines/src/gooddata_pipelines/ldm_extension/input_processor.pypackages/gooddata-pipelines/src/gooddata_pipelines/ldm_extension/ldm_extension_manager.pypackages/gooddata-pipelines/tests/test_ldm_extension/test_input_processor.py
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/content/en/latest/pipelines/ldm_extension/_index.md
- packages/gooddata-dbt/README.md
- packages/gooddata-dbt/src/gooddata_dbt/args.py
- packages/gooddata-pipelines/tests/test_ldm_extension/test_input_processor.py
- packages/gooddata-pipelines/src/gooddata_pipelines/ldm_extension/ldm_extension_manager.py
- packages/gooddata-dbt/src/gooddata_dbt/dbt_plugin.py
- packages/gooddata-dbt/tests/test_tables.py
- packages/gooddata-dbt/src/gooddata_dbt/dbt/base.py
JIRA: CQ-2783 risk: low
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/gooddata-pipelines/src/gooddata_pipelines/ldm_extension/input_processor.py (1)
85-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOwn the granularity lists per processor and per date dataset.
_SECOND_DATE_GRANULARITIESis a mutable class list, which triggers RUF012. The constructor can also alias the class-level list, and every date dataset receives the same instance list. If a consumer mutates one list, later LDM output can change.Mark both class-level constants as
ClassVar, create an instance-owned list, and pass a copy to each date dataset.Proposed fix
+from typing import ClassVar + - DATE_GRANULARITIES: list[str] = [ + DATE_GRANULARITIES: ClassVar[list[str]] = [ ... ] - _SECOND_DATE_GRANULARITIES: list[str] = [ + _SECOND_DATE_GRANULARITIES: ClassVar[list[str]] = [ ... ] - self._date_granularities = ( - self.DATE_GRANULARITIES + self._SECOND_DATE_GRANULARITIES - if enable_second_granularities - else self.DATE_GRANULARITIES - ) + self._date_granularities = list(self.DATE_GRANULARITIES) + if enable_second_granularities: + self._date_granularities.extend(self._SECOND_DATE_GRANULARITIES) - granularities=self._date_granularities, + granularities=list(self._date_granularities),Also applies to: 145-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/gooddata-pipelines/src/gooddata_pipelines/ldm_extension/input_processor.py` around lines 85 - 99, Update the granularity declarations in the processor class to use ClassVar for both class-level lists, then ensure __init__ builds an instance-owned list rather than aliasing DATE_GRANULARITIES. When constructing each date dataset, pass a copy of the processor’s granularity list so datasets cannot mutate shared state.Source: Linters/SAST tools
packages/gooddata-dbt/src/gooddata_dbt/dbt/tables.py (1)
455-459: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valuePrevent granularity-list drift.
SECOND_TIMESTAMP_GRANULARITIESand_SECOND_DATE_GRANULARITIEScontain the same four values in the same order. Use one shared constant or add a cross-package contract test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/gooddata-dbt/src/gooddata_dbt/dbt/tables.py` around lines 455 - 459, Update the granularity selection in the table logic around _enable_second_granularities to avoid duplicating the four second-level values between SECOND_TIMESTAMP_GRANULARITIES and _SECOND_DATE_GRANULARITIES. Reuse a shared constant accessible to both definitions, preserving the current values and ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/gooddata-dbt/src/gooddata_dbt/dbt/tables.py`:
- Around line 455-459: Update the granularity selection in the table logic
around _enable_second_granularities to avoid duplicating the four second-level
values between SECOND_TIMESTAMP_GRANULARITIES and _SECOND_DATE_GRANULARITIES.
Reuse a shared constant accessible to both definitions, preserving the current
values and ordering.
In
`@packages/gooddata-pipelines/src/gooddata_pipelines/ldm_extension/input_processor.py`:
- Around line 85-99: Update the granularity declarations in the processor class
to use ClassVar for both class-level lists, then ensure __init__ builds an
instance-owned list rather than aliasing DATE_GRANULARITIES. When constructing
each date dataset, pass a copy of the processor’s granularity list so datasets
cannot mutate shared state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed2e4d9e-24c9-4416-bac3-c0d117a63abb
📒 Files selected for processing (11)
docs/content/en/latest/pipelines/ldm_extension/_index.mdpackages/gooddata-dbt/README.mdpackages/gooddata-dbt/src/gooddata_dbt/args.pypackages/gooddata-dbt/src/gooddata_dbt/dbt/base.pypackages/gooddata-dbt/src/gooddata_dbt/dbt/tables.pypackages/gooddata-dbt/src/gooddata_dbt/dbt_plugin.pypackages/gooddata-dbt/tests/test_args.pypackages/gooddata-dbt/tests/test_tables.pypackages/gooddata-pipelines/src/gooddata_pipelines/ldm_extension/input_processor.pypackages/gooddata-pipelines/src/gooddata_pipelines/ldm_extension/ldm_extension_manager.pypackages/gooddata-pipelines/tests/test_ldm_extension/test_input_processor.py
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/gooddata-dbt/src/gooddata_dbt/args.py
- docs/content/en/latest/pipelines/ldm_extension/_index.md
- packages/gooddata-dbt/src/gooddata_dbt/dbt/base.py
- packages/gooddata-dbt/README.md
- packages/gooddata-dbt/src/gooddata_dbt/dbt_plugin.py
- packages/gooddata-dbt/tests/test_tables.py
- packages/gooddata-pipelines/tests/test_ldm_extension/test_input_processor.py
- packages/gooddata-pipelines/src/gooddata_pipelines/ldm_extension/ldm_extension_manager.py
janmatzek
approved these changes
Aug 6, 2026
hkad98
reviewed
Aug 6, 2026
|
|
||
| To extend the LDM, you need to define the custom datasets and the fields they should contain. The script also checks the validity of analytical objects before and after the update. Updates introducing new invalid relations are automatically rolled back. You can opt out of this behavior by setting the `check_relations` parameter to False. | ||
|
|
||
| To create date datasets with the second-based granularities (`SECOND`, `SECOND_OF_MINUTE`, `SECOND_OF_DAY`, `MINUTE_OF_DAY`), set the `enable_second_granularities` parameter to True when creating the LdmExtensionManager. This requires the `enableSecondGranularities` feature flag to be enabled for your GoodData organization. |
Contributor
There was a problem hiding this comment.
Nit: do not mention ff here, please. Cloud customers do not about feature flags.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JIRA: CQ-2783
risk: low
Summary by CodeRabbit
New Features
Documentation
Tests