Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sqllogictests --complete does not escape ( in error messages properly #5727

Closed
alamb opened this issue Mar 24, 2023 · 6 comments
Closed

sqllogictests --complete does not escape ( in error messages properly #5727

alamb opened this issue Mar 24, 2023 · 6 comments
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@alamb
Copy link
Contributor

alamb commented Mar 24, 2023

Describe the bug

sqllogictest automatically verifies error messages, including with regular expression matching

read more about sqllogictest here: https://github.com/apache/arrow-datafusion/tree/main/datafusion/core/tests/sqllogictests

Because it supports regular expression matching, to match an error like this (note ( and )):

query error Error during planning: arrow_cast requires its second argument to be a constant string, got Int64(43)

You need to \( and \):

query error Error during planning: arrow_cast requires its second argument to be a constant string, got Int64\(43\)

sqllogictests --complete mode is is awesome and will update the expected output, however for error messages that contain parenthesis it does not escape the parenthesis

This means that output captured with --complete that contains errors with ( and ) will not pass

To Reproduce

Apply this diff (

--- a/datafusion/core/tests/sqllogictests/test_files/arrow_typeof.slt
+++ b/datafusion/core/tests/sqllogictests/test_files/arrow_typeof.slt
@@ -95,7 +95,7 @@ SELECT arrow_cast('1', 'Int16')
 query error Error during planning: arrow_cast needs 2 arguments, 1 provided
 SELECT arrow_cast('1')
 
-query error Error during planning: arrow_cast requires its second argument to be a constant string, got Int64\(43\)
+query error TBD
 SELECT arrow_cast('1', 43)
 
 query error Error unrecognized word: unknown

Then run the sqllogictest complete mode:

cargo test --test sqllogictests -- arrow_typeof --complete

Then run sqllogictest:

cargo test --test sqllogictests -- arrow_typeof

It fails with a very hard to understand error (the reported expected and actual are the same 🤯 ):

Error: query is expected to fail with error:
	DataFusion error: Error during planning: arrow_cast requires its second argument to be a constant string, got Int64(43)
but got error:
	DataFusion error: Error during planning: arrow_cast requires its second argument to be a constant string, got Int64(43)
[SQL] SELECT arrow_cast('1', 43)
at tests/sqllogictests/test_files/arrow_typeof.slt:98

Expected behavior

I expect sqllogictest to pass after running --complete mode:

cargo test --test sqllogictests -- arrow_typeof

Additional context

@tustvold hit this while working on #5685

The symptom was the following very non clear message

Screenshot 2023-03-24 at 3 10 53 PM
image

@alamb alamb added bug Something isn't working good first issue Good for newcomers labels Mar 24, 2023
@alamb
Copy link
Contributor Author

alamb commented Mar 24, 2023

I strongly suspect this needs to be fixed upstream in https://github.com/risinglightdb/sqllogictest-rs @xxchan does this sound familiar to you at all?

@alamb alamb added the help wanted Extra attention is needed label Mar 24, 2023
@alamb
Copy link
Contributor Author

alamb commented Mar 24, 2023

cc @melgenek

@melgenek
Copy link
Contributor

The completed error should probably be wrapped in the regex::escape here and here.

@melgenek
Copy link
Contributor

melgenek commented Mar 24, 2023

I've opened a pr in the sqllogictest-rs risinglightdb/sqllogictest-rs#174.
It does effectively the same that I suggested above, but performs escaping when building a regex, rather than writing it.

@alamb
Copy link
Contributor Author

alamb commented Mar 25, 2023

I've opened a pr in the sqllogictest-rs risinglightdb/sqllogictest-rs#174.

Thank you so much @melgenek !

@alamb
Copy link
Contributor Author

alamb commented Mar 25, 2023

I just tested locally (after running cargo update in datafusion to update to https://crates.io/crates/sqllogictest/0.13.2) and this works great. What a wonderful way to start a Saturady.

🙇 @melgenek and @xxchan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants