test(integ-test): add OTel data stream ITs that track known schema evolution 500s

[cnoramut]

Description

Adds PPL integration tests over OTel logs and traces data streams for #5806, scope items 1 to 4. Each test rolls a data stream over so that the second backing index changes, removes or adds one field, then runs PPL through the stream name and a wildcard. A 500 fails the run unless KNOWN_500S lists it with its issue.

	File	What
1	OtelDataStreamConflictTestCase.java	The test flow shared by both signals. A Scenario is one rollover, an Observation is the outcome of one query, and Observations holds the checks that turn them into failures
2	CalciteOtelLogsDataStreamIT.java and CalciteOtelTracesDataStreamIT.java	The dataset for each signal
3	MappingConflictFixtures.java	The data both ITs run over. It holds the 7 field types, the known 500s, and 19 representative PPL commands, one per way PPL reads, filters, aggregates, orders or windows the field, covering each command family the mapping conflict issues in #5829 report as a 500. A fix that clears a 500 edits only this file
4	build.gradle	A mappingConflictIntegTest task on its own single-node cluster, excluded from integTest and integTestRemote. With assertions on, the node exits on the InternalComposite$InternalBucket.compareKey assert that every #5811 query reaches, so this cluster turns off the InternalComposite asserts and returns the 500 a production node returns
5	sql-test-and-build-workflow.yml	Runs the task in both integration steps, after yamlRestTest. It took about 3.5 minutes locally

How it works. Each scenario does five steps.

1. It creates the OTel data stream from its Data Prepper template and loads 5 documents into backing index 000001.
2. It rolls the stream over once, so 000002 changes one attributes.types.<type> field to another type, drops it, or adds a new one, and loads 5 more documents.
3. It reads the type the merged schema picked for that field, with fields <field> | head 1.
4. It runs each of the 19 commands through the stream name and through <stream>*, and records the HTTP status.
5. It deletes the stream.

The 21 pairs among text, keyword, byte, integer, long, date and object each take two scenarios, with the types swapped between 000001 and 000002. Inside one node the merge reads the backing indices in a fixed order, which a standalone probe showed and the run does not re-check, so the two scenarios give the merge both orders a two-index stream can produce, and cover every winner production can return. When a merge rule picks the type, such as text against keyword, both scenarios read that type. Each type is also removed once and added once, for 56 scenarios per signal.

The run refuses a cluster with more than one node, since the scenarios rely on one fixed read order. It fails on a 500 missing from KNOWN_500S and on a known 500 that no longer returns 500, as the lifecycle below shows. It also fails on a command that never returns 200, since such a command tests nothing.

What it checks. Only the never-500 part of #5806's acceptance, at pushdown on. A command's 200 or 4xx outside KNOWN_500S passes.

Known 500s. Today 93 keys return 500 in each IT, from four causes, where a key is one pair, winner and command, run through both sources for 186 queries. Logs and traces return the same ones, since the drift lands on the same fields. A Known500 entry covers both sources by default, and it can name DATA_STREAM or WILDCARD for a 500 that only one source returns.

Issue	Commands	Pairs
#5811	stats count() by, dedup, top, rare, chart, timechart	keyword against byte, integer or long, and against date when the string side wins
#5825	sort, reverse and head after a sort, and a self join	keyword against byte, integer, long or date
#5835	stats max, pushed down as a sorted top_hits	the same four, when the string side wins
#5836	the same sorts and stats max on an object	text/object, object/absent and absent/object

How a known 500 behaves in CI. One query, stats max over keyword/byte when the string side wins, through its life.

1. Today it returns 500, and its [BUG] stats max and stats min return 500 when a field is keyword or ip and boolean or a number in another #5835 entry in KNOWN_500S expects that, so the run passes.
2. The PR that fixes [BUG] stats max and stats min return 500 when a field is keyword or ip and boolean or a number in another #5835 turns the query into a 200. That PR's run fails until it deletes the entry, or narrows it if the fix clears only some queries.

