Put S3 Select for TEXTFILE behind an experimental flag #18102

alexjo2144 · 2023-06-30T16:07:37Z

Description

Fixes some correctness issues in JSON pushdown related to quote characters and gate pushdown for TEXTFILE (using CSV in S3 Select) behind a separate HiveConfig property.

Additional context and related issues

Relates to: #17775
Based on: #17563

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Hive
* Fix correctness issue when using S3 Select and query predicates include a quote character. ({issue}`17775`)
* Fix correctness issue when using S3 Select and query predicates include a decimal column. ({issue}`17775`)
* Add an additional opt-in property to enable S3 Select for TEXTFILE tables. ({issue}`17775`)

findepi · 2023-07-03T11:28:18Z

/test-with-secrets sha=8017b1f1e61240a2150029cdfa4ed3e907274876

github-actions · 2023-07-03T11:52:35Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/5444035845

findepi · 2023-07-06T07:02:43Z

/test-with-secrets sha=38bd9f9a86b5a214573ff3c3359c902980e470b1

github-actions · 2023-07-06T08:27:22Z

The CI workflow run with tests that require additional secrets has been started: https://github.com/trinodb/trino/actions/runs/5472865441

electrum

Capitalize "S3 Select" in commit titles

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

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

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

plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseTestHiveOnDataLake.java

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

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

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConfig.java

plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseTestHiveOnDataLake.java

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

findepi

"Disable S3 Select pushdown on decimal columns"

this requires blind correctness tests.

plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseTestHiveOnDataLake.java

findepi

"Put S3 Select for CSV files behind an experimental flag"

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

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

findepi · 2023-07-07T08:32:06Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConfig.java

+    }
+
+    @Config("hive.s3select-pushdown.experimental-pushdown-enabled")
+    @ConfigDescription("Enable query pushdown to TEXTFILE tables using the AWS S3 Select service. Requires 'hive.s3select-pushdown.enabled' also be set.")


Omit Requires 'hive.s3select-pushdown.enabled' also be set. here. It's kind of obvious.

Also, it's possible to enforce using @AssertTrue annotation (add a test, since recent changes from javax to jakarta broke some validations)

Omit Requires 'hive.s3select-pushdown.enabled' also be set. here. It's kind of obvious.

Sure

Also, it's possible to enforce using @AssertTrue annotation (add a test, since recent changes from javax to jakarta broke some validations)

I'd rather not require this, as the main toggle can also be enabled/disabled using a session property.

this makes things more complicated, agreed. so in the code we cannot assume one is enabled only IF the other is
however, that doesn't mean we should be accepting catalog configuration that makes no sense. no need to & easy to address

I think it still makes sense to have the main on/off toggle default to off but have textfile pushdown enabled. For example if you want to test the experimental feature using the session property but want it to stay disabled by default.

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

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

findepi · 2023-07-07T08:38:08Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseTestHiveOnDataLake.java

+            // These two should return a result, but incorrectly return nothing
+            assertThat(query(withS3SelectPushdown, "SELECT id FROM " + table.getName() + " WHERE string_t ='a,comma'")).returnsEmptyResult();


verify in the test that results are incorrect: run a query without s3select pushdown and assert actual results, so that the test self-validates the comment "... but incorrectly return ..."

We also need a blind correctness test ensuring that when S3Select is enabled (but the experimental pushdown is not), results are correct.

In other words -- which test would fail if I simply change HiveConfig#s3SelectExperimentalPushdownEnabled default value? Which test prevents me from enabling it before correctness problems are fixed?

Thanks, adding the first bullet exposed a bug in the native textfile reader implementation, submitted a separate issue: #18215

We also need a blind correctness test ensuring that when S3Select is enabled (but the experimental pushdown is not), results are correct.

Added a separate test class for this, since the experimental flag is not configurable by a session property.

Minio tests produced the correct results, however tests against a real S3 bucket did not.

electrum · 2023-07-10T20:33:32Z

Can you check the test failures?

alexjo2144 · 2023-07-10T20:49:02Z

Yeah, I forgot to check my RST formatting. Should be good this time.

findepi · 2023-07-21T11:27:47Z

/test-with-secrets sha=c65d4ba9834ed5433ba23533f35d1b0eb35cd641

github-actions · 2023-07-21T14:25:21Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/5622741842

S3 Select queries on CSV files are shown to have correctness problems. JSON files can still be enabled/disabled using the existing config and session properties.

alexjo2144 · 2023-07-21T19:23:39Z

Test failure was because I forgot to update one of the tests when I changed the config property name...

alexjo2144 · 2023-07-24T20:50:25Z

@electrum or @ebyhr can I get another test with secrets kicked off?

electrum · 2023-07-24T22:25:57Z

/test-with-secrets sha=eeda5941b6e05f7ec28f4d07adbf7c3b146eca7c

github-actions · 2023-07-24T23:53:01Z

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/5650768931

alexjo2144 · 2023-07-25T18:50:51Z

Failure is unrelated, had to do with: #18227

findepi · 2023-08-15T06:08:00Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseTestHiveOnDataLake.java

@@ -1799,14 +1800,14 @@ public void testS3SelectPushdown(String tableProperties)
                .setCatalogSessionProperty("hive", "insert_existing_partitions_behavior", "APPEND")


now that this class grew beyond its initial purpose, this should be the class default and the insert-overwrite tests should pick OVERWRITE

cla-bot bot added the cla-signed label Jun 30, 2023

alexjo2144 requested a review from electrum June 30, 2023 16:07

github-actions bot added docs tests:hive hive Hive connector labels Jun 30, 2023

alexjo2144 force-pushed the hive/s3-select-config branch 3 times, most recently from a61f404 to 8017b1f Compare June 30, 2023 19:32

