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

Remove table layouts from Hive #689

Merged
merged 12 commits into from Jun 1, 2019

Conversation

@electrum
Copy link
Member

commented Apr 29, 2019

No description provided.

@cla-bot cla-bot bot added the cla-signed label Apr 29, 2019

@electrum electrum force-pushed the electrum:hive-layout branch from ca94211 to 51a6dcb Apr 30, 2019

@electrum electrum force-pushed the electrum:hive-layout branch 9 times, most recently from 37b6dae to 65e99a0 Apr 30, 2019

@martint martint self-requested a review May 6, 2019

@martint
Copy link
Member

left a comment

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

@electrum electrum force-pushed the electrum:hive-layout branch 3 times, most recently from a1038b9 to e078200 May 10, 2019

@electrum electrum referenced this pull request May 16, 2019

Open

Remove table layouts from all connectors #781

10 of 15 tasks complete
@martint

This comment has been minimized.

Copy link
Member

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)

Show resolved Hide resolved ...to-hive/src/main/java/io/prestosql/plugin/hive/HivePartitionManager.java Outdated
Show resolved Hide resolved ...to-hive/src/main/java/io/prestosql/plugin/hive/HivePartitionManager.java
Show resolved Hide resolved ...src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java Outdated
Show resolved Hide resolved ...src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java Outdated
Show resolved Hide resolved ...rc/test/java/io/prestosql/plugin/hive/parquet/TestFullParquetReader.java Outdated
Show resolved Hide resolved ...ve/src/test/java/io/prestosql/plugin/hive/parquet/TestParquetReader.java Outdated
@@ -329,7 +332,11 @@ public ConnectorTableHandle getTableHandleForStatisticsCollection(ConnectorSessi
}
});

return handle;
HiveTableHandle table = handle;
return partitionValuesList

This comment has been minimized.

Copy link
@martint

martint May 21, 2019

Member

Why is this needed now?

This comment has been minimized.

Copy link
@electrum

electrum May 22, 2019

Author Member

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.

Show resolved Hide resolved presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java Outdated

@electrum electrum removed their assignment May 22, 2019

@electrum electrum force-pushed the electrum:hive-layout branch from e078200 to 783230c May 22, 2019

@electrum

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

@martint updated

@electrum electrum force-pushed the electrum:hive-layout branch from 783230c to d9b7e96 May 31, 2019

@electrum electrum merged commit 486fe07 into prestosql:master Jun 1, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
verification/cla-signed
Details

@electrum electrum deleted the electrum:hive-layout branch Jun 1, 2019

@findepi findepi added this to the 314 milestone Jun 6, 2019

@electrum electrum referenced this pull request Jun 7, 2019

Closed

Release notes for 314 #879

2 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.