Add S3 Select pushdown for JSON files by preethiratnam · Pull Request #13354 · trinodb/trino

preethiratnam · 2022-07-26T09:44:02Z

Enable S3 Select pushdown for JSON files.

This change includes some refactoring of the IonSqlQueryBuilder to support query generation of both CSV and JSON files. Also upgrades the Hadoop testing image to 3.1 as it contains the JSONSerDe class.

The pushdown logic is restricted to only base columns, similar to CSV. S3 Select does support nested column filtering on JSON files, which we plan to enable in a later PR (to keep this PR's scope limited).

Description

Is this change a fix, improvement, new feature, refactoring, or other?

New feature

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Trino Hive Connector (S3 Select)

How would you describe this change to a non-technical end user or system administrator?

Enable S3 Select pushdown for JSON files.

Related issues, pull requests, and links

None

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# Section
*  Add Amazon S3 Select pushdown for JSON files.

plugin/trino-hive-hadoop2/conf/hive-tests-defaults.sh

plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3select/S3SelectSerDeDataTypeMapper.java

arhimondr · 2022-07-28T16:53:19Z

...trino-hive/src/main/java/io/trino/plugin/hive/s3select/S3SelectLineRecordReaderProvider.java

Similar here. I wonder if this method should simply be static (or maybe even a private method of S3SelectRecordCursorProvider)

Yes, I made it a static method in the new commit. Did not want to make it a part of the S3SelectRecordCursorProvider as I wanted to keep the RecordReader separate from it.

plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3select/csv/CsvIonSqlQueryBuilder.java

plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3select/TestIonSqlQueryBuilder.java

arhimondr

LGTM % nits

plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3select/IonSqlQueryBuilder.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3select/S3SelectRecordCursorProvider.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3select/S3SelectSerDeDataTypeMapper.java

arhimondr · 2022-07-29T15:03:27Z

Please update the commit messages according to the guildelines: https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

arhimondr · 2022-08-01T15:07:00Z

@preethiratnam We usually don't terminate a commit message with a dot. Could you please remove it? Once done I should be able to merge this.

preethiratnam · 2022-08-01T16:16:00Z

@arhimondr Updated the commit message, thank you for reviewing!

arhimondr · 2022-08-01T21:19:26Z

@preethiratnam Merged, thanks!

colebow · 2022-08-01T21:54:24Z

Do we need user-facing docs for how to leverage this on this page?

@arhimondr

arhimondr · 2022-08-01T22:00:39Z

@preethiratnam From what I understand it's an optimization and should be transparent to the end user, right?

preethiratnam · 2022-08-02T07:15:54Z

Yes, that's right. JSON files will be pushed down to Select whenever hive.s3select-pushdown.enabled is set to true. @arhimondr

hashhar · 2022-08-03T06:42:42Z

plugin/trino-hive-hadoop2/bin/run_hive_s3_tests.sh

 cleanup_hadoop_docker_containers
+
+# Use Hadoop version 3.1 for S3 tests as the JSON SerDe class is not available in lower versions.
+export HADOOP_BASE_IMAGE="ghcr.io/trinodb/testing/hdp3.1-hive"


FYI @findepi (in case you weren't already aware).

Thanks @hashhar

If I am reading this correctly, this removes the point of running these tests in a matrix, which we still do

trino/.github/workflows/ci.yml

Lines 207 to 208 in b646676

source plugin/trino-hive-hadoop2/conf/hive-tests-${{ matrix.config }}.sh &&

plugin/trino-hive-hadoop2/bin/run_hive_s3_tests.sh

@arhimondr was it a conscious decision?
did you want to remove matrix from that CI job?

cc @nineinchnick @alexjo2144

Yes, we should either make testGetRecordsJson run only on HDP3 (via surefire config probably) or remove matrix (we probably shouldn't do this to maintain coverage across distros).

Yeah, I think we should maintain the matrix. The Hive version may matter for what exactly gets created on S3.

@arhimondr @preethiratnam
can you please remove export HADOOP_BASE_IMAGE=... hack from run_hive_s3_tests.sh?

Hi @findepi @hashhar I want to use the HADOOP_BASE_IMAGE 3.1 to run Hive S3 tests. This is because the JSONSerDe class is not available in the 2.6 version.

Is there a better way of configuring the JSON tests to use 3.1 version?

@preethiratnam We need to use test exclusions via maven profiles for that.

Thanks, I'll raise a new PR to fix this.

@findepi No, unfortunately it wasn't a conscious decision. I think I misread and assumed that it will only change the default value for S3 tests.

Created #13486 to fix this.

cla-bot bot added the cla-signed label Jul 26, 2022

preethiratnam mentioned this pull request Jul 26, 2022

Add JSON support to Amazon S3 Select #13337

Closed

github-actions bot added the tests:hive label Jul 26, 2022

preethiratnam requested review from arhimondr, fuatbasik and pettyjamesm July 26, 2022 14:39

arhimondr reviewed Jul 28, 2022

View reviewed changes

preethiratnam requested a review from arhimondr July 29, 2022 14:57

arhimondr approved these changes Jul 29, 2022

View reviewed changes

preethiratnam requested a review from arhimondr July 29, 2022 18:30

github-actions bot added docs jdbc Relates to Trino JDBC driver release-notes labels Aug 1, 2022

arhimondr approved these changes Aug 1, 2022

View reviewed changes

Add S3 Select pushdown for JSON files

64de601

pettyjamesm approved these changes Aug 1, 2022

View reviewed changes

arhimondr merged commit b97e7fa into trinodb:master Aug 1, 2022

github-actions bot added this to the 392 milestone Aug 1, 2022

colebow mentioned this pull request Aug 1, 2022

Add Trino 392 release notes #13342

Closed

hashhar reviewed Aug 3, 2022

View reviewed changes

preethiratnam mentioned this pull request Aug 3, 2022

Enable only Hadoop 3.1 testing version for S3 Select JSON tests #13486

Merged

dnanuti mentioned this pull request Nov 15, 2022

Hive Connector with Amazon S3 documentation updates #15035

Merged

	source plugin/trino-hive-hadoop2/conf/hive-tests-${{ matrix.config }}.sh &&
	plugin/trino-hive-hadoop2/bin/run_hive_s3_tests.sh

Conversation

preethiratnam commented Jul 26, 2022

Description

Related issues, pull requests, and links

Documentation

Release notes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

arhimondr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arhimondr commented Jul 29, 2022

Uh oh!

arhimondr commented Aug 1, 2022

Uh oh!

preethiratnam commented Aug 1, 2022

Uh oh!

arhimondr commented Aug 1, 2022

Uh oh!

colebow commented Aug 1, 2022

Uh oh!

arhimondr commented Aug 1, 2022

Uh oh!

preethiratnam commented Aug 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants