Fix time-bucket bug for advanced groups format#65
Merged
Merged
Conversation
Owner
Author
|
Aim to fix #46 |
Users configuring criteria in advanced groups format with AND-only operators were not receiving the sliding-window time-merge fix applied for issue #40. This caused photos straddling time-delta bucket boundaries to fail to stack. Add dispatcher logic in StackBy to detect AND-only groups, convert them to an OR(AND, AND, ...) expression tree, and route through stackByAdvanced which includes the sliding-window merge post-pass. Introduce helper functions: - canConvertGroupsToExpression: Checks whether groups contain only AND operators (safe for expression conversion) - convertGroupsToExpression: Builds expression tree from AND groups with proper handling of empty groups and single criteria - groupToExpression: Converts single AND group to expression node OR groups remain on the legacy connectivity-graph path since their cross-criterion union semantics are not representable in expression OR. Change-Type: fix Scope: stacker
Add test helpers (quietTestLogger, mustMarshalCriteria, assertContainsStack, stringSet) and three regression tests validating that AND-only groups inherit the sliding-window time-merge behavior. Include TestStackByGroups_AndGroupsApplyTimeBucketMerge which exercises realistic Pixel Motion-Photo and DSC RAW+JPEG variants straddling bucket boundaries — the exact symptom reported in issue #46. Also test single AND group routing and verify OR groups continue using legacy union semantics. All tests exercise the public StackBy entry point to validate the dispatcher fix end-to-end. Change-Type: test Scope: stacker
Majorfi
changed the title
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
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.
Summary
Closes #46. AND-group configs in advanced mode skipped the sliding-window time-bucket merge that was added in #40 for the legacy and expression code paths, so photos within delta but straddling a fixed bucket boundary failed to stack. The user reported "only 2 of 3 stacks formed" on his test set; with the same logical config expressed as an expression, all 3 stacked correctly.
Root cause
Commit d94ba48 (the #40 fix) added a post-pass call to
mergeTimeBasedGroupsinstackByLegacyandstackByAdvancedbut NOT instackByLegacyGroups. The bucket-division code inextractTimeWithDeltais still used by all three modes for the initial keying; the sliding-window post-pass is what recovers boundary-straddling assets, and the groups mode never ran it.What changed
pkg/stacker/stacker.go: dispatcher now routes AND-only groups throughstackByAdvanced(which already has the sliding-window merge). OR groups stay on the legacy connectivity-graph path because their cross-criterion union semantics aren't representable in expression OR (walkMatchingCriteriacollects values only from the first matching child).pkg/stacker/stacker_grouping.go: new helperscanConvertGroupsToExpression,convertGroupsToExpression,groupToExpressionperform the syntactic AND-group to OR(AND,...) rewrite. Documented caveat for overlapping AND groups.pkg/stacker/advanced_test.go: 3 regression tests + 4 shared helpers, placed next to the existingTestStackByLegacyGroups_UnionSemantics.Test plan
go test ./... -count=1go test -race ./...go vet ./...TestStackByLegacyGroups_UnionSemanticssubtests pass (OR semantics preserved)canConvertGroupsToExpressionand confirmed the new regression test FAILS with exactly the "2 of 3 stacks" symptom from the issueBreaking changes
None. OR groups keep the legacy code path. AND groups produce equivalent stacks (the conversion is a pure syntactic rewrite of AND group operators into expression AND nodes — same matching values, same extractors).