-
Notifications
You must be signed in to change notification settings - Fork 4
Added new metadata field forced-replication-method
#26
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
base: gl-master
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR adds support for a new metadata field forced-replication-method to control replication strategy on a per-table basis. The implementation determines whether tables should use incremental or full table replication based on manifest configuration.
Key Changes:
- Added logic to extract and track incremental status from table manifests
- Implemented
forced-replication-methodmetadata field withINCREMENTALorFULL_TABLEvalues - Refactored metadata handling to support the new field before converting to list format
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tap_heap/discover.py
Outdated
| table_name_to_incremental = {} | ||
|
|
||
| for all_table_manifests in manifests.values(): | ||
| for table_name, table_manifest in all_table_manifests.items(): | ||
| table_name_to_columns[table_name].update(set(table_manifest['columns'])) | ||
| table_name_to_incremental[table_name] = table_manifest.get('incremental', False) | ||
|
|
||
| for table_name, columns in table_name_to_columns.items(): | ||
| schema = generate_fake_schema(columns) | ||
| mdata = load_metadata(table_name, schema) | ||
|
|
||
| if table_name_to_incremental.get(table_name, False): |
Copilot
AI
Nov 7, 2025
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.
The incremental status is being overwritten on each iteration for the same table_name. If a table appears in multiple manifests with different incremental values, only the last one will be retained. Consider using a set or checking for conflicts to ensure consistent behavior across manifests.
| table_name_to_incremental = {} | |
| for all_table_manifests in manifests.values(): | |
| for table_name, table_manifest in all_table_manifests.items(): | |
| table_name_to_columns[table_name].update(set(table_manifest['columns'])) | |
| table_name_to_incremental[table_name] = table_manifest.get('incremental', False) | |
| for table_name, columns in table_name_to_columns.items(): | |
| schema = generate_fake_schema(columns) | |
| mdata = load_metadata(table_name, schema) | |
| if table_name_to_incremental.get(table_name, False): | |
| table_name_to_incremental = defaultdict(set) | |
| for all_table_manifests in manifests.values(): | |
| for table_name, table_manifest in all_table_manifests.items(): | |
| table_name_to_columns[table_name].update(set(table_manifest['columns'])) | |
| table_name_to_incremental[table_name].add(table_manifest.get('incremental', False)) | |
| # Resolve incremental status for each table, checking for conflicts | |
| resolved_table_name_to_incremental = {} | |
| for table_name, incremental_set in table_name_to_incremental.items(): | |
| if len(incremental_set) > 1: | |
| raise ValueError(f"Conflicting incremental values for table '{table_name}': {incremental_set}") | |
| resolved_table_name_to_incremental[table_name] = next(iter(incremental_set)) | |
| for table_name, columns in table_name_to_columns.items(): | |
| schema = generate_fake_schema(columns) | |
| mdata = load_metadata(table_name, schema) | |
| if resolved_table_name_to_incremental.get(table_name, False): |
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.
addressed
| if not flags: | ||
| replication = 'FULL_TABLE' | ||
| elif flags == {True}: | ||
| replication = 'INCREMENTAL' | ||
| elif flags == {False}: | ||
| replication = 'FULL_TABLE' | ||
| else: | ||
| LOGGER.warning("Conflicting incremental flags for %s: %s. Defaulting to \ | ||
| INCREMENTAL.", table_name, flags) | ||
| replication = 'INCREMENTAL' |
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.
| if not flags: | |
| replication = 'FULL_TABLE' | |
| elif flags == {True}: | |
| replication = 'INCREMENTAL' | |
| elif flags == {False}: | |
| replication = 'FULL_TABLE' | |
| else: | |
| LOGGER.warning("Conflicting incremental flags for %s: %s. Defaulting to \ | |
| INCREMENTAL.", table_name, flags) | |
| replication = 'INCREMENTAL' | |
| if flags == {True}: | |
| replication = 'INCREMENTAL' | |
| elif not flags or flags == {False}: | |
| replication = 'FULL_TABLE' | |
| else: | |
| LOGGER.warning("Conflicting incremental flags for %s: %s. Defaulting to \ | |
| INCREMENTAL.", table_name, flags) | |
| replication = 'INCREMENTAL' |
Description of change
SAC-28842
Added new metadata field in catalog's metadata, field name is forced-replication-method.
Manual QA steps
Risks
Rollback steps
AI generated code
https://internal.qlik.dev/general/ways-of-working/code-reviews/#guidelines-for-ai-generated-code