Repository navigation
feat: TPC-DS support - #212
Conversation
|
This is on my list to review -- hopefully I'll have time to do it later today |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
👏 pretty epic @clflushopt --
I took a careful look at the github stuff and gave a cursory review of the other 220 files (!!) and I think we should proceed with merging it and file some follow on tasks
The code I read was well structured, well commented, and I think will be straightforward to maintain / update as needed. Well done.
Some ideas for follow on tasks
- Work on conformance testing with C (e.g. #51 (comment))
- Improve CI checks (e.g. figure out how to avoid having to maintain all the java setup, etc)
- start integrating into the cli
|
I noticed several comments about "bugs in the C code" which I assume come from the Java port. It wasn't clear to me of the java people "fixed" the bugs in the C code -- if they did this could be the source of the differences between the C implementation and the rust one I would personally prefer to follow the C semantics (bugs and all) as that is the 'source of truth' but that can be a discussion we have after his is merged Thank you again -- this is pretty epic |
Actually the documented bugs are not fixed in the Java version and are not fixed in the Rust implementation as well they are just documented. Meanwhile thanks for the review, I will address the items for CI in the same PR (it's easier this way). |
|
@alamb I addressed all comments. Thanks again for the review ! Edit: I'll resolve all the comments to merge cleanly |
|
One thing to test folks: once the data is generated it would be good to see how many TPCDS queries return non empty results. Currently it is ~15% with blank results, nice if this generator could improve the ratio |
This PR adds TPC-DS support which was built initially out of tree in tpcdsgen