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

Update ClickBench queries to avoid to_timestamp_seconds #15475

Merged
merged 2 commits into from
Mar 29, 2025

Conversation

acking-you
Copy link
Contributor

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Copy link
Contributor

@adriangb adriangb left a comment

Choose a reason for hiding this comment

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

Looks good to me. Since we're only ordering by this it shouldn't matter that we order by an integer instead of a proper timestamp, ordering is equivalent.

@adriangb
Copy link
Contributor

Should we update the same query in the clickbench repo as well?

@Dandandan
Copy link
Contributor

Should we update the same query in the clickbench repo as well?

Yes, and we might rerun the queries as well (as to_timestamp_seconds takes some time itself as well).

@acking-you
Copy link
Contributor Author

acking-you commented Mar 28, 2025

Looks good to me. Since we're only ordering by this it shouldn't matter that we order by an integer instead of a proper timestamp, ordering is equivalent.

Thank you very much for your reply, but I have a question: why can't the following order by "EventTime" be modified? It seems that they are equivalent (since the default is UTC timestamps). And both ClickHouse and duckdb do this way: duckdb clickhouse @adriangb

@adriangb
Copy link
Contributor

Looks good to me. Since we're only ordering by this it shouldn't matter that we order by an integer instead of a proper timestamp, ordering is equivalent.

Thank you very much for your reply, but I have a question: why can't the following order by "EventTime" be modified? It seems that they are equivalent (since the default is UTC timestamps). And both ClickHouse and duckdb do this way: duckdb clickhouse

I'm agreeing with you! I think this change is good 😄

@acking-you
Copy link
Contributor Author

I have also made changes to the query in ClickBench: clickbench-pr
@Dandandan @adriangb

@adriangb
Copy link
Contributor

@Dandandan do you think we can merge this here?

@Dandandan Dandandan merged commit 9071503 into apache:main Mar 29, 2025
27 checks passed
@Dandandan
Copy link
Contributor

Thank you @acking-you

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

Successfully merging this pull request may close these issues.

Update ClickBench queries to avoid to_timestamp_seconds
3 participants