Allow testing records with sibling whitespace in SLT tests and add more string tests by findepi · Pull Request #13197 · apache/datafusion

findepi · 2024-10-31T08:47:25Z

motivation: Simplify EXPR LIKE 'constant' to expr = 'constant' #13061 (comment)
relates to Incorrect LIKE and ILIKE result for NULL input, `%', and '%%' pattern #12637

datafusion/sqllogictest/test_files/string/string_query.slt.part

findepi · 2024-10-31T14:51:23Z

@alamb @comphead ready for review

comphead

Thanks @findepi why tests have different results, excluding formatting

comphead · 2024-10-31T16:08:08Z

datafusion/sqllogictest/bin/sqllogictests.rs

+        .collect::<Vec<_>>();
+    let actual = actual
+        .iter()
+        .map(|strs| strs.iter().join(" "))


we prob can do this in a sinlge map?

I am not quite sure what you are suggesting here

I'm suggesting to have a single collection iteration instead of two

.map(|strs| strs.iter().join(" ").trim_end().to_owned())

yes, we can do this in single map.
i placed this into two map() steps so that the common part of actual and expected processing looks the same and so that i can write a nice comment

alamb

Thanks @findepi and @comphead - the code looks good to me. I have a suggestion about improving the docs but we could do that as a follow on PR too

alamb · 2024-10-31T17:29:35Z

datafusion/sqllogictest/bin/sqllogictests.rs

+        .collect::<Vec<_>>();
+    let actual = actual
+        .iter()
+        .map(|strs| strs.iter().join(" "))


I am not quite sure what you are suggesting here

alamb · 2024-10-31T17:30:37Z

datafusion/sqllogictest/bin/sqllogictests.rs

+    let expected = expected
+        .iter()
+        // Trailing whitespace from lines in SLT will typically be removed, but do not fail if it is not
+        // If particular test wants to cover trailing whitespace on a value,


This is probably a good thing to add in the user documentation (aka https://github.com/apache/datafusion/blob/main/datafusion/sqllogictest/README.md)

I think it would be hard to find here for someone writing sqllogictests

it feels so edge-case'y. but yes, we can add

I will make a follow on PR

Follow on PR for documentation: Minor: Document how to test for trailing whitespace in slt / sqllogictests #13215

findepi · 2024-10-31T21:38:25Z

Thanks @findepi why tests have different results, excluding formatting

@comphead please see https://github.com/apache/datafusion/pull/13197/commits
i separated commits to make it more visible. Some changes (formatting) are because of the first change, which adds support for trailing whitespace.
The other changes (new rows) are bcause i added more data to string test cases.
i could separate into two PRs, but then it would not be clear why i do the first change in the first place. it's just supportive for the second one.

alamb · 2024-11-01T16:23:26Z

Thanks @findepi and @comphead

findepi marked this pull request as draft October 31, 2024 08:47

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Oct 31, 2024

findepi mentioned this pull request Oct 31, 2024

Fix rendering of dictionary empty string values in SLT tests #13198

Merged

This comment was marked as resolved.

Sign in to view

findepi commented Oct 31, 2024

View reviewed changes

datafusion/sqllogictest/test_files/string/string_query.slt.part Show resolved Hide resolved

findepi force-pushed the findepi/add-empty-string-to-string-test-data-5c1dbd branch 2 times, most recently from 7d41bee to e64d6d0 Compare October 31, 2024 14:43

findepi changed the title ~~Add empty string to string test data~~ Allow testing records with sibling whitespace in SLT tests and add more LIKE predicate tests Oct 31, 2024

findepi changed the title ~~Allow testing records with sibling whitespace in SLT tests and add more LIKE predicate tests~~ Allow testing records with sibling whitespace in SLT tests and add more string tests Oct 31, 2024

findepi marked this pull request as ready for review October 31, 2024 14:45

findepi force-pushed the findepi/add-empty-string-to-string-test-data-5c1dbd branch 2 times, most recently from d4ea9ef to 1a3ac85 Compare October 31, 2024 14:51

findepi added 4 commits October 31, 2024 16:34

empty

02cf32e

Allow testing values with trailing whitespace in SLT tests

0d5abd6

Update SLT tests for "Allow testing values with trailing whitespace ..."

1f98c54

Add empty string to string test data

fa96eb3

findepi force-pushed the findepi/add-empty-string-to-string-test-data-5c1dbd branch from 1a3ac85 to fa96eb3 Compare October 31, 2024 15:35

comphead reviewed Oct 31, 2024

View reviewed changes

alamb approved these changes Oct 31, 2024

View reviewed changes

comphead approved these changes Oct 31, 2024

View reviewed changes

alamb merged commit 5e53b63 into apache:main Nov 1, 2024

alamb mentioned this pull request Nov 1, 2024

Minor: Document how to test for trailing whitespace in slt / sqllogictests #13215

Merged

findepi deleted the findepi/add-empty-string-to-string-test-data-5c1dbd branch November 1, 2024 19:48

Conversation

findepi commented Oct 31, 2024

Uh oh!

This comment was marked as resolved.

Uh oh!

findepi commented Oct 31, 2024

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi commented Oct 31, 2024

Uh oh!

alamb commented Nov 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alamb Nov 1, 2024 •

edited

Loading