Fix: forward aggregate report_type to TriggerReconAggregateService#2423
Open
moomindani wants to merge 7 commits into
Open
Fix: forward aggregate report_type to TriggerReconAggregateService#2423moomindani wants to merge 7 commits into
moomindani wants to merge 7 commits into
Conversation
TriggerReconService.trigger_recon validates that report_type is in
{"schema","data","row","all","aggregate"} via _RECON_REPORT_TYPES, so
"aggregate" is accepted at the entry point. However, _do_recon_one only
branches on {"schema","all"} and {"data","row","all"}; the "aggregate"
case falls through, no comparison is performed, and recon_capture.start()
records the run with status=true. Callers see a successful reconciliation
even when source and target differ.
The aggregate path lives in TriggerReconAggregateService.
trigger_recon_aggregates. CLI dispatch in execute.py already routes the
AGG_RECONCILE_OPERATION_NAME to that service, but programmatic callers
of TriggerReconService.trigger_recon (e.g. notebooks built around the
public Reconcile API) hit the silent no-op.
Forward report_type=="aggregate" to TriggerReconAggregateService at the
top of trigger_recon. The import is deferred because
trigger_recon_aggregate_service already imports trigger_recon_service.
Co-authored-by: Isaac
Cover the routing introduced in this branch:
- report_type='aggregate' is forwarded to
TriggerReconAggregateService.trigger_recon_aggregates without going
through create_recon_dependencies of the data path
- The aggregate-specific kwargs (table_recon, reconcile_config,
local_test_run) are passed through unchanged
- All other report_types ('schema', 'data', 'row', 'all') keep the
existing path and never call the aggregate service
Without the dispatch, the existing _do_recon_one short-circuits silently
for report_type='aggregate' and the run is recorded as successful. The
first test reproduces the original behavior (no aggregate forward) and
fails on origin/main.
Co-authored-by: Isaac
…ispatch-in-trigger-recon # Conflicts: # src/databricks/labs/lakebridge/reconcile/trigger_recon_service.py
6 tasks
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2423 +/- ##
==========================================
- Coverage 65.78% 64.89% -0.90%
==========================================
Files 98 104 +6
Lines 9237 9418 +181
Branches 992 993 +1
==========================================
+ Hits 6077 6112 +35
- Misses 2984 3129 +145
- Partials 176 177 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
❌ 141/148 passed, 7 failed, 5 skipped, 26m1s total ❌ test_schema_recon_with_data_source_exception: AttributeError: does not have the attribute 'uuid4' (12.52s)❌ test_recon_for_wrong_report_type: AttributeError: does not have the attribute 'uuid4' (10.098s)❌ test_schema_recon_with_general_exception: AttributeError: does not have the attribute 'uuid4' (12.063s)❌ test_recon_for_report_type_is_data: AttributeError: does not have the attribute 'uuid4' (10.982s)❌ test_data_recon_with_general_exception: AttributeError: does not have the attribute 'uuid4' (30.556s)❌ test_recon_for_report_type_schema: AttributeError: does not have the attribute 'uuid4' (29.576s)❌ test_data_recon_with_source_exception: AttributeError: does not have the attribute 'uuid4' (13.126s)Running from acceptance #4337 |
black prefers parenthesised multi-context manager syntax over backslash-style. Co-authored-by: Isaac
The deferred import of TriggerReconAggregateService is intentional to break the module-load cycle (trigger_recon_aggregate_service already imports trigger_recon_service). pylint still flags it as a code smell. The cyclic-import + import-outside-toplevel pair is whitelisted by tests/unit/no_cheat.py for exactly this case. Co-authored-by: Isaac
m-abulazm
requested changes
May 11, 2026
Contributor
m-abulazm
left a comment
m-abulazm
left a comment
There was a problem hiding this comment.
we do not support this behavior. users can run aggregate reconcile using cli databricks labs lakebridge aggregates_reconcile or the service directly if using python
Per review feedback, the deferred import workaround in trigger_recon_service.py introduced a real cyclic dependency between TriggerReconService and TriggerReconAggregateService. Extract the three helpers (create_recon_dependencies, verify_successful_reconciliation, get_schemas) plus a new cleanup_intermediate_persist helper into a new module reconcile/recon_service_helpers.py. Both services now import from the helpers module instead of from each other, and trigger_recon_service.py can safely import TriggerReconAggregateService at module load time for the aggregate dispatch. cleanup_intermediate_persist also dedupes the dbfs cleanup block that was otherwise flagged by pylint's similarity checker. Co-authored-by: Isaac
Contributor
Author
|
Reopening after addressing the cyclic-dependency concern we discussed. The previous version used a deferred import inside This revision breaks the cycle by extracting the shared helpers ( Dependency graph after the change:
Behavior is unchanged: The diff is larger than the actual new logic — most of it is mechanical. Breakdown:
The actual new behavior is: (1) the 5-line aggregate dispatch in |
moomindani
force-pushed
the
fix/recon-aggregate-dispatch-in-trigger-recon
branch
from
749aba7 to
2eb5f36
Compare
moomindani
force-pushed
the
fix/recon-aggregate-dispatch-in-trigger-recon
branch
from
2eb5f36 to
c5a750a
Compare
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
2 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.
Changes
What does this PR do?
Forwards
report_type=="aggregate"fromTriggerReconService.trigger_recontoTriggerReconAggregateService.trigger_recon_aggregatesso programmatic callers that pass an aggregate report type get the actual aggregate comparison instead of a silent no-op.Relevant implementation details
TriggerReconService.trigger_reconvalidatesreport_typeagainst_RECON_REPORT_TYPES = {"schema","data","row","all","aggregate"}, so"aggregate"is accepted at entry. However,_do_recon_oneonly branches on{"schema","all"}and{"data","row","all"}. The aggregate case falls through, no comparison runs, andrecon_capture.start()records the run withstatus=true. Callers see a successful reconciliation even when source and target differ.The CLI dispatch in
execute.pyalready routesAGG_RECONCILE_OPERATION_NAMEtoTriggerReconAggregateService.trigger_recon_aggregates, but programmatic users of the public Reconcile API (notebooks, integrations) hit the silent no-op without this change.This PR adds an early dispatch at the top of
trigger_recon. The import is deferred becausetrigger_recon_aggregate_servicealready importstrigger_recon_serviceand a top-level import would be circular.Caveats/things to watch out for when reviewing:
execute.py's existing dispatchschema,data,row,all) keep the original pathLinked issues
None
Functionality
TriggerReconService.trigger_reconTests
Manual end-to-end on Databricks Serverless with two tables of different row counts:
aggregate=None,status=true, no rows inrecon.aggregate_*aggregate=False,mismatch=1, rows present inrecon.aggregate_metricsUnit tests added in
tests/unit/reconcile/test_verify_reconcile.py:test_trigger_recon_dispatches_aggregate_to_aggregate_service: forwarding works and the data-path setup is skippedtest_trigger_recon_does_not_dispatch_non_aggregate:schema/data/row/allkeep the original pathReopened from #2401 on an upstream branch to bypass the fork-PR OIDC restriction on JFrog auth (CI cannot run on fork PRs). All review comments and history are preserved on the original PR.