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

Port tests in timestamp.rs to sqllogictest #8216

Closed
Tracked by #6195
alamb opened this issue Nov 15, 2023 · 12 comments · Fixed by #8859
Closed
Tracked by #6195

Port tests in timestamp.rs to sqllogictest #8216

alamb opened this issue Nov 15, 2023 · 12 comments · Fixed by #8859
Labels
good first issue Good for newcomers

Comments

@alamb
Copy link
Contributor

alamb commented Nov 15, 2023

Is your feature request related to a problem or challenge?

Part of #6195. We are trying to

  1. keep the DataFusion code base clean and easy to contribute to and modify
  2. Understand what features are covered and what are not

Part of doing so is having a single location for most test coverage so new tests can be added easily and people can follow the existing patterns easily

Describe the solution you'd like

Port the tests in https://github.com/apache/arrow-datafusion/blob/main/datafusion/core/tests/sql/timestamp.rs to
timestamp.slt in https://github.com/apache/arrow-datafusion/tree/main/datafusion/sqllogictest/test_files

Note you can create catalogs and schema via SQL now

Catalogs: https://arrow.apache.org/datafusion/user-guide/sql/ddl.html#create-database
Schema: https://arrow.apache.org/datafusion/user-guide/sql/ddl.html#create-schema

Notes:

  1. Instructions for running sqllogitest are here: https://github.com/apache/arrow-datafusion/tree/main/datafusion/sqllogictest

Describe alternatives you've considered

No response

Additional context

I think these are good first issues as they teach the contributor about the DataFusion codebase and tests, as well as see the end user apis in practice.

@caicancai
Copy link
Member

caicancai commented Dec 7, 2023

Thank you. Can I have a try?

@alamb
Copy link
Contributor Author

alamb commented Dec 7, 2023

Thank you @caicancai

@caicancai
Copy link
Member

caicancai commented Dec 10, 2023

@caicancai
Copy link
Member

The number of tests seems to be quite large

@alamb
Copy link
Contributor Author

alamb commented Dec 11, 2023

Thanks @caicancai

@alamb Hello, all tests are added to https://github.com/apache/arrow-datafusion/tree/main/datafusion/sqllogictest/test_files ?

Yes perhaps in timestamp.slt or something similar

I think part of the reason there are many old tests is that the tests predate arrow_cast so the setup of the data took quite a lot more code. Using arrow_cast(col, Timestamp(Nanosecond, None)) type calls should reduce the code significatnly

The number of tests seems to be quite large

Indeed -- perhaps you could pick one or two tests to port over initially rather than trying to it all in a single PR. That would let us provide incremental feedback along the way as well and make reviewing the PRs easier as well

@caicancai
Copy link
Member

Thanks @caicancai

@alamb Hello, all tests are added to https://github.com/apache/arrow-datafusion/tree/main/datafusion/sqllogictest/test_files ?

Yes perhaps in timestamp.slt or something similar

I think part of the reason there are many old tests is that the tests predate arrow_cast so the setup of the data took quite a lot more code. Using arrow_cast(col, Timestamp(Nanosecond, None)) type calls should reduce the code significatnly

The number of tests seems to be quite large

Indeed -- perhaps you could pick one or two tests to port over initially rather than trying to it all in a single PR. That would let us provide incremental feedback along the way as well and make reviewing the PRs easier as well

Thanks for the reply, I will try it

@alamb
Copy link
Contributor Author

alamb commented Dec 11, 2023

Thanks for the reply, I will try it

Thank you very much @caicancai ❤️

@caicancai
Copy link
Member

Thanks for the reply, I will try it

Thank you very much @caicancai ❤️

I prefer to understand the code before developing it, so this takes a certain amount of time. This is a good project.

@alamb
Copy link
Contributor Author

alamb commented Dec 11, 2023

I prefer to understand the code before developing it, so this takes a certain amount of time.

Indeed -- I think this is a mark of a good software engineer

@caicancai
Copy link
Member

Sorry, I didn't start this part of the work some time ago because I was busy with other things. I will complete this part of the work next week.

@caicancai
Copy link
Member

caicancai commented Jan 9, 2024

@alamb Hello, excuse me, please allow me to ask a stupid question. I want to know how I test the results locally, cargo test timestamps.slt? But cargo test timestamps.slt doesn't show the correct result to me if the result is an error result
I am a rust beginner
I am a beginner in rust, and I am not very clear about some operations in rust engineering. Sorry.

@alamb
Copy link
Contributor Author

alamb commented Jan 9, 2024

@alamb Hello, excuse me, please allow me to ask a stupid question. I want to know how I test the results locally, cargo test timestamps.slt?

Hi @caicancai -- no worry at all!

To run sqllogictests tests locally, you need to run a command like

cargo test --test sqllogictests -- timestamps.slt

There is more information on how to run the tests is here: https://github.com/apache/arrow-datafusion/tree/main/datafusion/sqllogictest#running-tests-tldr-examples

cargo test timestamps.slt doesn't show the correct result to me if the result is an error result I am a rust beginner I am a beginner in rust, and I am not very clear about some operations in rust engineering. Sorry.

No problem at all -- welcome to the community! The datafusion sqllogictest testing framework is not a standard rust test, which makes the bar higher

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants