Skip to content

test(integ-test): stabilize floating window SUM cases - #5744

Open
mengweieric wants to merge 1 commit into
opensearch-project:mainfrom
mengweieric:menwe/window-sum-float-stability
Open

test(integ-test): stabilize floating window SUM cases#5744
mengweieric wants to merge 1 commit into
opensearch-project:mainfrom
mengweieric:menwe/window-sum-float-stability

Conversation

@mengweieric

Copy link
Copy Markdown
Collaborator

Description

Two SQL correctness cases were unstable when the flights fixture used multiple shards:

  • SUM(DistanceMiles) OVER () intermittently differed from H2 and SQLite by 0.01.
  • SUM(DistanceMiles) OVER (ORDER BY FlightDelayMin) also compared rows positionally even though 17 rows share the same ordering key and the query had no outer ORDER BY.

The correctness runner rounds floating cells to two decimals with CEILING and then compares them exactly. The legacy SQL engine preserves SUM(FLOAT) as FLOAT, so shard-dependent input order can move the single-precision result across a cent boundary without changing the window semantics.

This change casts only the summed operand in these two queries to DOUBLE. It also adds a unique outer ordering to the ordered case. The internal window order and its default RANGE frame remain unchanged.

No production code, shared comparator, or fixture is modified.

Validation

Focused tests used the official SQL correctness comparison framework against OpenSearch, H2, and SQLite.

Fixture shards Repeated runs Result
1 1 Pass
3 5 5/5 pass
5 10 10/10 pass

Mutation checks:

  • Removing only the DOUBLE casts reproduced floating SUM failures in 10/10 five-shard runs.
  • Removing only the outer tiebreaker reproduced positional failures in 3/3 five-shard runs.

compileTestJava and git diff --check pass.

Related Issues

None. This is a test-only alignment with the existing approximate floating-point contract.

Check List

  • New functionality includes testing.
  • Commits are signed per the DCO using --signoff or -s.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit e13e060)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to e13e060
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Add missing column to SELECT

The ORDER BY clause references FlightNum which is not in the SELECT list. This may
cause errors in some SQL engines or produce unexpected results. Consider either
adding FlightNum to the SELECT clause or removing it from ORDER BY if it's not
necessary for test stability.

integ-test/src/test/resources/correctness/queries/window.txt [16]

-SELECT FlightDelayMin, DistanceMiles, SUM(CAST(DistanceMiles AS DOUBLE)) OVER (ORDER BY FlightDelayMin) AS num FROM opensearch_dashboards_sample_data_flights ORDER BY FlightDelayMin, DistanceMiles, FlightNum
+SELECT FlightDelayMin, DistanceMiles, FlightNum, SUM(CAST(DistanceMiles AS DOUBLE)) OVER (ORDER BY FlightDelayMin) AS num FROM opensearch_dashboards_sample_data_flights ORDER BY FlightDelayMin, DistanceMiles, FlightNum
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that FlightNum is referenced in the ORDER BY clause but not in the SELECT list. While some SQL engines allow this, adding it to SELECT ensures consistency and prevents potential errors across different SQL implementations.

Medium

Previous suggestions

Suggestions up to commit 83fae4d
CategorySuggestion                                                                                                                                    Impact
Possible issue
Remove undefined column from ORDER BY

The ORDER BY clause references FlightNum which is not in the SELECT list. This may
cause errors in some SQL engines or produce unexpected results. Consider either
adding FlightNum to the SELECT clause or removing it from the ORDER BY clause.

integ-test/src/test/resources/correctness/queries/window.txt [16]

-SELECT FlightDelayMin, DistanceMiles, SUM(CAST(DistanceMiles AS DOUBLE)) OVER (ORDER BY FlightDelayMin) AS num FROM opensearch_dashboards_sample_data_flights ORDER BY FlightDelayMin, DistanceMiles, FlightNum
+SELECT FlightDelayMin, DistanceMiles, SUM(CAST(DistanceMiles AS DOUBLE)) OVER (ORDER BY FlightDelayMin) AS num FROM opensearch_dashboards_sample_data_flights ORDER BY FlightDelayMin, DistanceMiles
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that FlightNum is referenced in the ORDER BY clause but not in the SELECT list. While some SQL engines allow this, it can cause issues or unexpected behavior. However, this may be intentional for testing purposes in an integration test file.

Medium

@dai-chen dai-chen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I forgot where is exact the code. But could you double check if adjust precision is the better way like here https://github.com/opensearch-project/sql/blob/main/integ-test/src/test/java/org/opensearch/sql/correctness/runner/resultset/Row.java#L37

Comment thread integ-test/src/test/resources/correctness/queries/window.txt
The SQL correctness runner compares OpenSearch with H2 and SQLite after
rounding floating cells to two decimals with CEILING. The legacy SQL engine
keeps SUM(FLOAT) in single precision, so multi-shard merge order can move
the result across a cent boundary even though the window is correct.

Cast the summed operand to DOUBLE in the two affected window queries. This
keeps the tests focused on window behavior without requiring an exact cent
from an order-dependent float32 accumulator.

The ordered case also had no outer ORDER BY. Its internal window ordering
contains 17 FlightDelayMin=0 peers, so the harness compared a shard-dependent
physical row order positionally. Add a unique outer ordering using FlightNum
without changing the internal RANGE window frame.

Signed-off-by: Eric Wei <menwe@amazon.com>
@mengweieric
mengweieric force-pushed the menwe/window-sum-float-stability branch from 83fae4d to e13e060 Compare September 4, 2026 22:35
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit e13e060

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Related to improving software testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants