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.

Sign up for GitHub

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

Jump to bottom

Remove table layouts from Hive #689

Merged
merged 12 commits into from
Jun 1, 2019
Merged

Remove table layouts from Hive #689

merged 12 commits into from
Jun 1, 2019

Conversation

electrum
Copy link
Member

No description provided.

@cla-bot cla-bot bot added the cla-signed label Apr 29, 2019
@electrum electrum force-pushed the hive-layout branch 9 times, most recently from 37b6dae to 65e99a0 Compare May 3, 2019 00:18
@martint martint self-requested a review May 6, 2019 23:00
martint
martint reviewed May 10, 2019
View reviewed changes
Copy link
Member

@martint martint left a comment

Choose a reason for hiding this comment

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

Some initial comments. I'm still reviewing the "Remove table layouts from Hive" commit

presto-main/src/main/java/io/prestosql/operator/MetadataDeleteOperator.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/sql/planner/optimizations/AddExchanges.java Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/sql/planner/plan/MetadataDeleteNode.java Outdated Show resolved Hide resolved
@electrum electrum force-pushed the hive-layout branch 3 times, most recently from a1038b9 to e078200 Compare May 16, 2019 23:52
@electrum electrum mentioned this pull request May 16, 2019
Closed
16 tasks
@martint
Copy link
Member

martint commented May 20, 2019

This should wait for #731. The legacy PushPredicateIntoTableScan logic processes the predicates through DomainTranslator, which normalizes expressions in the same way UnwrapCastsInComparison in that PR does. However, the normalization is not done for the new applyFilter. Leaving casts "wrapped" causes some of the cost estimations to fail, so it affects the ability for the join ordering optimizer to do its job. #731 introduces the normalization for expressions anywhere in the plan, so it compensates for the "missing" behavior in handling applyFilter (arguably, it should have never been there in the first place)

martint
martint reviewed May 21, 2019
View reviewed changes
presto-hive/src/main/java/io/prestosql/plugin/hive/HivePartitionManager.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HivePartitionManager.java Show resolved Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java Outdated Show resolved Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java Outdated Show resolved Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/parquet/TestFullParquetReader.java Outdated Show resolved Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/parquet/TestParquetReader.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java
@@ -329,7 +332,11 @@ public ConnectorTableHandle getTableHandleForStatisticsCollection(ConnectorSessi
}
});

return handle;
HiveTableHandle table = handle;
return partitionValuesList
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed now?

Copy link
Member Author

Choose a reason for hiding this comment

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

The listing has to be done somewhere. Doing it here makes sense as we'd otherwise need to move this into HivePartitionManager.getOrLoadPartitions(), with the special case logic for the analyze partition values list.

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java Outdated Show resolved Hide resolved
@electrum electrum removed their assignment May 22, 2019
@electrum
Copy link
Member Author

@martint updated

@martint martint mentioned this pull request May 27, 2019
Closed
@electrum electrum merged commit 486fe07 into trinodb:master Jun 1, 2019
@electrum electrum deleted the hive-layout branch June 1, 2019 22:45
@findepi findepi added this to the 314 milestone Jun 6, 2019
@electrum electrum mentioned this pull request Jun 7, 2019
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kokosing kokosing kokosing left review comments

@takezoe takezoe takezoe left review comments

@martint martint martint approved these changes

Assignees

@martint martint

Labels
cla-signed
Milestone
314
Development

Successfully merging this pull request may close these issues.
5 participants
@electrum @martint @takezoe @kokosing @findepi