Add support for query pushdown to S3 using S3Select #11033

same3r · 2018-07-13T01:43:02Z

This change will allow Presto users to improve the performance of their queries using S3SelectPushdown.
It pushes down projections and predicate evaluations to S3. As a result
Presto doesn't need to download full S3 objects and only data required to answer the user's query
is returned to Presto, thereby improving performance.

S3SelectPushdown Technical Document: S3SelectPushdown.pdf

PR UPDATE
Closed this PR as it was slow to work with due to large volume of comments. Created a new PR to continue the work #11970

facebook-github-bot · 2018-07-13T01:43:11Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

same3r · 2018-07-13T01:49:14Z

Working on getting the CLA action item closed.

sopel39 · 2018-07-13T09:06:45Z

@sameer2086 This only works for text files, am I correct? Do you plan to use other file formats in the future?

presto-hive/src/main/java/com/facebook/presto/hive/HiveS3Optimizer.java

same3r · 2018-07-13T17:10:23Z

@sopel39
The optimizer currently supports only CSV documents. We will add new formats in near future. Our plan is to keep the change list small to make it easier for community to review the changes.

presto-hive/src/main/java/com/facebook/presto/hive/IonSqlQueryBuilder.java

presto-hive/src/main/java/com/facebook/presto/hive/S3SelectCsvRecordReader.java

same3r · 2018-07-14T00:35:37Z

Addressed cr feedback. New revision is out.

hyandell · 2018-07-18T06:42:19Z

Just noting that I'm told (by FB OSPO) that "If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed." is inaccurate.

Facebook's position (from CLA page) is: "If you are making contributions to our repositories on behalf of your company, then we will need a Corporate Contributor License Agreement (CLA) signed."

Just as a note that your CLABot may be out of date.

presto-hive/src/main/java/com/facebook/presto/hive/S3SelectLineRecordReader.java

findepi · 2018-07-26T07:15:15Z

Referring to our offline discussion -- some general thoughts. I am looking for someone to substitute me in the review here, as I have limited availability currently.

this feature needs some documentation.
- mentioning the feature toggle in the configs table for hive connector
- a separate section about feature's pros and cons. No need for marketing stuff "hey this is great" -- just technical description what it does, limitations for supported file format, perhaps a standard warning that may incur additional cost, and a sentence or two of recommendation when it makes sense to use this (and when it does not)
it would be good to have some benchmarks. @kbajda do you have any suggestions what kind of benchmarks would be welcomed here? I thought about TPC-DS suite, CSV without S3 select, SCV with S3 select and ORC, but maybe you could suggest something better.

kbajda · 2018-07-26T13:13:22Z

@findepi Agreed. Either TPC-H or TPC-DS against Text (with S3 select on/off) plus ORC as a reference.
Showing runtimes and the effectiveness of S3 Select (faster queries vs some extra cost incurred).

same3r · 2018-07-26T19:48:52Z

Thanks for the inputs findepi@ and kbajda@, we are working on the technical documents and benchmark numbers and will update the PR with the same.

nezihyigitbasi

I made a pass over non-test classes only. Here are some notes:

This feature definitely needs proper documentation.
There is code duplication in PrestoS3Client and PrestoS3FileSystem
There is lots of log info statements (e.g., for each branch taken, etc.)
We shouldn't introduce a new dependency just for retry support (see my comment about RetryDriver)
Class hierarchies can be given more thought (e.g., abstract classes and interfaces with only one subclass/implementation)
I left many comments to give you an idea about how to make this PR more consistent with our coding conventions.

presto-hive/pom.xml

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

presto-hive/src/main/java/com/facebook/presto/hive/HiveS3Optimizer.java

presto-hive/src/main/java/com/facebook/presto/hive/S3SelectRecordCursorProvider.java

same3r · 2018-07-27T22:10:36Z

Thanks for the detailed code review @nezihyigitbasi, working on the feedback. Will respond to your questions soon.

presto-hive/src/main/java/com/facebook/presto/hive/HiveS3Optimizer.java

presto-hive/src/main/java/com/facebook/presto/hive/S3SelectLineRecordReader.java

kobokinken · 2018-07-29T01:41:23Z

Where's Nikki?

…

