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

Add json output #43

Merged
merged 1 commit into from
Apr 29, 2024
Merged

Conversation

bhagenbourger
Copy link
Contributor

#32

@bhagenbourger bhagenbourger marked this pull request as draft March 23, 2024 15:36
@bhagenbourger bhagenbourger force-pushed the feature/output_json branch 4 times, most recently from b46c8d9 to f0408ab Compare March 25, 2024 21:35
@bhagenbourger bhagenbourger marked this pull request as ready for review April 24, 2024 12:44
@bhagenbourger bhagenbourger marked this pull request as draft April 24, 2024 13:51
@bhagenbourger
Copy link
Contributor Author

Hello @vianneybacoup ,
In this PR I put all test results into "target" folder excepted "output.*" because it's the default value for the output_name.
I do that to keep root folder "clean", to easier clean test results and also to avoid to add all extensions into the .gitignore file.
But, as default value is "output", I don't really know if it's really useful.
What do you think about that?

Thank you for your feedback.

@vianneybacoup
Copy link
Collaborator

Hello @vianneybacoup , In this PR I put all test results into "target" folder excepted "output.*" because it's the default value for the output_name. I do that to keep root folder "clean", to easier clean test results and also to avoid to add all extensions into the .gitignore file. But, as default value is "output", I don't really know if it's really useful. What do you think about that?

Thank you for your feedback.

I see that we both had the same issues, I did way too much 'rm *.parquet *.csv' past months haha
I had in mind we could have like in Python a tearDown fucntion that cleanup the output of the test automatically, but that does not exist in rust, and this is not a mandatory feature, the folder is clearly enough.
Maybe rename it with target/test_generated or something like this just to avoid a possible confusion with the other targets?

@bhagenbourger bhagenbourger force-pushed the feature/output_json branch 2 times, most recently from 32da7cb to 8d281cb Compare April 25, 2024 20:25
@bhagenbourger
Copy link
Contributor Author

I moved output tests into target/test_generated/ folder.
I kept "all options" tests into target folder because they are run only by github action and generation fails if folder not exists (maybe could be a good enhancement to automatically generate intermediate folders?).
I added ctor crate to add macro to create target/test_generated/ folder before tests and clean test outputs after tests.

About json output, I did a POC using arrow (https://github.com/bhagenbourger/Fakelake/tree/poc/use_arrow_for_all_format) because I found interesting to use the same "generator" for all formats. It works but with some limitations as using the same date format for all columns. So definitively, serde-json is more flexible and I keep this implementation.

@bhagenbourger bhagenbourger marked this pull request as ready for review April 27, 2024 12:27
Copy link
Collaborator

@vianneybacoup vianneybacoup left a comment

Choose a reason for hiding this comment

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

About arrow for json, I am also wondering if it has poorer performances than serde-json.
Fakelake whole purpose is the speed, so I'd say it should be the focus

Small adjustements to be done for this PR, thank you for the work :)

Copy link
Collaborator

@vianneybacoup vianneybacoup left a comment

Choose a reason for hiding this comment

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

Thanks ! :)

@vianneybacoup vianneybacoup merged commit 07a5860 into soma-smart:main Apr 29, 2024
5 checks passed
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.

2 participants