-
Notifications
You must be signed in to change notification settings - Fork 305
feat(sqllogictest): Add sqllogictest schedule definition and parsing #1630
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
Conversation
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.
Thanks @lliangyu-lin for this pr, LGTM! Just one nit.
Ok(Self::new(engines, steps)) | ||
} | ||
|
||
pub async fn run(mut self) -> anyhow::Result<()> { |
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.
Add a log about current schedule file?
crates/sqllogictest/src/schedule.rs
Outdated
.ok_or_else(|| anyhow!("Engine {} not found", step.engine))?; | ||
|
||
engine | ||
.run_slt_file(&PathBuf::from(step.slt.clone())) |
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.
This maybe a potential issue, the file path is relative to testdata dir, but we could fix it later.
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.
Good point, added the relative path in the previous draft PR, but missed it here. https://github.com/apache/iceberg-rust/pull/1621/files#diff-26b82f60b13d2afef45d856281a6fae47fefd48a440613ddcdd27059aec7639bR145-R150
Build seems to be broken. Put out a fix for it. Need to rerun after #1635 |
@@ -61,6 +51,16 @@ impl Engine for DataFusionEngine { | |||
} | |||
|
|||
impl DataFusionEngine { | |||
pub async fn new(config: TomlTable) -> Result<Self> { |
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 we move this out of trait?
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.
Thanks @lliangyu-lin for this pr!
Which issue does this PR close?
What changes are included in this PR?
Are these changes tested?
Yes, through unit tests. Some parts, like parse engines, require integrations and will be tested once engine and catalog support is added as part of the sql logic tests.