-
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
Add CI checks that we can serde all benchmark queries #4047
Conversation
@avantgardnerio fyi |
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.
I couldn't find evidence that this test actually ran 🤔
|
||
/// CI checks | ||
#[cfg(test)] | ||
#[cfg(feature = "ci")] |
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.
Why feature gate this with ci
? I would have thought
#[cfg(test)]
was sufficient.
Additionally, I couldn't find evidence that these new tests were actually run:
-*- mode: grep; default-directory: "~/Downloads/logs_52324/" -*-
Grep started at Tue Nov 1 12:19:52
rg -n -H --no-heading -e 'serde_q7'
Grep finished with no matches found at Tue Nov 1 12:19:52
🤔
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.
These tests rely on the data files existing in a path from TPCH_DATA
env var, which is set in CI
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.
I did look into using MemTable but we don't support serde with MemTable
Thanks for catching that! I pushed a commit to add these tests to the workflow |
Verified that tests ran this time:
|
Benchmark runs are scheduled for baseline = 525ac45 and contender = 8c26530. 8c26530 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
* Add CI checks that we can serde all benchmark queries * move all ci tests to separate module * format * run serde tests in CI
Which issue does this PR close?
N/A
Rationale for this change
Ballista relies on the ability to serde all benchmark queries. These CI checks help ensure that DataFusion continues to support this.
What changes are included in this PR?
Are there any user-facing changes?
No