PARQUET-7: [thrift] avoid thrift amender if all fields are optional. #7
Conversation
|
Hi @yan-qi , I created a patch that automatically disables the amender when the schema has no required fields. That's not as good as we could do, but it's a start, and should affect your schema in particular. Could you test using this pull request? |
|
Sorry for the late reply! :) I did some experiment, and it proves working. Thanks again for the help! When will this be released? |
|
@isnotinvain @julienledem @tsdeng suggestions for improving this? @yan-qi we need to get this into mergeable state -- as is it's probably a bit hacky and only applies to schemas that have no required fields, which is worse than we could potentially do; although perhaps a sufficiently common use case that we should just push that in for now. Thanks for testing that this really solves your problem, I'm glad :-). You could of course just build a jar off this branch, in the meantime, if you need it immediately. |
| case SET: | ||
| return somethingIsRequiredInSet((ThriftType.SetType) elementType); | ||
| default: | ||
| return false; |
There was a problem hiding this comment.
should this be an exception? Is it valid to land here?
There was a problem hiding this comment.
This is correct. since before it's saying "if (isRequired) return true;"
When it hits the default case, meaning it's a primitive type and also isRequired is false, so here it should just return false.
To make the intention of the code more clear, maybe @dvryaboy can change it to "return isRequired"
|
I reimplemented the idea here https://github.com/apache/incubator-parquet-mr/pull/28 |
…n the requested schema https://issues.apache.org/jira/browse/PARQUET-61 This PR is trying to redo the https://github.com/apache/incubator-parquet-mr/pull/7 In this PR, it fixes the protocol event in a more precise condition: Only when the requested schema missing some required fields that are present in the full schema So even if there a projection, as long as the projection is not getting rid of the required field, the protocol events amender will not be called. Could you take a look at this ? @dvryaboy @yan-qi Author: Tianshuo Deng <tdeng@twitter.com> Closes #28 from tsdeng/fix_protocol_when_required_field_missing and squashes the following commits: ba778b9 [Tianshuo Deng] add continue for readability d5639df [Tianshuo Deng] fix unused import 090e894 [Tianshuo Deng] format 13a609d [Tianshuo Deng] comment format ef1fe58 [Tianshuo Deng] little refactor, remove the hasMissingRequiredFieldFromProjection method 7c2c158 [Tianshuo Deng] format 83a5655 [Tianshuo Deng] do ProtocolEvents fixing only when there is required fields missing in the requested schema
…n the requested schema https://issues.apache.org/jira/browse/PARQUET-61 This PR is trying to redo the https://github.com/apache/incubator-parquet-mr/pull/7 In this PR, it fixes the protocol event in a more precise condition: Only when the requested schema missing some required fields that are present in the full schema So even if there a projection, as long as the projection is not getting rid of the required field, the protocol events amender will not be called. Could you take a look at this ? @dvryaboy @yan-qi Author: Tianshuo Deng <tdeng@twitter.com> Closes apache#28 from tsdeng/fix_protocol_when_required_field_missing and squashes the following commits: ba778b9 [Tianshuo Deng] add continue for readability d5639df [Tianshuo Deng] fix unused import 090e894 [Tianshuo Deng] format 13a609d [Tianshuo Deng] comment format ef1fe58 [Tianshuo Deng] little refactor, remove the hasMissingRequiredFieldFromProjection method 7c2c158 [Tianshuo Deng] format 83a5655 [Tianshuo Deng] do ProtocolEvents fixing only when there is required fields missing in the requested schema
…n the requested schema https://issues.apache.org/jira/browse/PARQUET-61 This PR is trying to redo the https://github.com/apache/incubator-parquet-mr/pull/7 In this PR, it fixes the protocol event in a more precise condition: Only when the requested schema missing some required fields that are present in the full schema So even if there a projection, as long as the projection is not getting rid of the required field, the protocol events amender will not be called. Could you take a look at this ? @dvryaboy @yan-qi Author: Tianshuo Deng <tdeng@twitter.com> Closes #28 from tsdeng/fix_protocol_when_required_field_missing and squashes the following commits: ba778b9 [Tianshuo Deng] add continue for readability d5639df [Tianshuo Deng] fix unused import 090e894 [Tianshuo Deng] format 13a609d [Tianshuo Deng] comment format ef1fe58 [Tianshuo Deng] little refactor, remove the hasMissingRequiredFieldFromProjection method 7c2c158 [Tianshuo Deng] format 83a5655 [Tianshuo Deng] do ProtocolEvents fixing only when there is required fields missing in the requested schema
|
this was merged as part of #28 |
Fix bug: Optional field in RowGroup is treated as always showing up
…ncement (apache#7) * [CARMEL-1260][FOLLOWUP] cannot read non-encrypted columns without decryptProperties if the first column is encrypted * [CARMEL-1858] fix ParquetFileReader for crypto after performance enhancement
No perf tests... I did some rudimentary testing which seems to bear out the idea but nothing scientific yet.