[DRAFT] feat(test): Add support for sqllogictests framework #1621
Conversation
| spark-connect-rs = { git = "https://github.com/apache/spark-connect-rust.git", rev = "061cb3ecb187b039141f20c722c7984e915f3b9d" } | ||
| #spark-connect-rs = "0.0.2" |
There was a problem hiding this comment.
There seems to have some issues with spark-connect-rs 0.0.2 because it's using arrow 53, and that isn't compatible with chrono 0.4.40+, which is a known problem apache/arrow-rs#7196
New version of spark-connect-rs doesn't seem will be release soon, using git url for right now until new version is released.
There was a problem hiding this comment.
Will this block our release? We don't need to publish this crate, just use it in integration tests.
There was a problem hiding this comment.
Sorry for the late response. Have been busy with other works.
The issue I'm concerned here is that with using direct git url, to build the crate will now require to build spark-connect-rs first, which requires cmake and protobuf installed. https://github.com/sjrusso8/spark-connect-rs/blob/main/README.md?plain=1#L52
That's the main reason why I excluded to build this in Makefile
There was a problem hiding this comment.
I don't think it will block the release of iceberg rust, but just need additional environment setup changes in the CI.
There was a problem hiding this comment.
Latest version has add requirements for protobuf. I don't quite understand why spark-connect-rs requires cmake.
I don't think it will block the release of iceberg rust,
The problems is that when you publish crates to crates.io, and you have a dependency in url format, the publishing may fail. But I'm not sure if this will affect sqllogictest, since we don't need to publish this crate.
…1630) ## Which issue does this PR close? - Closes #1214 . ## What changes are included in this PR? * Add schedule definition * Parse schedule to engines involved and test steps * Part of #1621 ## 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. --------- Co-authored-by: liurenjie1024 <liurenjie2008@gmail.com> Co-authored-by: Leon Lin <lliangyu@amazon.com>
| Ok(Self { engines, steps }) | ||
| } | ||
|
|
||
| async fn parse_engines( |
There was a problem hiding this comment.
The parsing logic maintained manually. Would be nice if we could use ser/de framework for them.
There was a problem hiding this comment.
Also applied to creation of engines.
There was a problem hiding this comment.
I agree, using serde could help clean up the logics here. Probably could improve on this once it's working end to end.
Which issue does this PR close?
What changes are included in this PR?
Are these changes tested?