Skip to content

Optimize: constant-fold interval conversions for literal intervals#1707

Merged
JPercival merged 1 commit intomainfrom
fix/interval-conversion-constant-folding
Mar 18, 2026
Merged

Optimize: constant-fold interval conversions for literal intervals#1707
JPercival merged 1 commit intomainfrom
fix/interval-conversion-constant-folding

Conversation

@JPercival
Copy link
Copy Markdown
Contributor

@JPercival JPercival commented Mar 18, 2026

Summary

When convertIntervalExpression receives an Interval literal (e.g., Interval(null, null)), it now directly converts the bounds and preserves lowClosed/highClosed booleans instead of using runtime Property extraction.

Before:

Interval(
  low = As(Integer, Property(source=Interval(null, null), path="low")),
  lowClosedExpression = Property(source=Interval(null, null), path="lowClosed"),
  ...)

After:

Interval(low = As(Integer, null), lowClosed = false, ...)

The Property extraction path is still used for non-literal intervals (expression refs, function call results) where bounds aren't statically known.

Test plan

  • All existing cql-to-elm JVM tests pass
  • The optimization only fires for Interval literals, preserving existing behavior for computed intervals

🤖 Generated with Claude Code

@JPercival @claude
perf: constant-fold interval conversions for literal intervals
7b6f511 
When convertIntervalExpression receives an Interval literal (not a
computed expression), directly convert the bounds and preserve the
lowClosed/highClosed booleans instead of using runtime Property
extraction.

Before: Interval(low=As(T, Property("low", source=Interval(...))), ...)
After:  Interval(low=As(T, originalLow), lowClosed=originalLowClosed, ...)

The Property extraction is still used for non-literal intervals
(expression refs, function call results) where bounds aren't known
at compile time.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 18, 2026

Related Issues

The following open issues may be related to this PR:

Issue Title Score Matched Terms
#442 FunctionRef Signatures in Nested Function Queries 18 "cql elm", "property source", path, property, function, com, source, null, expression, https, elm, cqframework (path), java (path), src (path), main (path), org (path), cql (path)
#576 Interval end introduces an unnecessary Query in the output ELM 17.5 "interval low", "property source", before, low, path, highclosed, property, source, lowclosed, interval, expression, elm, test, library (path)
#1400 patientBirthDatePropertyName marked as deprecated but still required for SystemFunctionResolver 16 "cql elm", non, instead, com, code, behavior, call, null, https, elm, cqframework (path), java (path), src (path), cql2elm (path), library (path), main (path), org (path), cql (path)
#1133 ToList is not implemented according to the spec 15 "cql elm", property, com, code, behavior, null, expression, https, elm, test, cqframework (path), java (path), src (path), main (path), org (path), cql (path)
#734 Width allows Interval as a parameter 14 "cql elm", "interval null", integer, intervals, interval, null, https, elm, tests, org (path), cql (path)

Tip: If this PR addresses any of these issues, please link them using Closes #NNN or Refs #NNN in the PR description.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 18, 2026

Formatting check succeeded!

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 18, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.97%. Comparing base (75aa1b6) to head (7b6f511).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...tlin/org/cqframework/cql/cql2elm/LibraryBuilder.kt 81.81% 0 Missing and 2 partials ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #1707      +/-   ##
============================================
+ Coverage     61.96%   61.97%   +0.01%     
- Complexity     3940     3941       +1     
============================================
  Files           210      210              
  Lines         20367    20378      +11     
  Branches       3879     3882       +3     
============================================
+ Hits          12621    12630       +9     
  Misses         6139     6139              
- Partials       1607     1609       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Mar 18, 2026

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

c-schuler
c-schuler approved these changes Mar 18, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@c-schuler c-schuler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! The only thing I'd point out is that the existing test for this path only validates that no errors occurred. There's no test that verifies the optimized output differs from the old output or that the simpler ELM evaluates correctly. Not a blocker for approval though

@JPercival JPercival merged commit ae88efe into main Mar 18, 2026
9 of 10 checks passed
@JPercival JPercival deleted the fix/interval-conversion-constant-folding branch March 18, 2026 19:29
JPercival added a commit that referenced this pull request Mar 18, 2026
@JPercival @claude
Fix: broaden interval constant-folding to handle As-wrapped literals
9ff2fe8 
#1707 constant-folds interval conversions for bare Interval literals,
but some operator paths (e.g., `ends`) route through a cast+interval
conversion chain where convertExpression wraps the Interval in an As
cast before the interval conversion. The `expression is Interval`
check then fails, falling back to runtime Property extraction.

Unwrap As casts to find the inner Interval literal, enabling constant
folding for all operator paths that produce Interval(null, null) or
similar literal interval conversions.

Extracted constantFoldIntervalConversion and unwrapIntervalLiteral
helpers to keep convertIntervalExpression within the detekt line limit.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@JPercival JPercival mentioned this pull request Mar 18, 2026
Merged
2 tasks
JPercival added a commit that referenced this pull request Mar 18, 2026
@JPercival @claude
Fix: broaden interval constant-folding to handle As-wrapped literals (#…
20b6cd7 
…1709)

#1707 constant-folds interval conversions for bare Interval literals,
but some operator paths (e.g., `ends`) route through a cast+interval
conversion chain where convertExpression wraps the Interval in an As
cast before the interval conversion. The `expression is Interval`
check then fails, falling back to runtime Property extraction.

Unwrap As casts to find the inner Interval literal, enabling constant
folding for all operator paths that produce Interval(null, null) or
similar literal interval conversions.

Extracted constantFoldIntervalConversion and unwrapIntervalLiteral
helpers to keep convertIntervalExpression within the detekt line limit.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@c-schuler c-schuler c-schuler approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@JPercival @c-schuler