-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Automate sqllogictest for String, LargeString and StringView behavior #12525
Conversation
create table test_source as values | ||
('Andrew', 'X', 'datafusion📊🔥', '🔥'), | ||
('Xiangpeng', 'Xiangpeng', 'datafusion数据融合', 'datafusion数据融合'), | ||
('Raphael', 'R', 'datafusionДатаФусион', 'аФус'), | ||
(NULL, 'R', NULL, '🔥'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we started to implement some ASCII-faster paths, I think we should always consider testing both ASCII-only and Unicode cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much @goldmedal
I really like this structured, and I think the tests are super well commented and easy to follow. Really nice ❤️ 🏆
@2010YOUY01 do you perhaps have time to take a look at this PR as well?
cc @tshauck
drop table test_substr_base; | ||
|
||
|
||
# -------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is well organized 👍🏼
I have one minor suggestion: we should add a separate README under test_files/string/
to explain the structure
Nice idea! I have added the README for them. Thanks for the suggestion 🙇 |
THANK YOU SO MUCH @goldmedal and @2010YOUY01 -- this looks really great |
Thanks @alamb @2010YOUY01 🙇♂️ |
Which issue does this PR close?
Part of #12415
Rationale for this change
I am implementing a testing framework for string types to ensure consistent behavior across different string types. I have extracted test cases from
datafusion/sqllogictest/test_files/string_view.slt
, which contains many good tests for string types.Currently, I haven’t moved all the tests from
string_view.slt
because I don’t want to make this PR too large (I want to confirm if I’m on the right way 🤔). For now, I’ve only moved the following case:Some TODO items should be finished in the follow-up PR
string_view.slt
LIKE/ILIKE
SUBSTR
planing||
operator~
operator (regex match)~*
operator (regex match case-insensitive)!~~
operator (not like match)!~~*
operator (not like match case-insensitive)string/string_view.slt
function.slt
tostring/string_literal.rs
What changes are included in this PR?
Refer to #12433, I made some changes. I regroup the test case by the data type. The file structure is
Any entry point file should include
init_data.slt.part
andstring_query.slt.part
.For planning tests for
StringView
(e.g.EXPLAIN ...
), I moved to thestring/string_view.slt
because it's only used to assert the planning behavior ofStringView
.Are these changes tested?
Refactor and enhance the original tests.
Are there any user-facing changes?
no