Skip to content

Conversation

@2010YOUY01
Copy link
Contributor

@2010YOUY01 2010YOUY01 commented Nov 21, 2025

Which issue does this PR close?

Part of #17612

Rationale for this change

sqllogictests are in general easier to maintain than rust tests, however it's not able to test EXPLAIN ANALYZE results, because their results include changing part:

(in datafusion-cli) The elapsed_compute measurement changes from run to run.

> EXPLAIN ANALYZE SELECT * FROM generate_series(100);
+-------------------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| plan_type         | plan                                                                                                                                                                                             |
+-------------------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Plan with Metrics | LazyMemoryExec: partitions=1, batch_generators=[generate_series: start=0, end=100, batch_size=8192], metrics=[output_rows=101, elapsed_compute=74.042µs, output_bytes=64.0 KB, output_batches=1] |
|                   |                                                                                                                                                                                                  |
+-------------------+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row(s) fetched.
Elapsed 0.006 seconds.

We can add a special marker to sqllogictest to skip those non-deterministic parts.

What changes are included in this PR?

  • Changed sqllogictest validator to recognize <slt:ignore> marker
  • doc
  • slt test

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Nov 21, 2025
if let Some(idx) = actual_snapshot[pos..].find(frag) {
pos += idx + frag.len();
} else {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

This early return would make it hard to debug. It would be nice to log a warning with the reason, as done below (line 116+).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I don't know what those log are used for. I think the error message is generated elsewhere, and I want to make sure they're developer-friendly.

It looks okay now, but with false positives

2. query result mismatch:
[SQL] select * from generate_series(3);
[Diff] (-expected|+actual)
-   <slt:ignore>
+   0
    1
-   3<slt:ignore>
+   2
    3
at /Users/yongting/Code/datafusion/datafusion/sqllogictest/test_files/slt_features.slt:84

I have filed #18878 for improvement.

continue;
}
if let Some(idx) = actual_snapshot[pos..].find(frag) {
pos += idx + frag.len();
Copy link
Member

@martin-g martin-g Nov 21, 2025

Choose a reason for hiding this comment

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

I think this logic won't detect a prefix in the actual_snapshot.
Let's say the first expected fragment is "Hello" but the actual_snapshot starts with "Anything here before Hello". The find() will set the position to the correct index but Anything here before will be ignored and assumed as matching.
Similar issue with a suffix in actual_snapshot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good catch. I fixed it in a3154e7 and added more tests.

2010YOUY01 and others added 2 commits November 22, 2025 12:25
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
@2010YOUY01 2010YOUY01 added this pull request to the merge queue Nov 23, 2025
Merged via the queue into apache:main with commit fbe1ae6 Nov 23, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants