search

sunchao / parquet-rs

Apache Parquet implementation in Rust
Apache License 2.0
149 stars 20 forks source link

Disallow get_encoder function for dictionary encoding #123

Closed sadikovi closed 6 years ago

sadikovi commented 6 years ago

This PR updates get_encoder() method, similar to its counterpart get_decoder() method, to disallow instantiation of DictEncoder, because dictionary encoder provides several methods for writing dictionary indices and values, which are not exposed in Encoder API.

Another approach would be to just document that dictionary encoder should be instantiated explicitly, if those methods are needed, but then it would defeat having dictionary encoder in the first place. Plus, the current approach is consistent with decoding.

I also updated tests to use get_encoder and get_decoder methods, similar to decoder tests.

sadikovi commented 6 years ago

@sunchao could you review this PR? Adds consistent handling of encoding/decoding. Do you think it would be useful to add any tests for these changes? Thanks!

sunchao commented 6 years ago

Test succeeded locally - let me re-trigger the build.

sadikovi commented 6 years ago

@sunchao Thanks for the review. I refactored code in both decoding and encoding to have similar test where we initialise encoders/decoders and check the error. Let me know if you want me to add/update tests.

sunchao commented 6 years ago

This looks good. Thanks!

sunchao commented 6 years ago

Merged. Thanks @sadikovi !