On Thu, Jul 12, 2018, 6:43 PM Sameer Choudhary ***@***.***> wrote: This change will allow Presto users to improve the performance of their queries using S3Select. The optimizer pushes down projections and predicate evaluation to S3 using S3Optimizer. As a result Presto doesn't need to download full S3 objects and only data required to answer the user's query is returned to Presto, thereby improving performance. ------------------------------ You can view, comment on, or merge this pull request online at: #11033 Commit Summary - Add support for S3Optimizer using S3Select File Changes - *M* pom.xml <https://github.com/prestodb/presto/pull/11033/files#diff-0> (9) - *M* presto-hive-hadoop2/pom.xml <https://github.com/prestodb/presto/pull/11033/files#diff-1> (32) - *M* presto-hive/pom.xml <https://github.com/prestodb/presto/pull/11033/files#diff-2> (21) - *M* presto-hive/src/main/java/com/facebook/presto/hive/BackgroundHiveSplitLoader.java <https://github.com/prestodb/presto/pull/11033/files#diff-3> (5) - *M* presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java <https://github.com/prestodb/presto/pull/11033/files#diff-4> (15) - *M* presto-hive/src/main/java/com/facebook/presto/hive/HiveClientModule.java <https://github.com/prestodb/presto/pull/11033/files#diff-5> (1) - *M* presto-hive/src/main/java/com/facebook/presto/hive/HivePageSourceProvider.java <https://github.com/prestodb/presto/pull/11033/files#diff-6> (6) - *M* presto-hive/src/main/java/com/facebook/presto/hive/HivePartitionMetadata.java <https://github.com/prestodb/presto/pull/11033/files#diff-7> (9) - *A* presto-hive/src/main/java/com/facebook/presto/hive/HiveS3Optimizer.java <https://github.com/prestodb/presto/pull/11033/files#diff-8> (204) - *M* presto-hive/src/main/java/com/facebook/presto/hive/HiveSessionProperties.java <https://github.com/prestodb/presto/pull/11033/files#diff-9> (11) - *M* presto-hive/src/main/java/com/facebook/presto/hive/HiveSplit.java <https://github.com/prestodb/presto/pull/11033/files#diff-10> (13) - *M* presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java <https://github.com/prestodb/presto/pull/11033/files#diff-11> (24) - *M* presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitSource.java <https://github.com/prestodb/presto/pull/11033/files#diff-12> (4) - *M* presto-hive/src/main/java/com/facebook/presto/hive/HiveUtil.java <https://github.com/prestodb/presto/pull/11033/files#diff-13> (20) - *M* presto-hive/src/main/java/com/facebook/presto/hive/InternalHiveSplit.java <https://github.com/prestodb/presto/pull/11033/files#diff-14> (10) - *A* presto-hive/src/main/java/com/facebook/presto/hive/IonSqlQueryBuilder.java <https://github.com/prestodb/presto/pull/11033/files#diff-15> (175) - *A* presto-hive/src/main/java/com/facebook/presto/hive/PrestoS3Client.java <https://github.com/prestodb/presto/pull/11033/files#diff-16> (135) - *A* presto-hive/src/main/java/com/facebook/presto/hive/PrestoS3SelectClient.java <https://github.com/prestodb/presto/pull/11033/files#diff-17> (97) - *A* presto-hive/src/main/java/com/facebook/presto/hive/QueryBuilder.java <https://github.com/prestodb/presto/pull/11033/files#diff-18> (181) - *A* presto-hive/src/main/java/com/facebook/presto/hive/S3SelectClient.java <https://github.com/prestodb/presto/pull/11033/files#diff-19> (33) - *A* presto-hive/src/main/java/com/facebook/presto/hive/S3SelectCsvRecordReader.java <https://github.com/prestodb/presto/pull/11033/files#diff-20> (94) - *A* presto-hive/src/main/java/com/facebook/presto/hive/S3SelectLineRecordReader.java <https://github.com/prestodb/presto/pull/11033/files#diff-21> (200) - *A* presto-hive/src/main/java/com/facebook/presto/hive/S3SelectRecordCursor.java <https://github.com/prestodb/presto/pull/11033/files#diff-22> (201) - *A* presto-hive/src/main/java/com/facebook/presto/hive/S3SelectRecordCursorProvider.java <https://github.com/prestodb/presto/pull/11033/files#diff-23> (85) - *M* presto-hive/src/main/java/com/facebook/presto/hive/s3/HiveS3Config.java <https://github.com/prestodb/presto/pull/11033/files#diff-24> (1) - *M* presto-hive/src/main/java/com/facebook/presto/hive/util/InternalHiveSplitFactory.java <https://github.com/prestodb/presto/pull/11033/files#diff-25> (8) - *M* presto-hive/src/test/java/com/facebook/presto/hive/AbstractTestHiveClient.java <https://github.com/prestodb/presto/pull/11033/files#diff-26> (3) - *M* presto-hive/src/test/java/com/facebook/presto/hive/AbstractTestHiveFileSystem.java <https://github.com/prestodb/presto/pull/11033/files#diff-27> (3) - *M* presto-hive/src/test/java/com/facebook/presto/hive/HiveTestUtils.java <https://github.com/prestodb/presto/pull/11033/files#diff-28> (13) - *A* presto-hive/src/test/java/com/facebook/presto/hive/MockAmazonS3.java <https://github.com/prestodb/presto/pull/11033/files#diff-29> (1014) - *M* presto-hive/src/test/java/com/facebook/presto/hive/TestBackgroundHiveSplitLoader.java <https://github.com/prestodb/presto/pull/11033/files#diff-30> (5) - *M* presto-hive/src/test/java/com/facebook/presto/hive/TestHiveClientConfig.java <https://github.com/prestodb/presto/pull/11033/files#diff-31> (7) - *M* presto-hive/src/test/java/com/facebook/presto/hive/TestHivePageSink.java <https://github.com/prestodb/presto/pull/11033/files#diff-32> (3) - *M* presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSplit.java <https://github.com/prestodb/presto/pull/11033/files#diff-33> (4) - *M* presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSplitSource.java <https://github.com/prestodb/presto/pull/11033/files#diff-34> (20) - *A* presto-hive/src/test/java/com/facebook/presto/hive/TestIonSqlQueryBuilder.java <https://github.com/prestodb/presto/pull/11033/files#diff-35> (158) - *M* presto-main/etc/catalog/hive.properties <https://github.com/prestodb/presto/pull/11033/files#diff-36> (4) Patch Links: - https://github.com/prestodb/presto/pull/11033.patch - https://github.com/prestodb/presto/pull/11033.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#11033>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWhhj2qSlq5oET5a4xNKHJen-sa-sp7Qks5uF_tNgaJpZM4VOLCF> .

facebook-github-bot · 2018-07-31T20:29:21Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

presto-hive/src/main/java/com/facebook/presto/hive/S3SelectCsvRecordReader.java

presto-hive/src/main/java/com/facebook/presto/hive/PrestoS3Client.java

same3r · 2018-08-18T01:52:53Z

We ran TPCDS-100 tests and a small fraction of the queries are currently failing due to com.amazonaws.SdkClientException.

I had a quick discussion with nezih@ and the plan is to fix this bug before merging the code with master.

I have updated the code as per the feedback and will upload the new revision soon. There will be follow-up revision to fix the identified bug.

2018-08-11T20:41:33.437Z        ERROR   remote-task-callback-387        com.facebook.presto.execution.StageStateMachine Stage 20180811_203931_00007_ew4yh.22 failed
com.facebook.presto.spi.PrestoException: com.github.rholder.retry.RetryException: Retrying failed to complete successfully after 5 attempts.
        at com.facebook.presto.hive.GenericHiveRecordCursor.advanceNextPosition(GenericHiveRecordCursor.java:215)
        at com.facebook.presto.hive.S3SelectRecordCursor.advanceNextPosition(S3SelectRecordCursor.java:35)
        at com.facebook.presto.hive.HiveCoercionRecordCursor.advanceNextPosition(HiveCoercionRecordCursor.java:103)
        at com.facebook.presto.hive.HiveRecordCursor.advanceNextPosition(HiveRecordCursor.java:175)
        at com.facebook.presto.$gen.CursorProcessor_20180811_203934_46.process(Unknown Source)
        at com.facebook.presto.operator.ScanFilterAndProjectOperator.processColumnSource(ScanFilterAndProjectOperator.java:237)
        at com.facebook.presto.operator.ScanFilterAndProjectOperator.getOutput(ScanFilterAndProjectOperator.java:229)
        at com.facebook.presto.operator.Driver.processInternal(Driver.java:373)
        at com.facebook.presto.operator.Driver.lambda$processFor$8(Driver.java:282)
        at com.facebook.presto.operator.Driver.tryWithLock(Driver.java:672)
        at com.facebook.presto.operator.Driver.processFor(Driver.java:276)
        at com.facebook.presto.execution.SqlTaskExecution$DriverSplitRunner.processFor(SqlTaskExecution.java:973)
        at com.facebook.presto.execution.executor.PrioritizedSplitRunner.process(PrioritizedSplitRunner.java:162)
        at com.facebook.presto.execution.executor.TaskExecutor$TaskRunner.run(TaskExecutor.java:477)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
        at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.RuntimeException: com.github.rholder.retry.RetryException: Retrying failed to complete successfully after 5 attempts.
        at com.facebook.presto.hive.S3SelectLineRecordReader.readLine(S3SelectLineRecordReader.java:114)
        at com.facebook.presto.hive.S3SelectLineRecordReader.next(S3SelectLineRecordReader.java:122)
        at com.facebook.presto.hive.S3SelectLineRecordReader.next(S3SelectLineRecordReader.java:43)
        at com.facebook.presto.hive.GenericHiveRecordCursor.advanceNextPosition(GenericHiveRecordCursor.java:200)
        ... 16 more
