Conversation
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Reviewed e4b2e758 against authoritative base 2d1aab3a. The existing suite checks query assertion modes and record association. This PR adds four tests for the metadata and statement records that the shared parser supplies to the fixture runner. The authored change is 75 added lines in SqlFileTestParserSuite.scala. The base contains a separate shuffle fix that is absent from the branch tip, but the verified merge retains that fix and adds exactly this test change.
The new assertions check the complete parsed values rather than merely checking that parsing succeeds. Empty and comment-only inputs must produce the empty model, including absent version bounds. Interleaved Config and ConfigMatrix directives must preserve order within their respective sequences and trim both keys and values. The minimum-only, maximum-only and combined cases independently check both version fields, so assigning one bound to the other field would fail.
The mixed fixture follows the documented blank-line record format. It checks both whitespace-only and empty separators, preserves multiline SQL and indentation, and asserts the SQL start lines as 4, 10, 13 and 16. Exact record equality also catches missing or reordered statements and an incorrect default query mode. Uppercase and lowercase CREATE TABLE statements supply exactly two cleanup names, while the insert contributes no table. The final query is checked at EOF and with a trailing blank line.
I traced these fields through CometSqlFileTestSuite: configuration matrices generate combinations, version bounds control skipping, source lines appear in diagnostics, and the table sequence goes to withTable. The maintained Spark 3.5 and 4.0 helpers drop those named tables after the body. This changes no Spark expression, operator, ANSI, timezone or fallback semantics. The tests exercise the in-memory parse(Seq[String]) path. They do not claim file-decoding coverage, actual table cleanup, matrix execution, version filtering, or rejection of malformed directives. Those limits do not undermine the assertions added here. I found no verified P1/P2 issue.
Validation and CI
At September 15, 14:09 UTC, 41 checks succeeded and 11 were skipped, with none failed or pending. I read the original Spark 3.5 expression-job log and Spark 4.1 expression-job log. Both explicitly ran all 12 parser tests, including the four additions, and completed successfully on JDK 17.
Both jobs checked out b1943707, whose parents are exactly the assigned base and head. The changed test, shared parser, runner and inspected CI configuration match the reviewed head. The separate upstream Spark SQL, Iceberg, macOS, benchmark and site jobs were skipped and receive no execution credit. The relevant parser tests ran in the Linux expression jobs. No local build or test run was needed for this review. Maintained Spark 3.4/4.1 source branches were unavailable, so their source-level semantic coverage is not claimed.
Performance
The additions parse short in-memory sequences in an AnyFunSuite, without starting a Spark session or writing fixtures to disk. Their cost is confined to the test suite. There is no change to production parsing, execution, allocation or shuffle behavior from the authored patch, and no performance claim requiring a microbenchmark.
The loops reuse the expected structure for the two record endings and three version-bound cases. They add a small, bounded amount of work and avoid expensive setup for assertions that only need parsed values.
Design
Direct unit tests are appropriate here because the contract being checked is the parser's output model. Running SQL fixtures through Spark would obscure metadata and line-number errors behind fixture execution. The four cases separate empty input, configuration metadata, version metadata and record collection while using the actual shared parser throughout.
The tests preserve the existing format and permissive parsing behavior. They introduce no new syntax or interpretation. Their expectations match how the runner consumes the fields, and the base-to-merge comparison confirms that they integrate without changing the newer shuffle fix.
Abstraction & complexity
The patch uses the existing case classes and ordinary ScalaTest assertions. It adds no parser copy, mock, helper framework or dependency. The small input loops remove repetition while leaving each expected record and metadata value explicit. I found no actionable simplification or additional concern.
Which issue does this PR close?
No linked issue; this adds test coverage for the existing SQL fixture parser.
Rationale for this change
The parser tests cover query assertion modes but do not directly check the configuration and version metadata that control fixture execution, or the statement records and table names used for diagnostics and cleanup.
What changes are included in this PR?
Add four tests for empty/comment-only fixtures, configuration ordering and whitespace, independent Spark version bounds, and mixed statement/query records. Check multiline SQL, source line numbers, table extraction, and the final record with and without a trailing blank line.
How are these changes tested?
All 12 tests in
SqlFileTestParserSuitepass locally on Spark 3.5.9 and 4.1.3 with JDK 17, including the four new tests.make format(Spark 3.5) and Apache RAT passed.