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

Reorgnize the StringView tests in sqllogictests #12572

Merged
merged 3 commits into from
Sep 22, 2024

Conversation

goldmedal
Copy link
Contributor

@goldmedal goldmedal commented Sep 21, 2024

Which issue does this PR close?

Part of #12415

Rationale for this change

It's the follow-up PR of #12525. The main purpose of this PR is to remove the original string_view.slt. Some string function tests won't be covered by this PR.
See #12415 (comment) for the remaining works.

What changes are included in this PR?

Move the function tests to string/string_query.slt.part and remove the string_view.slt.

Are these changes tested?

It's a refactor for string tests.

Are there any user-facing changes?

no

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Sep 21, 2024
Comment on lines +972 to +981
################################################
# Test for Dictionary String concatenation
################################################

# || same type (column1 has null, so also tests NULL || NULL)
# expect all results to be the same for each row as they all have the same values
query T
SELECT
column1_dict || column1_dict
FROM test;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to create a dictionary_stirng.slt for them. However, I found that the output type of Dictionary String differs from the normal string. It leads the schema assertion to fail.

External error: query columns mismatch:
[SQL] SELECT ascii_1, ascii_2, unicode_1, unicode_2 FROM test_basic_operator
[Expected] [T][T][T][T]
[Actual  ] [?][?][?][?]
at test_files/string/./string_query.slt.part:23
at test_files/string/dictionary_string.slt:40

I think we should avoid returning the testing data directly to avoid this error. Maybe we can do it in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think this would be a good thing to include in a separate PR -- it is strange to have the Dictionary tests in a file called string_view.slt

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you very much @goldmedal

@@ -352,5 +352,651 @@ logical_plan
statement ok
drop table test_lowercase

# Ensure string functions use native StringView implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

EPIC

Comment on lines +972 to +981
################################################
# Test for Dictionary String concatenation
################################################

# || same type (column1 has null, so also tests NULL || NULL)
# expect all results to be the same for each row as they all have the same values
query T
SELECT
column1_dict || column1_dict
FROM test;
Copy link
Contributor

Choose a reason for hiding this comment

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

I do think this would be a good thing to include in a separate PR -- it is strange to have the Dictionary tests in a file called string_view.slt

@alamb
Copy link
Contributor

alamb commented Sep 22, 2024

🚀

@alamb alamb merged commit 7dc0151 into apache:main Sep 22, 2024
25 checks passed
@alamb
Copy link
Contributor

alamb commented Sep 22, 2024

Thanks again @goldmedal -- this is a really important help to the project for testing

@goldmedal
Copy link
Contributor Author

Thanks @alamb 👍

@goldmedal goldmedal deleted the chore/12415-movine-test-1 branch September 23, 2024 01:59
bgjackma pushed a commit to bgjackma/datafusion that referenced this pull request Sep 25, 2024
* move the string function tests

* move the operator test

* add back the test for dictionary string
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.

2 participants