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. Part 1 #8818

Merged
merged 1 commit into from
Jan 13, 2024

Conversation

caicancai
Copy link
Member

@caicancai caicancai commented Jan 10, 2024

Which issue does this PR close?

part of #8216

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jan 10, 2024
@caicancai caicancai marked this pull request as draft January 10, 2024 14:24
@caicancai
Copy link
Member Author

caicancai commented Jan 10, 2024

2024-01-10 22-25-35屏幕截图
@alamb Hello,excuse me
I found that there is a test about the now function in timestamp.slt. I don’t know if I should add it.
And if I add the select now test, the result may be fixed, but the result of now is changing all the time, which may have an impact on the project itself.

@caicancai
Copy link
Member Author

Dot't merge

@alamb
Copy link
Contributor

alamb commented Jan 10, 2024

@alamb Hello,excuse me
I found that there is a test about the now function in timestamp.slt. I don’t know if I should add it.
And if I add the select now test, the result may be fixed, but the result of now is changing all the time, which may have an impact on the project itself.

Hi @caicancai -- if there are tests that can't be ported to sqllogictest (for example, that now() doesn't change across session statements) can you please put a comment on them saying something like "can't write this in sqllogictest, so use a rust test" and just leave them in timestamp.rs?

Thank you 🙏

@caicancai
Copy link
Member Author

caicancai commented Jan 11, 2024

2024-01-11 22-49-38屏幕截图

@alamb Hello, I have two questions here.

  1. The result I generated here seems to be different from the original result. Maybe there is something wrong with my table creation statement?
  2. I have observed that there are many duplicate test samples in timestamp_coercion. I am not sure whether I should add them all. My idea is that I only need to add one test.

@caicancai caicancai marked this pull request as ready for review January 11, 2024 14:52
Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

I think now can still be ported, its not stable value, but we can do it quasi stable like

select now() = current_time

or

select now() > to_date('2023-01-01')

@caicancai
Copy link
Member Author

I think now can still be ported, its not stable value, but we can do it quasi stable like

select now() = current_time

or

select now() > to_date('2023-01-01')

Thank you for your reply. I noticed that the original timestamp.slt already has some tests belonging to now. I am not sure whether to continue adding them.

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

Thanks for double checking now() is in timestamp.slt, I also verified that and looks like test in slt for now is enough, and now I'm wondering if we still need timestamp.rs?

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 @caicancai -- this looks like a good improvement to me. Perhaps we can investigate @comphead 's question about needing timestamp.rs at all as part of a follow on PR

@caicancai
Copy link
Member Author

Thanks for double checking now() is in timestamp.slt, I also verified that and looks like test in slt for now is enough, and now I'm wondering if we still need timestamp.rs?

From a personal point of view, I agree

@comphead comphead changed the title Port tests in timestamp.rs to sqllogictest Port tests in timestamp.rs to sqllogictest. Part 1 Jan 13, 2024
Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm @caicancai thanks.
I think it will be good to create a followup ticket to move remaining tests. I went trhough them real quick they looks migratable imho

@comphead comphead merged commit be361fd into apache:main Jan 13, 2024
23 checks passed
@caicancai caicancai deleted the timestamp branch February 4, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants