Skip to content

Conversation

@philrz
Copy link
Contributor

@philrz philrz commented Jul 3, 2024

I've been holding off on adding Parquet as an Export format in Zui until #2751 was fixed, since there's so many Parquet limitations that can prevent the export from succeeding (e.g., lack of support for multiple types, lack of duration support, poor union support, etc.) But now that #2751 has been fixed and users will see the errors when exports fail, here I'm adding support for Parquet as an Export format.

Due to those same Parquet limitations, I could not just extend the existing loop-through-all-formats approach in the existing Export e2e test because an attempted export of that same test data file would fail. I saw a few ways this could be addressed:

  1. I could rework the existing test to use a different, Parquet-eligible test data file for export in all formats and change all the expected byte counts to match. I chose not to pursue this since it didn't seem appropriate to "dumb down" to the lowest common denominator.

  2. I could add an additional test just for Parquet that imports its own Parquet-eligible test data file. I took this approach in the first commit on this branch. However, I had to duplicate some code from the existing test.

  3. Same as the prior, but I could break out the common code to a helper function that could be called by both the prior loop-through-all-formats approach as well as the new test for just Parquet. I took this approach in the second commit on this branch.

Of course, there's surely other variations, so I'm open to other adjustments.

Closes #2591

@philrz philrz requested a review from jameskerr July 3, 2024 19:31
@philrz philrz self-assigned this Jul 3, 2024
Copy link
Contributor

@jameskerr jameskerr left a comment

Choose a reason for hiding this comment

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

This is great!

@philrz philrz merged commit a28b421 into main Jul 3, 2024
@philrz philrz deleted the export-parquet branch July 3, 2024 19:46
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.

Add Parquet as an Export format

3 participants