alexjo2144 force-pushed the hive/s3-select-config branch from 8017b1f to 38bd9f9 Compare July 5, 2023 16:58

findepi changed the title ~~Put s3 Select for CSV files behind an experimental flag~~ Put S3 Select for CSV files behind an experimental flag Jul 6, 2023

electrum approved these changes Jul 6, 2023

View reviewed changes

electrum changed the title ~~Put S3 Select for CSV files behind an experimental flag~~ Put S3 Select for TEXTFILE behind an experimental flag Jul 6, 2023

electrum mentioned this pull request Jul 6, 2023

Correctness issues in S3 Select Pushdown #17775

Closed

alexjo2144 force-pushed the hive/s3-select-config branch from 38bd9f9 to e72bb89 Compare July 6, 2023 20:05

findepi reviewed Jul 7, 2023

View reviewed changes

plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseTestHiveOnDataLake.java Show resolved Hide resolved

findepi reviewed Jul 7, 2023

View reviewed changes

alexjo2144 force-pushed the hive/s3-select-config branch from e72bb89 to 7d4dad8 Compare July 7, 2023 19:49

alexjo2144 added 2 commits July 10, 2023 14:00

Fix JSON S3 select queries with quote characters

766a546

Disable S3 Select pushdown on decimal columns

3e73bca

Minio tests produced the correct results, however tests against a real S3 bucket did not.

alexjo2144 force-pushed the hive/s3-select-config branch from 7d4dad8 to 3557abf Compare July 10, 2023 18:03

alexjo2144 force-pushed the hive/s3-select-config branch from 3557abf to d5aea6b Compare July 10, 2023 20:46

alexjo2144 requested a review from findepi July 14, 2023 19:02

Put S3 Select for CSV files behind an experimental flag

eeda594

S3 Select queries on CSV files are shown to have correctness problems. JSON files can still be enabled/disabled using the existing config and session properties.

alexjo2144 force-pushed the hive/s3-select-config branch from c65d4ba to eeda594 Compare July 21, 2023 19:22

electrum merged commit 406861a into trinodb:master Jul 26, 2023

github-actions bot added this to the 423 milestone Jul 26, 2023

colebow mentioned this pull request Aug 2, 2023

Add Trino 423 release notes #18496

Merged

findepi reviewed Aug 15, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put S3 Select for TEXTFILE behind an experimental flag #18102

Put S3 Select for TEXTFILE behind an experimental flag #18102

alexjo2144 commented Jun 30, 2023 •

edited

Loading

findepi commented Jul 3, 2023

github-actions bot commented Jul 3, 2023 •

edited

Loading

findepi commented Jul 6, 2023

github-actions bot commented Jul 6, 2023

electrum left a comment

findepi left a comment

findepi left a comment

findepi Jul 7, 2023

alexjo2144 Jul 7, 2023

findepi Jul 10, 2023

alexjo2144 Jul 18, 2023 •

edited

Loading

findepi Jul 7, 2023

alexjo2144 Jul 10, 2023

alexjo2144 Jul 10, 2023

electrum commented Jul 10, 2023

alexjo2144 commented Jul 10, 2023

findepi commented Jul 21, 2023

github-actions bot commented Jul 21, 2023 •

edited

Loading

alexjo2144 commented Jul 21, 2023

alexjo2144 commented Jul 24, 2023

electrum commented Jul 24, 2023

github-actions bot commented Jul 24, 2023 •

edited

Loading

alexjo2144 commented Jul 25, 2023

findepi Aug 15, 2023

		// These two should return a result, but incorrectly return nothing
		assertThat(query(withS3SelectPushdown, "SELECT id FROM " + table.getName() + " WHERE string_t ='a,comma'")).returnsEmptyResult();

		@@ -1799,14 +1800,14 @@ public void testS3SelectPushdown(String tableProperties)
		.setCatalogSessionProperty("hive", "insert_existing_partitions_behavior", "APPEND")

Put S3 Select for TEXTFILE behind an experimental flag #18102

Put S3 Select for TEXTFILE behind an experimental flag #18102

Conversation

alexjo2144 commented Jun 30, 2023 • edited Loading

Description

Additional context and related issues

Release notes

findepi commented Jul 3, 2023

github-actions bot commented Jul 3, 2023 • edited Loading

findepi commented Jul 6, 2023

github-actions bot commented Jul 6, 2023

electrum left a comment

Choose a reason for hiding this comment

findepi left a comment

Choose a reason for hiding this comment

findepi left a comment

Choose a reason for hiding this comment

findepi Jul 7, 2023

Choose a reason for hiding this comment

alexjo2144 Jul 7, 2023

Choose a reason for hiding this comment

findepi Jul 10, 2023

Choose a reason for hiding this comment

alexjo2144 Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

findepi Jul 7, 2023

Choose a reason for hiding this comment

alexjo2144 Jul 10, 2023

Choose a reason for hiding this comment

alexjo2144 Jul 10, 2023

Choose a reason for hiding this comment

electrum commented Jul 10, 2023

alexjo2144 commented Jul 10, 2023

findepi commented Jul 21, 2023

github-actions bot commented Jul 21, 2023 • edited Loading

alexjo2144 commented Jul 21, 2023

alexjo2144 commented Jul 24, 2023

electrum commented Jul 24, 2023

github-actions bot commented Jul 24, 2023 • edited Loading

alexjo2144 commented Jul 25, 2023

findepi Aug 15, 2023

Choose a reason for hiding this comment

alexjo2144 commented Jun 30, 2023 •

edited

Loading

github-actions bot commented Jul 3, 2023 •

edited

Loading

alexjo2144 Jul 18, 2023 •

edited

Loading

github-actions bot commented Jul 21, 2023 •

edited

Loading

github-actions bot commented Jul 24, 2023 •

edited

Loading