Caused by: com.github.rholder.retry.RetryException: Retrying failed to complete successfully after 5 attempts.
        at com.github.rholder.retry.Retryer.call(Retryer.java:120)
        at com.facebook.presto.hive.S3SelectLineRecordReader.readLine(S3SelectLineRecordReader.java:94)
        ... 19 more
Caused by: com.amazonaws.SdkClientException: Unable to execute HTTP request: java.lang.RuntimeException: Unexpected error: java.security.InvalidAlgorithmParameterException: the trus
tAnchors parameter must be non-empty
        at com.amazonaws.http.AmazonHttpClient$RequestExecutor.handleRetryableException(AmazonHttpClient.java:1116)
        at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeHelper(AmazonHttpClient.java:1066)
        at com.amazonaws.http.AmazonHttpClient$RequestExecutor.doExecute(AmazonHttpClient.java:743)
        at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeWithTimer(AmazonHttpClient.java:717)
        at com.amazonaws.http.AmazonHttpClient$RequestExecutor.execute(AmazonHttpClient.java:699)
        at com.amazonaws.http.AmazonHttpClient$RequestExecutor.access$500(AmazonHttpClient.java:667)
        at com.amazonaws.http.AmazonHttpClient$RequestExecutionBuilderImpl.execute(AmazonHttpClient.java:649)
        at com.amazonaws.http.AmazonHttpClient.execute(AmazonHttpClient.java:513)
        at com.amazonaws.services.s3.AmazonS3Client.invoke(AmazonS3Client.java:4365)
        at com.amazonaws.services.s3.AmazonS3Client.invoke(AmazonS3Client.java:4312)
        at com.amazonaws.services.s3.AmazonS3Client.selectObjectContent(AmazonS3Client.java:2923)
        at com.facebook.presto.hive.PrestoS3SelectClient.getRecordsContent(PrestoS3SelectClient.java:54)
        at com.facebook.presto.hive.S3SelectLineRecordReader.lambda$readLine$0(S3SelectLineRecordReader.java:97)
        at com.github.rholder.retry.AttemptTimeLimiters$NoAttemptTimeLimit.call(AttemptTimeLimiters.java:78)
        at com.github.rholder.retry.Retryer.call(Retryer.java:110)
        ... 20 more
Caused by: javax.net.ssl.SSLException: java.lang.RuntimeException: Unexpected error: java.security.InvalidAlgorithmParameterException: the trustAnchors parameter must be non-empty
        at sun.security.ssl.Alerts.getSSLException(Alerts.java:208)
        at sun.security.ssl.SSLSocketImpl.fatal(SSLSocketImpl.java:1964)
        at sun.security.ssl.SSLSocketImpl.fatal(SSLSocketImpl.java:1921)
        at sun.security.ssl.SSLSocketImpl.handleException(SSLSocketImpl.java:1904)
        at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:1420)
        at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:1397)
        at org.apache.http.conn.ssl.SSLConnectionSocketFactory.createLayeredSocket(SSLConnectionSocketFactory.java:396)
        at org.apache.http.conn.ssl.SSLConnectionSocketFactory.connectSocket(SSLConnectionSocketFactory.java:355)
        at com.amazonaws.http.conn.ssl.SdkTLSSocketFactory.connectSocket(SdkTLSSocketFactory.java:142)
        at org.apache.http.impl.conn.DefaultHttpClientConnectionOperator.connect(DefaultHttpClientConnectionOperator.java:142)
        at org.apache.http.impl.conn.PoolingHttpClientConnectionManager.connect(PoolingHttpClientConnectionManager.java:373)
        at sun.reflect.GeneratedMethodAccessor400.invoke(Unknown Source)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at com.amazonaws.http.conn.ClientConnectionManagerFactory$Handler.invoke(ClientConnectionManagerFactory.java:76)
        at com.amazonaws.http.conn.$Proxy184.connect(Unknown Source)
        at org.apache.http.impl.execchain.MainClientExec.establishRoute(MainClientExec.java:381)
        at org.apache.http.impl.execchain.MainClientExec.execute(MainClientExec.java:237)
        at org.apache.http.impl.execchain.ProtocolExec.execute(ProtocolExec.java:185)
        at org.apache.http.impl.client.InternalHttpClient.doExecute(InternalHttpClient.java:185)
        at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:83)
        at org.apache.http.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:56)
        at com.amazonaws.http.apache.client.impl.SdkHttpClient.execute(SdkHttpClient.java:72)
        at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeOneRequest(AmazonHttpClient.java:1238)
        at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeHelper(AmazonHttpClient.java:1058)
        ... 33 more
Caused by: java.lang.RuntimeException: Unexpected error: java.security.InvalidAlgorithmParameterException: the trustAnchors parameter must be non-empty
        at sun.security.validator.PKIXValidator.<init>(PKIXValidator.java:91)
        at sun.security.validator.Validator.getInstance(Validator.java:179)
        at sun.security.ssl.X509TrustManagerImpl.getValidator(X509TrustManagerImpl.java:312)
        at sun.security.ssl.X509TrustManagerImpl.checkTrustedInit(X509TrustManagerImpl.java:171)
        at sun.security.ssl.X509TrustManagerImpl.checkTrusted(X509TrustManagerImpl.java:184)
        at sun.security.ssl.X509TrustManagerImpl.checkServerTrusted(X509TrustManagerImpl.java:124)
        at sun.security.ssl.ClientHandshaker.serverCertificate(ClientHandshaker.java:1596)
        at sun.security.ssl.ClientHandshaker.processMessage(ClientHandshaker.java:216)
        at sun.security.ssl.Handshaker.processLoop(Handshaker.java:1052)
        at sun.security.ssl.Handshaker.process_record(Handshaker.java:987)
        at sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:1072)
        at sun.security.ssl.SSLSocketImpl.performInitialHandshake(SSLSocketImpl.java:1385)
        at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:1413)
        ... 53 more
Caused by: java.security.InvalidAlgorithmParameterException: the trustAnchors parameter must be non-empty
        at java.security.cert.PKIXParameters.setTrustAnchors(PKIXParameters.java:200)
        at java.security.cert.PKIXParameters.<init>(PKIXParameters.java:120)
        at java.security.cert.PKIXBuilderParameters.<init>(PKIXBuilderParameters.java:104)
        at sun.security.validator.PKIXValidator.<init>(PKIXValidator.java:89)
        ... 65 more

same3r · 2018-09-12T20:36:17Z

We were able to root cause the issue. It was caused due to default max number of file descriptors (4096) configured for the hosts.

For some queries, Presto will send many S3Select requests on one node. For eg, Presto sends 8000+ requests for TPCDS query 4. Each time we send a HTTPS request, we will read the truststore file, and then 8000 requests means we open 8000 fd for the files.

However, Linux has a limit on the max # of fd one user could open, which is set to 4096 for our test hosts, and this is the reason why we cannot read the truststore file, and see the above exception.

