Skip to content

test: restore Comet coverage for recursive HAVING and ORDER BY - #5755

Merged
comphead merged 1 commit into
apache:mainfrom
rich7420:test/5569-restore-having-order-by
Sep 8, 2026
Merged

test: restore Comet coverage for recursive HAVING and ORDER BY#5755
comphead merged 1 commit into
apache:mainfrom
rich7420:test/5569-restore-having-order-by

Conversation

@rich7420

@rich7420 rich7420 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #5569 (HAVING / ORDER BY SQL coverage).

Rationale for this change

The Spark 4.1.3 patch disables Comet for the entire having-and-order-by-recursive-type-name-resolution.sql file, citing #4123. The map-containing grouping-key fallback was fixed by #4157, so restore coverage with Comet enabled.

What changes are included in this PR?

Remove the file-wide spark.comet.enabled = false directive and its stale TODO. Regenerate dev/diffs/4.1.3.diff from Spark v4.1.3 sources without changing queries or golden results.

Regeneration also refreshes the existing pom.xml result hash without changing its patch content.

How are these changes tested?

The regenerated patch applies cleanly to Spark v4.1.3. Comparing the resulting source trees confirms that only the target SQL input changes, and it matches the original Spark file exactly. git diff --check passes, and the CI path selector enables Spark 4.1 tests.

The Spark 4.1.3 golden file contains 44 query blocks. Fork CI passed all seven Spark 4.1 test groups. The logs confirm that both having-and-order-by-recursive-type-name-resolution.sql and its _analyzer_test variant ran and passed.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Correctness

Reviewed 99256586 against authoritative base bc74cc79. The Spark 4.1.3 patch still forced Comet off for having-and-order-by-recursive-type-name-resolution.sql, citing the now-closed #4123. This removes that file-wide override and its explanation, allowing the existing tests to inherit the suite's Comet configuration. It restores regression coverage after the grouping-key fallback fix without changing queries or golden expectations.

The underlying protection is already present in the base. CometBaseAggregate.doConvert refuses grouping expressions whose types recursively contain MapType, and the aggregate compatibility precheck uses the same predicate. SupportLevel.containsType traverses structs and arrays, so the reported Struct(Map(...)) case is covered before native aggregate serialization. Existing CometAggregateSuite regressions check the fallback reason and, for the struct case, the absence of a native aggregate. Those tests are unchanged. Their source supports the intended fallback behavior, but I did not rerun them locally or infer their execution from this PR's SQL-test jobs.

The maintained Spark 3.5 and 4.0 test runners interpret --SET comments as per-file configuration. The maintained 4.0 runner separately handles execution and analyzer cases, applies settings to a new session, and compares schemas and normalized outputs with golden files. Its execution helper preserves order for semantically sorted queries. In the Comet 4.1.3 patch, the shared-session setup enables the extension, execution and shuffle when ENABLE_COMET=true. The remaining ignore-list entries do not match this filename. The PR changes no query, ANSI setting, null/overflow rule, error expectation or runner assertion.

Validation and CI

Local structural checks validate both unified patches and confirm that all 96 remaining sections are identical after excluding index metadata. Only the existing POM result hash changes within those sections. The repository's path selector selects Spark 4.1 for this file, and git diff --check passes. No local Spark/JNI rebuild or full Spark patch application was performed.

Apache CI tested merge 19782739, whose parents are exactly the assigned base and head. Its tree equals the head. The build log records Spark 4.1.3 at 77bbf77e, successful application of this patch, and compiled native/JVM artifacts whose digests match those downloaded by the test jobs. The Spark 4.1.3 core-2 log confirms that the execution test and _analyzer_test variant both ran and passed under the ExtendedSQLTest filter. The invocation sets ENABLE_COMET=true. At 2026-09-08T02:37:35.686Z, checks report 14 successful and 12 skipped, with no failure or unfinished check. All seven Spark 4.1 test groups passed.

These results establish execution of the restored golden-result coverage with Comet enabled. They do not establish native execution for every query or assert the exact fallback plan for each query. The analyzer variant adds analysis coverage, not native runtime evidence. Maintained Spark 3.4/4.1 source refs are unavailable, and the exact 4.1 query corpus was not independently inspected from a maintained branch. The description's 44-block count remains an author report. No P1/P2 correctness finding within this test-restoration scope.

Performance

This changes the configuration of an existing SQL test file and introduces no production scan, allocation, copy or parser overhead. The execution test took 1.221 seconds and its analyzer variant 73 milliseconds in the inspected CI run. Those are test durations without a matched baseline, not a Comet speedup measurement. A query microbenchmark would not validate removal of this stale test override, and no material performance concern was found.

Design

Removing the obsolete file-wide override lets the existing operator-level fallback handle unsupported map-containing grouping keys while retaining the file's other compatibility checks. The change uses the established versioned Spark patch mechanism and preserves all assertions. It appropriately addresses this part of #5569 without claiming to close the entire cleanup issue or implement native sorting of map-containing keys. No additional design change is needed for this PR.

Abstraction & complexity

The patch removes a special-case configuration header and adds no helper, annotation, dependency or execution path. The unchanged recursive type predicate and focused aggregate regressions already provide the necessary fallback mechanism. Keeping the upstream SQL and expected results intact avoids maintaining a duplicate test corpus. No actionable complexity concern. Approving this revision.

@comphead

comphead commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Thanks @rich7420 and @sunchao for the review, lets give it a try

@comphead
comphead merged commit 6368c4e into apache:main Sep 8, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants