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

Support adding fields to nested Parquet structs #40

Open
wants to merge 134 commits into
base: twitter-master
Choose a base branch
from

Conversation

billonahill
Copy link
Collaborator

Patch of prestodb#4939 for twitter fork.

Sailesh Mittal and others added 30 commits September 25, 2015 15:25
Parquet only calls converts for which it found the values. The missing
values are not reported. The BlockBuilder must be appended with
nulls for the missing values based on fieldIndex of the currently
read value by Parquet.
…pass

Ignore errors if the partitions have no corresponding valid hdfs path.
…gParquetColumns

Append nulls for missing values in Parquet.
This reverts commit 7365f1a.
This blocked us from reading from mountable clusters.
# Conflicts:
#	presto-main/src/main/java/com/facebook/presto/sql/analyzer/ExpressionAnalyzer.java
Pick username from unix system and disallow overriding it.
@@ -69,6 +69,7 @@
implements ConnectorSplitManager
{
public static final String PRESTO_OFFLINE = "presto_offline";
private static final String STRUCT_PREFIX = "struct<";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to use "RowType" instead of "struct"? Or may be use com.facebook.presto.hive.HiveType.java.
Same for the rest of the patch. Presto has RowType and struct is not used here directly IIRC.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this part pained me pretty bad. I dug around for something more type safe but I missed HiveType. Refactoring.

@saileshmittal
Copy link
Collaborator

LGTM, I saw some existing PR as well. We can start with this and wait for that to merge to master and released..

@billonahill
Copy link
Collaborator Author

Yeah, sounds good. Thanks.

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.

3 participants