Raising the limit on the hosts resolves the issue. We are now able to execute all TPCDS-100 and TPCDS-1000 queries.

sudo /bin/sh -c 'echo "* soft nofile 50000" >> /etc/security/limits.conf' 
sudo /bin/sh -c 'echo "* hard nofile 50000" >> /etc/security/limits.conf'

We will add this configuration tuning in the documentation.

findepi · 2018-09-13T08:17:07Z

Each time we send a HTTPS request, we will read the truststore file, and then 8000 requests means we open 8000 fd for the files.

This doesn't sound good. The truststore file rarely changes (and I doubt we're obliged to react to its changes anyway).

kbajda · 2018-09-21T21:12:26Z

@sameer2086 The technical doc looks good to me. I believe to capture all concern well. Feel free to translate that into the actual end-user documentation likely as a section under https://prestodb.io/docs/current/connector/hive.html#amazon-s3-configuration

same3r · 2018-09-21T21:39:18Z

@sameer2086 The technical doc looks good to me. I believe to capture all concern well. Feel free to translate that into the actual end-user documentation likely as a section under https://prestodb.io/docs/current/connector/hive.html#amazon-s3-configuration

Thanks for reviewing the document @kbajda. Could you please point me to instructions on how to update Presto documentation. I don't have prior experience with that.

findepi · 2018-09-22T18:19:11Z

instructions on how to update Presto documentation

@sameer2086 this is hive.rst -- https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/connector/hive.rst

nezihyigitbasi

Thanks for working on this. I made another pass and left some more comments.

In general I have two comments:

We need proper user documentation.
The test coverage is low, please add more tests to increase it.

pom.xml

presto-hive/src/main/java/com/facebook/presto/hive/BackgroundHiveSplitLoader.java

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientConfig.java

presto-hive/src/main/java/com/facebook/presto/hive/HiveUtil.java

presto-hive/src/main/java/com/facebook/presto/hive/S3SelectLineRecordReader.java

presto-hive/src/main/java/com/facebook/presto/hive/S3SelectRecordCursor.java

pom.xml

presto-hive-hadoop2/pom.xml

presto-hive/pom.xml

findepi · 2018-09-28T19:33:28Z

The test coverage is low, please add more tests to increase it.

@nezihyigitbasi @sameer2086 i am afraid we cannot run integration tests on Travis (can we, somehow?)
It would be good to have such tests anyway, as we have for Glue metastore.
@sameer2086 would it make sense for Amazon to set up some automation to make sure AWS-related bits do not regress as we progress with development on master?

same3r · 2018-10-01T21:28:05Z

@nezihyigitbasi

We need proper user documentation.

I attached user documentation to the PR earlier. @kbajda reviewed that. We are currently working on incorporating that doc in https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/connector/hive.rst. Requesting you to please review the same and let us know incase any updates are required.

The test coverage is low, please add more tests to increase it.
Working on improving test coverage as per your feedback.

same3r · 2018-10-01T21:35:11Z

@sameer2086 would it make sense for Amazon to set up some automation to make sure AWS-related bits do not regress as we progress with development on master?

Thanks for the feedback @findepi. It makes a lot of sense to me to have automated tests in place to ensure we don't regress on AWS related Presto features. I will pass the feedback to the right teams and will request them to follow-up. Having said that I believe this shouldn't be a blocker for this PR, right?

findepi · 2018-10-02T08:38:16Z

It makes a lot of sense to me to have automated tests in place to ensure we don't regress on AWS related Presto features. I will pass the feedback to the right teams and will request them to follow-up. Having said that I believe this shouldn't be a blocker for this PR, right?

Right, it's not a blocker (unfortunately 😄 ).

findepi · 2018-10-18T21:39:20Z