listed under #5835 in KNOWN_500S but no longer returns 500, pair keyword/byte, winner string, command stats_max, narrow that entry to the queries that still fail
  query: source=otel-logs-drift* | stats max(`attributes.types.byte`)

3. With the entry gone, the run passes again, and the query now guards against a regression.
4. If a later PR breaks it again, the query is not in KNOWN_500S, so that PR's run fails.

500 missing from KNOWN_500S, pair keyword/byte, winner string, command stats_max
  query: source=otel-logs-drift* | stats max(`attributes.types.byte`)
  error: RuntimeException: java.sql.SQLException: exception while executing query: Failed to fetch data from the index: the background task failed or interrupted.   Inner error: Failed to execute phase [fetch]

A brand new 500 fails the same way as step 4. Its author fixes it, or files an issue and adds an entry to KNOWN_500S.

Testing. ./gradlew :integ-test:mappingConflictIntegTest passes on a local single node, macOS on JDK 21. Each IT ran 2,128 queries in about 95 s. CI on the first commit passed every integration job, and the Linux JDK 25 and Windows JDK 21 logs show both ITs passing. A temporary fake entry failed with no longer returns 500, a dropped entry with 500 missing from KNOWN_500S, and planted mistakes in an entry with their own messages. One plant that fires every failure message gave the same failure text before and after the code was split into scenarios and named checks. Without the -da flag the node exits at compareKey. spotlessCheck passes.

Related Issues

Part of #5806.

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 3a325f2.

