fix(workflow-operator, v1.2): fall back to day semantics for an unset interval type; extend aggregate and interval-join tests - #7007
Open
Yicong-Huang wants to merge 1 commit into
Conversation
…val type; extend aggregate and interval-join tests (#6907) ### What changes were proposed in this PR? Writing the coverage tests asked for in #6901 turned up a live defect in `IntervalJoinOpExec`, so this fixes that and adds the tests around it. **An unset `timeIntervalType` crashed the timestamp branch instead of falling back to days.** `IntervalJoinOpDesc` declares `var timeIntervalType: Option[TimeIntervalType] = _`, and an absent JSON field deserializes to `Some(null)` — not `None`, and not `null`. The match in `intervalCompare` only handled `Some(<each unit>)` and `None`, so any TIMESTAMP interval join whose descriptor omits the field died with `scala.MatchError: Some(null)` rather than taking the documented day fallback. Replaced `case None` with a catch-all, which also covers `None` and any unit added later. This makes the fallback arm reachable for the first time; it was dead code before. On the coverage side, `AggregationOperationSpec` gains the accepting half of the SUM/MIN/MAX type guard — the four-clause `!= INTEGER && != DOUBLE && != LONG && != TIMESTAMP` chain was only ever driven with INTEGER/DOUBLE and the rejecting types, so its later clauses were never walked — plus AVERAGE over a TIMESTAMP column, which is the only route into the `parseTimestamp(...).getTime` branch of `getNumericalValue` (AVERAGE is otherwise only exercised over DOUBLE). Two items from the issue are deliberately **not** included, because they are already covered on `main`: the `TimeIntervalType` ladder and the `processNumValue` bound-combination arms were covered by #6098 (`"join timestamps across every interval unit"` plus the four `[] [) (] ()` tests), and `newAggFunc`'s non-COUNT arm is covered by the existing SUM `getFinal` pipeline test. `AveragePartialObj` is likewise already constructed by an existing test. The `maxAgg` finaliser asymmetry the issue mentions is left alone — it is a behavior change in live code and deserves its own PR. ### Any related issues, documentation, discussions? Closes #6901. ### How was this PR tested? Both touched specs pass (27 tests), and the full `WorkflowOperator/test` suite passes with 1957 tests and no regressions; `scalafmtCheck` and `scalafix --check` are clean. The new interval-join test is the regression test for the fix: reverting the catch-all back to `case None` reddens it with the original `scala.MatchError: Some(null)`, which is how the defect was found in the first place. ### Was this PR authored or co-authored using generative AI tooling? (backported from commit 03a0e3a) Generated-by: Claude Code (Fable 5)
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/v1.2 #7007 +/- ##
==================================================
- Coverage 53.95% 52.39% -1.56%
- Complexity 1441 2493 +1052
==================================================
Files 809 1077 +268
Lines 34144 42266 +8122
Branches 3448 4547 +1099
==================================================
+ Hits 18421 22146 +3725
- Misses 14815 18813 +3998
- Partials 908 1307 +399
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
What changes were proposed in this PR?
Backport of #6907 to
release/v1.2, cherry-picked from 03a0e3a.Source fix only. The test changes from #6907 were omitted: the touched test spec(s) do not exist on
release/v1.2(or depend on main-only test infrastructure), so backporting them cleanly is not possible. Only the source fix is carried over, per maintainer guidance.Any related issues, documentation, discussions?
Backport of #6907. Originally linked #6901.
How was this PR tested?
Release-branch CI runs on this PR. The source change cherry-picked cleanly; test changes were intentionally dropped (see above).
Was this PR authored or co-authored using generative AI tooling?
Yes — backport prepared with Claude Code (mechanical cherry-pick + conflict resolution; the change itself is #6907 by its original author).
🤖 Generated with Claude Code