@sameer2086 were you able to make any progress on tests infra (#11033 (comment))?

same3r · 2018-10-20T01:49:56Z

@sameer2086 were you able to make any progress on tests infra (#11033 (comment))?

@findepi I am not actively working on the tests infra project. Although, I did forward the feedback to the concerned team and they will review it in their sprint planning. I don't have a date for this though.

same3r · 2018-11-02T22:27:39Z

Published a new revision of the code after incorporating cr feedback.

@nezihyigitbasi, requesting another review. We are adding more tests in this Sprint and will update the diff soon with the requested unit tests.

nezihyigitbasi

I did another pass and it's getting close. My comments are mostly minor stuff. Thanks a lot for working on this PR and addressing all the comments.

You have an internal amazon link in the commit message detail.
I ran TestIonSqlQueryBuilder locally and testDecimalColumns test fails.

pom.xml

presto-docs/src/main/sphinx/connector/hive.rst

presto-hive/src/test/java/com/facebook/presto/hive/TestIonSqlQueryBuilder.java

presto-main/etc/catalog/hive.properties

nezihyigitbasi · 2018-11-06T00:07:41Z

presto-docs/src/main/sphinx/connector/hive.rst

@@ -341,6 +344,52 @@ the ``org.apache.hadoop.conf.Configurable`` interface from the Hadoop Java API,
 will be passed in after the object instance is created and before it is asked to provision or retrieve any
 encryption keys.

+S3SelectPushdown


/cc @electrum any comments for the doc (and the PR)?

same3r · 2018-11-09T23:37:07Z

@nezihyigitbasi, published a new revision for review. Requested unit tests need to be added. We are working on it, and will update the PR with them sometime next week.

same3r · 2018-11-16T20:01:39Z

@nezihyigitbasi Published a new revision with the requested unit tests.

nezihyigitbasi

@sameer2086 First, thanks for addressing all these comments. I made another pass and left some more comments. Currently this PR has lots of comments and github has a hard time loading it. I think we should create a new PR (and link to this one) and continue working on that.

Commit message details are longer than 72 characters and look weird on github.
Commit message includes a link to an amazon cr (cr https://code.amazon.com/reviews/CR-3642036
), please remove it.
I have left comments for the docs, but looks like we may need to clean it up a bit more.

presto-docs/src/main/sphinx/connector/hive.rst

nezihyigitbasi · 2018-11-20T00:08:11Z

presto-docs/src/main/sphinx/connector/hive.rst

+###############################################################
+By default, Presto uses PrestoS3FileSystem or EMRFS as its file system, and the setting ``fs.s3.maxConnections`` configuration specifies
+the maximum allowable client connections to Amazon S3 through EMRFS. By default, this is 5000.
+S3 Select Pushdown bypasses the filesystems when accessing Amazon S3 for predicate operations. In this case, the value of


What is a predicate operation?

Filter condition in the where clause of a query.

presto-hive/src/test/java/com/facebook/presto/hive/TestS3SelectRecordCursor.java

presto-docs/src/main/sphinx/connector/hive.rst

same3r · 2018-11-23T17:55:26Z

This PR is slow to work with due to large volume of comments. Thus, closing this and creating a new PR as per @nezihyigitbasi request.

New PR Link: #11970

Change-Id: I1498d5d7de5dc9f29f13e1239dbf57b8fc8d8bd8

findepi reviewed Jul 13, 2018

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/HiveS3Optimizer.java Outdated Show resolved Hide resolved

findepi reviewed Jul 13, 2018

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/IonSqlQueryBuilder.java Outdated Show resolved Hide resolved

presto-hive/src/main/java/com/facebook/presto/hive/S3SelectCsvRecordReader.java Show resolved Hide resolved

same3r force-pushed the s3optimizer-using-s3select branch from 549f31e to 428443c Compare July 13, 2018 23:53

same3r force-pushed the s3optimizer-using-s3select branch 2 times, most recently from 2f5e7c6 to 0fbb993 Compare July 17, 2018 20:27

amoghmargoor reviewed Jul 23, 2018

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/S3SelectLineRecordReader.java Outdated Show resolved Hide resolved

same3r force-pushed the s3optimizer-using-s3select branch from 0fbb993 to b457e31 Compare July 24, 2018 17:44

nezihyigitbasi reviewed Jul 27, 2018

View reviewed changes

shubhamtagra reviewed Jul 27, 2018

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/HiveS3Optimizer.java Outdated Show resolved Hide resolved

shubhamtagra reviewed Jul 28, 2018

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/S3SelectLineRecordReader.java Outdated Show resolved Hide resolved

facebook-github-bot added the CLA Signed label Jul 31, 2018

amoghmargoor reviewed Aug 3, 2018

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/S3SelectCsvRecordReader.java Outdated Show resolved Hide resolved

amoghmargoor reviewed Aug 3, 2018

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/PrestoS3Client.java Outdated Show resolved Hide resolved

amoghmargoor reviewed Aug 3, 2018

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/PrestoS3Client.java Outdated Show resolved Hide resolved

same3r force-pushed the s3optimizer-using-s3select branch from b457e31 to 3b1c7c2 Compare August 18, 2018 03:19

nezihyigitbasi added CLA Signed and removed CLA Signed labels Sep 28, 2018

nezihyigitbasi requested changes Sep 28, 2018

View reviewed changes

same3r force-pushed the s3optimizer-using-s3select branch from c6b6c36 to d18f384 Compare November 2, 2018 22:24

nezihyigitbasi requested changes Nov 6, 2018

View reviewed changes

same3r mentioned this pull request Nov 8, 2018

Update AWS SDK to version 1.11.445 #11880

Merged

same3r force-pushed the s3optimizer-using-s3select branch from d18f384 to bd95074 Compare November 9, 2018 22:31

same3r force-pushed the s3optimizer-using-s3select branch 2 times, most recently from 53d6576 to 8242a4e Compare November 16, 2018 19:53

same3r force-pushed the s3optimizer-using-s3select branch from 8242a4e to 5f7935e Compare November 17, 2018 00:36

nezihyigitbasi requested changes Nov 20, 2018

View reviewed changes

kbajda reviewed Nov 21, 2018

View reviewed changes

presto-docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved

Add support for query pushdown to S3 using S3 Select

6a71a5f

same3r force-pushed the s3optimizer-using-s3select branch from 5f7935e to 6a71a5f Compare November 23, 2018 17:50

same3r closed this Nov 23, 2018

same3r mentioned this pull request Nov 23, 2018

Add support for query pushdown to S3 using S3 Select #11970

Merged

same3r referenced this pull request in steveloughran/hadoop Dec 21, 2018

HADOOP-15364 Patch 001: S3 Select tests and CLI

875062e

Change-Id: I1498d5d7de5dc9f29f13e1239dbf57b8fc8d8bd8

Add support for query pushdown to S3 using S3Select #11033

Add support for query pushdown to S3 using S3Select #11033

Conversation

same3r commented Jul 13, 2018 • edited

facebook-github-bot commented Jul 13, 2018

same3r commented Jul 13, 2018

sopel39 commented Jul 13, 2018

same3r commented Jul 13, 2018

same3r commented Jul 14, 2018

hyandell commented Jul 18, 2018

findepi commented Jul 26, 2018

kbajda commented Jul 26, 2018

same3r commented Jul 26, 2018

nezihyigitbasi left a comment • edited

Choose a reason for hiding this comment

same3r commented Jul 27, 2018

kobokinken commented Jul 29, 2018 via email

facebook-github-bot commented Jul 31, 2018

same3r commented Aug 18, 2018 • edited

same3r commented Sep 12, 2018

findepi commented Sep 13, 2018

kbajda commented Sep 21, 2018

same3r commented Sep 21, 2018

findepi commented Sep 22, 2018

nezihyigitbasi left a comment

Choose a reason for hiding this comment

findepi commented Sep 28, 2018

same3r commented Oct 1, 2018 • edited

same3r commented Oct 1, 2018

findepi commented Oct 2, 2018

findepi commented Oct 18, 2018

same3r commented Oct 20, 2018

same3r commented Nov 2, 2018

nezihyigitbasi left a comment

Choose a reason for hiding this comment

nezihyigitbasi Nov 6, 2018

Choose a reason for hiding this comment

same3r commented Nov 9, 2018 • edited

same3r commented Nov 16, 2018

nezihyigitbasi left a comment

Choose a reason for hiding this comment

nezihyigitbasi Nov 20, 2018

Choose a reason for hiding this comment

same3r Nov 20, 2018

Choose a reason for hiding this comment

same3r commented Nov 23, 2018 • edited

same3r commented Jul 13, 2018 •

edited

nezihyigitbasi left a comment •

edited

same3r commented Aug 18, 2018 •

edited

same3r commented Oct 1, 2018 •

edited

same3r commented Nov 9, 2018 •

edited

same3r commented Nov 23, 2018 •

edited