Path	Line	Severity	Description
integ-test/build.gradle	512	low	Hardcoded encryption master key '1234567812345678' set as a cluster setting in the test configuration. While acceptable for a local test cluster, weak hardcoded keys can propagate into staging/production if config is reused.
integ-test/build.gradle	1063	low	Hardcoded credentials (user='admin', password='admin') passed as system properties to the mappingConflictIntegTest task. Consistent with existing test cluster patterns but worth noting; these values should never reach a non-test environment.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 0 | Low: 2

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 3a325f2)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The requireOneNode method throws an exception if the cluster has more than one node, but this check occurs after super.init() and enableCalcite() have already executed. If the cluster has multiple nodes, the test will fail after partial initialization, potentially leaving resources in an inconsistent state. The node count check should occur before any initialization steps. private void requireOneNode() throws IOException { JSONArray nodes = new JSONArray(executeRequest(new Request("GET", "/_cat/nodes?format=json"))); if (nodes.length() != 1) { throw new IllegalStateException( "The scenarios need one node to hold the merge order fixed, the cluster has " + nodes.length()); } Resource Leak In observeScenario, if an exception occurs after stream.rollover() but before the observations loop completes, the stream is deleted in the catch block. However, if the exception occurs during stream.rollover() itself, the stream may be left in a partially rolled-over state and still deleted, potentially masking the root cause. More critically, if stream.delete() in the catch block throws an exception that is added as suppressed, the original exception's context about which operation failed (create vs rollover vs observation) is lost. private List<Observation> observeScenario(String name, Scenario scenario) throws IOException { OtelDataStream stream = create(name); List<Observation> observations = new ArrayList<>(); try { String field = "`" + scenario.field() + "`"; stream.rollover(scenario.driftFactory().build(name)); for (String source : List.of(name, name + "*")) { String resolved = winner(source, field); logger.info("pair {}, field {} via {} reads {}", scenario.pair(), field, source, resolved); for (Command command : REPRESENTATIVE) { String query = command.query(source, field); Response response = run(query); observations.add( new Observation( scenario.pair(), resolved, command.name(), source, query, response.status(), response.error())); } } } catch (IOException | RuntimeException e) { try { stream.delete(); } catch (IOException | RuntimeException cleanup) { e.addSuppressed(cleanup); } throw e; } stream.delete(); return observations; Possible Issue The baseValues method retrieves documents with size=100 but expects exactly BASE_DOCS (5) documents. If the stream somehow contains more than 5 documents in the base index (e.g., from a previous failed test run or concurrent test execution), the method will throw an exception. The query should limit results to BASE_DOCS to match the expectation, or the validation should check for at least BASE_DOCS documents and take the first 5. private static List<Object> baseValues(String streamName, String attribute) throws IOException { JSONObject body = new JSONObject( getResponseBody( client() .performRequest(new Request("GET", "/" + streamName + "/_search?size=100")))); JSONArray hits = body.getJSONObject("hits").getJSONArray("hits"); List<Object> values = new ArrayList<>(); for (int i = 0; i < hits.length(); i++) { values.add( hits.getJSONObject(i) .getJSONObject("_source") .getJSONObject("attributes") .get(attribute)); } if (values.size() != BASE_DOCS) { throw new IllegalStateException( streamName + " holds " + values.size() + " base documents, expected " + BASE_DOCS); } return values;

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 3a325f2

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix undefined method call The method calls executeRequest() which is not defined in this class or visible parent classes. Verify that this method exists in the inheritance chain, or replace it with the correct method like client().performRequest() to avoid runtime errors. integ-test/src/test/java/org/opensearch/sql/calcite/remote/OtelDataStreamConflictTestCase.java [78-85] private void requireOneNode() throws IOException { - JSONArray nodes = new JSONArray(executeRequest(new Request("GET", "/_cat/nodes?format=json"))); + JSONArray nodes = new JSONArray(getResponseBody(client().performRequest(new Request("GET", "/_cat/nodes?format=json")))); if (nodes.length() != 1) { throw new IllegalStateException( "The scenarios need one node to hold the merge order fixed, the cluster has " + nodes.length()); } } Suggestion importance[1-10]: 8 __ Why: The suggestion identifies a potentially critical issue where executeRequest() is called but not defined in the visible code. The proposed fix using getResponseBody(client().performRequest()) aligns with the pattern used elsewhere in the file (line 316-317), making this a valid and important correction.	Medium
General	Improve failure count reporting The failure message construction creates a TreeSet to deduplicate failures, but the count reports distinct.size() while the message shows all distinct failures. Consider reporting the original failure count alongside the distinct count for better diagnostics, or clarify if duplicates are expected. integ-test/src/test/java/org/opensearch/sql/calcite/remote/OtelDataStreamConflictTestCase.java [60-68] List<String> failures = new ArrayList<>(); failures.addAll(observed.unreadableWinners()); failures.addAll(observed.commandsThatNeverReturn200()); failures.addAll(observed.unlisted500s()); failures.addAll(observed.staleKnown500s()); if (!failures.isEmpty()) { Set<String> distinct = new TreeSet<>(failures); - fail(distinct.size() + " failures\n" + String.join("\n", distinct)); + fail(failures.size() + " failures (" + distinct.size() + " distinct)\n" + String.join("\n", distinct)); } Suggestion importance[1-10]: 5 __ Why: The suggestion improves diagnostic clarity by reporting both the original and distinct failure counts. This is a reasonable enhancement for debugging, though the current implementation is not incorrect—it just provides less information.	Low
	Clarify duplicate issue comments The duplicate comment for issue #5811 appears twice consecutively, which may cause confusion. Consider consolidating these two Known500 entries into a single entry with combined pairs, or add distinct comments explaining why they are separated (e.g., different winner values). integ-test/src/test/java/org/opensearch/sql/calcite/remote/MappingConflictFixtures.java [104-119] static final List<Known500> KNOWN_500S = List.of( // #5811 Composite aggregation pushdown does not support a field whose type differs across - // indices + // indices (any winner) new Known500( "#5811", List.of("stats_count_by", "dedup", "top", "rare", "chart_by", "timechart_by"), List.of("keyword/byte", "keyword/integer", "keyword/long"), null), // #5811 Composite aggregation pushdown does not support a field whose type differs across - // indices + // indices (string winner only) new Known500( "#5811", List.of("stats_count_by", "dedup", "top", "rare", "chart_by", "timechart_by"), List.of("keyword/date"), "string"), Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies duplicate comments for issue #5811, but the entries are intentionally separate due to different winner values (null vs "string"). The improved comments add clarity, though the issue is minor since the separation is functionally necessary.	Low

---

Previous suggestions

Suggestions up to commit 61726e3

Category	Suggestion	Impact
General	Use test assumption for node requirement The test silently fails when run on multi-node clusters, which is common in CI environments. Consider adding a test assumption or annotation to skip the test gracefully on multi-node setups instead of throwing an exception mid-execution. integ-test/src/test/java/org/opensearch/sql/calcite/remote/OtelDataStreamConflictTestCase.java [78-85] private void requireOneNode() throws IOException { JSONArray nodes = new JSONArray(executeRequest(new Request("GET", "/_cat/nodes?format=json"))); if (nodes.length() != 1) { - throw new IllegalStateException( - "The scenarios need one node to hold the merge order fixed, the cluster has " - + nodes.length()); + org.junit.Assume.assumeTrue( + "Skipping test: requires single-node cluster but found " + nodes.length() + " nodes", + false); } } Suggestion importance[1-10]: 7 __ Why: Using Assume.assumeTrue() is a better practice for test prerequisites than throwing IllegalStateException, as it properly marks the test as skipped rather than failed. This improves CI/CD behavior when tests run on multi-node clusters.	Medium
	Ensure document visibility before validation The validation assumes exactly BASE_DOCS documents exist, but search results may be inconsistent immediately after indexing. Add a refresh call before the search or implement retry logic to ensure documents are visible before validation. integ-test/src/test/java/org/opensearch/sql/calcite/remote/OtelDataStreamConflictTestCase.java [327-330] if (values.size() != BASE_DOCS) { + // Refresh the index to ensure all documents are visible + client().performRequest(new Request("POST", "/" + streamName + "/_refresh")); throw new IllegalStateException( streamName + " holds " + values.size() + " base documents, expected " + BASE_DOCS); } Suggestion importance[1-10]: 6 __ Why: The suggestion addresses a potential timing issue where documents may not be immediately visible after indexing. However, the improved code adds the refresh call only after the validation fails, which doesn't actually fix the issue. The refresh should occur before the search query at line 317.	Low
	Verify object type naming consistency The TYPES list includes "object" but the OTel schema typically uses "struct" for nested objects in PPL. Verify that "object" is the correct OpenSearch mapping type for the test scenarios, or consider using the PPL type name consistently. integ-test/src/test/java/org/opensearch/sql/calcite/remote/MappingConflictFixtures.java [18-19] +// Verify "object" matches the actual OpenSearch mapping type used in OTel data streams static final List<String> TYPES = List.of("text", "keyword", "byte", "integer", "long", "date", "object"); Suggestion importance[1-10]: 5 __ Why: The suggestion asks to verify that "object" is the correct type name, which is reasonable given the context mentions PPL uses "struct". However, this is a verification request rather than identifying a concrete bug, and the code appears intentional given line 139 explicitly expects "struct" as the winner for object fields.	Low

Suggestions up to commit 4f988ec

Category	Suggestion	Impact
Possible issue	Fix undefined method call The method calls executeRequest() which is not defined in this class or its parent PPLIntegTestCase. This will cause a compilation error. Use client().performRequest() instead, which is the standard pattern used elsewhere in the test. integ-test/src/test/java/org/opensearch/sql/calcite/remote/OtelDataStreamConflictTestCase.java [81-88] private void requireOneNode() throws IOException { - JSONArray nodes = new JSONArray(executeRequest(new Request("GET", "/_cat/nodes?format=json"))); + JSONArray nodes = new JSONArray(getResponseBody(client().performRequest(new Request("GET", "/_cat/nodes?format=json")))); if (nodes.length() != 1) { throw new IllegalStateException( "The scenarios need one node to hold the merge order fixed, the cluster has " + nodes.length()); } } Suggestion importance[1-10]: 10 __ Why: This identifies a critical compilation error. The method executeRequest() is not defined in the class or parent, and the suggested fix using getResponseBody(client().performRequest(...)) matches the pattern used elsewhere in the file (line 318-320), making this a necessary correction.	High
General	Clarify duplicate issue comments The duplicate comment for issue #5811 creates confusion about which entry addresses which specific type combinations. Each entry should have a distinct comment explaining why it differs from the other #5811 entry, particularly noting the winner constraint difference. integ-test/src/test/java/org/opensearch/sql/calcite/remote/MappingConflictFixtures.java [105-120] static final List<Known500> KNOWN_500S = List.of( // #5811 Composite aggregation pushdown does not support a field whose type differs across - // indices + // indices (numeric types with any winner) new Known500( "#5811", List.of("stats_count_by", "dedup", "top", "rare", "chart_by", "timechart_by"), List.of("keyword/byte", "keyword/integer", "keyword/long"), null), // #5811 Composite aggregation pushdown does not support a field whose type differs across - // indices + // indices (date type when winner is string) new Known500( "#5811", List.of("stats_count_by", "dedup", "top", "rare", "chart_by", "timechart_by"), List.of("keyword/date"), "string"), Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies duplicate comments for issue #5811, but the improvement is minor. Adding clarifying text helps readability but doesn't fix a functional issue. The distinction between entries is already clear from the different pairs and winner parameters.	Low
	Improve command validation logic This check may produce false positives if a command legitimately returns only non-200 responses across all scenarios. Consider adding a threshold or logging to distinguish between commands that should return 200 but don't versus commands that are expected to fail in all tested scenarios. integ-test/src/test/java/org/opensearch/sql/calcite/remote/OtelDataStreamConflictTestCase.java [224-230] List<String> commandsThatNeverReturn200() { return REPRESENTATIVE.stream() .map(Command::name) - .filter(n -> all.stream().noneMatch(o -> o.command().equals(n) && o.status() == 200)) + .filter(n -> { + long totalRuns = all.stream().filter(o -> o.command().equals(n)).count(); + long successRuns = all.stream().filter(o -> o.command().equals(n) && o.status() == 200).count(); + return totalRuns > 0 && successRuns == 0; + }) .map(n -> n + " never returned 200, so it tests nothing") .toList(); } Suggestion importance[1-10]: 3 __ Why: The suggestion adds redundant logic. The existing filter already checks if no observations match the command with status 200, which implicitly means the command ran but never succeeded. The added totalRuns > 0 check is unnecessary since if no observations exist for a command, the noneMatch would be vacuously true anyway.	Low

Suggestions up to commit c349f83

Category	Suggestion	Impact
General	Handle cleanup exceptions consistently The stream deletion in the happy path occurs outside the try-catch block, meaning if it throws an exception, it won't be caught and the test will fail unexpectedly. Wrap the final deletion in a try-catch or use try-with-resources to ensure consistent cleanup error handling. integ-test/src/test/java/org/opensearch/sql/calcite/remote/OtelDataStreamConflictTestCase.java [204-213] } catch (IOException | RuntimeException e) { try { stream.delete(); } catch (IOException | RuntimeException cleanup) { e.addSuppressed(cleanup); } throw e; } -stream.delete(); +try { + stream.delete(); +} catch (IOException | RuntimeException e) { + // Log or handle cleanup failure if needed +} Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that stream.delete() on line 212 could throw an exception that isn't caught, unlike the exception handler above. Adding consistent error handling for cleanup operations improves robustness, though the impact is moderate since cleanup failures in tests are typically non-critical.	Low
	Fix misleading error message The error message mentions "swap" but the code doesn't perform any swap operation. This misleading message could confuse developers debugging test failures. Update the message to accurately reflect that the test requires a single-node cluster to ensure deterministic index merge ordering. integ-test/src/test/java/org/opensearch/sql/calcite/remote/OtelDataStreamConflictTestCase.java [59-62] JSONArray nodes = new JSONArray(executeRequest(new Request("GET", "/_cat/nodes?format=json"))); if (nodes.length() != 1) { throw new IllegalStateException( - "The swap needs one node to fix the merge order, the cluster has " + nodes.length()); + "The test needs one node to fix the merge order, the cluster has " + nodes.length()); } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that the error message mentions "swap" which is misleading. However, this is a minor documentation/clarity issue that doesn't affect functionality. The improved message "The test needs one node" is clearer than "The swap needs one node".	Low
	Fail fast on schema read errors The code collects failures from cells with unreadable winners but continues processing all cells afterward, potentially masking critical schema reading failures. Consider failing fast when winner reading fails, as subsequent test logic depends on valid winner values for accurate pin matching. integ-test/src/test/java/org/opensearch/sql/calcite/remote/OtelDataStreamConflictTestCase.java [91-96] -cells.stream() +List<Cell> unreadable = cells.stream() .filter(c -> c.winner().startsWith("unread-")) - .forEach( - c -> - failures.add( - c.pair() + " via " + c.source() + " could not read its winner, " + c.winner())); + .toList(); +if (!unreadable.isEmpty()) { + unreadable.forEach( + c -> + failures.add( + c.pair() + " via " + c.source() + " could not read its winner, " + c.winner())); + fail(failures.size() + " failures\n" + String.join("\n", failures)); +} Suggestion importance[1-10]: 3 __ Why: While the suggestion has merit about failing fast on unreadable winners, the current implementation collects all failures before reporting them, which provides more comprehensive test feedback. The suggested change would hide subsequent failures, which may not be desirable for test diagnostics.	Low

Suggestions up to commit 168150e

Category	Suggestion	Impact
General	Clarify error message terminology The error message mentions "swap" but the context is about fixing merge order for drift tests. The term "swap" is unclear and doesn't match the actual purpose. Update the error message to accurately describe why a single-node cluster is required for these tests. integ-test/src/test/java/org/opensearch/sql/calcite/remote/OtelDataStreamConflictTestCase.java [120-123] JSONArray nodes = new JSONArray(executeRequest(new Request("GET", "/_cat/nodes?format=json"))); if (nodes.length() != 1) { throw new IllegalStateException( - "The swap needs one node to fix the merge order, the cluster has " + nodes.length()); + "These tests need one node to fix the merge order, the cluster has " + nodes.length()); } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that "swap" is unclear terminology in the error message. However, this is a minor improvement to error message clarity that doesn't affect functionality or correctness.	Low
	Prevent duplicate failure reporting The code filters cells with unreadable winners and adds them to failures, but this happens before checking for 500 errors. If a cell has both an unreadable winner and a 500 status, it will be reported twice. Consider consolidating the failure reporting or ensuring cells are only reported once. integ-test/src/test/java/org/opensearch/sql/calcite/remote/OtelDataStreamConflictTestCase.java [152-157] -cells.stream() +Set<String> unreadableWinners = cells.stream() .filter(c -> c.winner().startsWith("unread-")) - .forEach( - c -> - failures.add( - c.pair() + " via " + c.source() + " could not read its winner, " + c.winner())); + .map(c -> c.pair() + " via " + c.source() + " could not read its winner, " + c.winner()) + .collect(Collectors.toSet()); +failures.addAll(unreadableWinners); Suggestion importance[1-10]: 3 __ Why: The suggestion addresses a potential duplicate reporting issue, but the improved_code doesn't actually prevent duplicates better than the original - it just collects to a Set before adding to failures, which is already a List. The concern about double-reporting is valid but the solution doesn't meaningfully improve the situation.	Low

Suggestions up to commit ebea19d

Category	Suggestion	Impact
General	Add validation for issue parameter The validation allows winner to be null (as seen in usage) but doesn't validate that issue is non-null or non-empty. Add validation for issue to prevent invalid Pin instances with missing issue identifiers. integ-test/src/test/java/org/opensearch/sql/calcite/remote/OtelDataStreamConflictTestCase.java [97-102] protected record Pin(String issue, List<String> commands, List<String> pairs, String winner) { protected Pin { + if (issue == null || issue.isEmpty()) { + throw new IllegalArgumentException("Pin requires a non-empty issue identifier"); + } if (commands.isEmpty() || pairs.isEmpty()) { throw new IllegalArgumentException("The " + issue + " pin needs commands and pairs"); } } Suggestion importance[1-10]: 7 __ Why: The issue parameter is used throughout the code (in error messages and logging) but lacks null/empty validation. Adding this check prevents potential NullPointerException or confusing error messages with empty issue identifiers.	Medium
	Improve error message clarity The error message doesn't indicate the expected number of documents, making debugging harder. Include BASE_DOCS in the message to show both expected and actual values for clearer diagnostics. integ-test/src/test/java/org/opensearch/sql/calcite/remote/OtelDataStreamConflictTestCase.java [301-303] if (values.size() != BASE_DOCS) { - throw new IllegalStateException(name + " holds " + values.size() + " base documents"); + throw new IllegalStateException(name + " holds " + values.size() + " base documents, expected " + BASE_DOCS); } Suggestion importance[1-10]: 6 __ Why: Adding the expected value (BASE_DOCS) to the error message improves debugging by showing both actual and expected document counts, making it easier to diagnose issues when this validation fails.	Low
	Fix misleading error message The error message mentions "swap" but the context is about fixing merge order for testing. This appears to be a copy-paste error or unclear messaging. Update the error message to accurately describe why a single-node cluster is required for these tests. integ-test/src/test/java/org/opensearch/sql/calcite/remote/OtelDataStreamConflictTestCase.java [120-123] JSONArray nodes = new JSONArray(executeRequest(new Request("GET", "/_cat/nodes?format=json"))); if (nodes.length() != 1) { throw new IllegalStateException( - "The swap needs one node to fix the merge order, the cluster has " + nodes.length()); + "These tests need one node to fix the merge order, the cluster has " + nodes.length()); } Suggestion importance[1-10]: 5 __ Why: The error message references "swap" which is unclear in context. Changing to "These tests" improves clarity, though the impact is minor since it only affects error messaging in exceptional cases.	Low

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.66%. Comparing base (803ed31) to head (3a325f2).
⚠️ Report is 8 commits behind head on main.

❌ Your project check has failed because the head coverage (63.66%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5839      +/-   ##
============================================
+ Coverage     63.36%   63.66%   +0.30%     
- Complexity     8872     9022     +150     
============================================
  Files           949      959      +10     
  Lines         40322    40836     +514     
  Branches       4546     4635      +89     
============================================
+ Hits          25549    26000     +451     
- Misses        13929    13964      +35     
- Partials        844      872      +28     

Flag	Coverage Δ
sql-engine	63.66% <ø> (+0.30%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 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.

[github-actions]

Persistent review updated to latest commit 168150e

[github-actions]

Persistent review updated to latest commit c349f83

[ahkcs]

A 4xx is supposed to pass, but if the winner probe gets a 4xx, the run fails. So a fix that returns 400 on conflicting types can never pass. Could a 4xx probe be allowed?

[cnoramut]

Fixed. A winner query that returns a 4xx on a type pair no longer fails the run, so a fix that rejects conflicting types with a 400 can pass. One that returns a 5xx, or a 200 with no type, still fails, and so does a 4xx on the scenarios where one side has no field, since nothing conflicts there. If a winner-scoped entry in KNOWN_500S loses its winner that way, the run asks for that entry to be removed.

[ahkcs]

Only 500s are checked, so a command that returns 4xx everywhere (say, from a typo) passes without testing anything. Could we assert each command gets at least one 200?

[cnoramut]

Agreed, and it is added. The run now fails when a command never returns 200 across its 112 queries, so a broken template can no longer pass by being rejected everywhere. A 200 that never reads the field still passes, as where f > f did once the planner folded it to an empty result. I can add an _explain check to this PR that fails any command whose plan never names the field.

[ahkcs]

Pins don't include the source (otel-logs-drift vs otel-logs-drift*). If a 500 happens on only one source, it can't be pinned. Could Pin take an optional source?

[cnoramut]

Added. Known500, renamed from Pin in penghuo's review, now takes an optional Source, meaning DATA_STREAM, WILDCARD or BOTH.

[penghuo]

runDrifts over 100 lines, hard for human to read.
my 2 cents

protected void runDrifts(String name) throws IOException {
  List<Scenario> scenarios = Scenario.allDrifts(TYPES);  
  Observations observed = observe(name, scenarios); 
  ....
}

And many new conept in this PR, but not explain what is it, e.g. Cell, it has specific meaning in CS, you would better find a better word.

[penghuo]

Any reason mappingConflictIntegTest not part of integTest?

[cnoramut]

Yes, because A stats ... by over a field that is keyword in one backing index and a number or date in the other, the #5811 case, trips the InternalComposite$InternalBucket.compareKey assert in OpenSearch core. The integTest node runs with assertions on, so the first such query kills the node and fails every IT after it. This task runs them on their own node with only that assert off, as production runs. It follows tracingIntegTest, which also runs on its own cluster beside integTest.

[penghuo]

Avoid using System.out.println, it is not controlled by testLogging { events "passed", "skipped", "failed" }

[cnoramut]

Fixed, the three System.out.println calls now go through the logger inherited from OpenSearchTestCase.

[penghuo]

nit: why test releated to node count?

[cnoramut]

Fixed with a Javadoc on requireOneNode. The merged schema keeps the type of the backing index it iterates last, in an order set by the index names and a salt each JVM draws at start. Every scenario recreates the same two names, so on one node the two scenarios of a pair let each type win once. On several nodes each node has its own order and requests spread across them, so a query's winner can differ from the one its probe read.

[penghuo]

How to support future supported TYPES, e.g. flat_object, #5830

[cnoramut]

A type the fixture already holds is one entry in TYPES, and the run then pairs it with every other type. flat_object is not among the 22 types the fixture holds under attributes.types, so it also needs a field and base values in both data files and a mapping in both templates, since dynamic mapping never produces flat_object.

I tried it on main with #5830. Against keyword, the numbers and date it returns only 200 or 400, and it adds 24 keys returning 500 when it stands alone or wins against text or object, all from does not support doc_value in root field on a sort or aggregation pushed to the flat_object root. I can add it in this or a follow-up PR, together with an issue for that 500.

[penghuo]

what is the meaing of Pin from OO design perspective?

[cnoramut]

Renamed to Known500, a group of queries that return a 500 today, tied to the issue that tracks them. The list keeps its name, KNOWN_500S, which now matches the type.

[penghuo]

what is the meaning of control?

[cnoramut]

Removed. It only fed a second field to five templates, such as chart count() over {K} by {F}, and the run gives the same 500s without it, so those templates now read only the field under test.

[penghuo]

what is the defition of Cell from OO perpective?

[cnoramut]

Renamed to Observation, the outcome of one query, meaning one command over one source under one winner. The checks over all of them now live on an Observations record.

[penghuo]

pass() is confusing, it just run query, but did not pass any test.

[cnoramut]

Agreed, it is now observeScenario, called from observe(name, scenarios), which runs every scenario and returns the Observations the named checks read.

[github-actions]

Persistent review updated to latest commit 4f988ec

[github-actions]

Persistent review updated to latest commit 61726e3

[cnoramut]

Thank you, @penghuo, I appreciate the review. All nine comments are addressed, with an answer on each thread.

• runDrifts now follows your sketch. Observations observed = observe(name, Scenario.allDrifts(TYPES)) runs every scenario, logSummary logs the counts, and four named checks on Observations collect the failures.
• Cell, Pin and pass() are renamed to Observation, Known500 and observeScenario, and control is removed.
• Drift and pair are defined on Scenario, and winner on Observation.
• A planted run that fires every failure message gave byte-identical output before and after the split, so the restructuring changes no behaviour.

[github-actions]

Persistent review updated to latest commit 3a325f2