Fix correctness issues in S3 Select pushdown

[alexjo2144]

Description

The IonSqlQueryBuilder would produce select queries which were not proper transormations of the given TupleDomain, leading to incorrect results when S3 Select was enabled.

When reading JSON files predicates like x IS NULL or x IS NOT NULL were evaluated as x = '' or x <> ''.

When reading TextFile data the query builder ignores the table's null_format field, instead assuming that null fields are encoded as the empty string.

Additional context and related issues

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 incorrect query results when s3 Select is used with `IS NULL` or `IS NOT NULL` predicates.
* Fix incorrect query results when using s3 Select and the table's null format is set.

[alexjo2144]

@findepi mind starting a run with secrets?

[alexjo2144]

Never mind, let me fix the IonSqlQueryBuilder tests first

[alexjo2144]

Updated with more Minio tests and some refactoring that makes the queries pushed into S3 a little cleaner.

There are still some failures when I run that test against real s3 manually though:

• Equality predicates on decimal types like WHERE decimal_t = 2.2 or WHERE decimal_t <= 2.2 produces incorrect results
• The test fails entirely when using the TextFile serde against real s3. It can't even parse the files

I think these changes are an improvement but there are still bugs to squash

[findepi]

We can run these tests agains MinIO for developers convenience, but will CI run them against real S3?
S3 Select pushdown looks like a complex functionality, so we shouldn't blindly assume S3-compatible is equivalent of S3.

[alexjo2144]

I'd like to do both Minio and S3 tests, but honestly I've been finding enough wrong with the feature I'm tempted to say we should just take it out.

[findepi]

Agreed. It's hard to take things, so lets rename to "experimental" the config toiggles and session properties

[alexjo2144]

@findepi can I get a run with secrets when you get a chance?

[findepi]

/test-with-secrets sha=fa43ed349ed9918bb0341250d97246e27d549043

[dnanuti]

There is another test file with the same naming: https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3select/TestS3SelectPushdown.java

Would you please rename this to make sure we don't exclude the tests above?

[alexjo2144]

Good catch, thanks!

[alexjo2144]

@dnanuti I think the main issue around some of these edge cases with whitespace or null encoding is that the s3 select CSV encoding is mapped in Trino to textfile. This works okay if, like in the test I wrote, the table explicitly defines a field separator, escape character, and null encoding, but if those are not explicitly defined csv and textfile have different defaults so data will not behave properly.

Handling for CSVs with whitespace or padding could be an issue, but before these changes data doesn't round-trip properly, even when written by Trino and read back by Trino. This is a much bigger problem.

[alexjo2144]

@dnanuti for this query, against a JSON table I've tried:
SELECT s.id, s.decimal_t FROM S3Object s WHERE s.decimal_t IS NOT NULL AND s.decimal_t = 2.20000
or
SELECT s.id, s.decimal_t FROM S3Object s WHERE s.decimal_t IS NOT NULL AND CAST(s.decimal_t AS DECIMAL(10, 5)) = 2.20000

But I get no rows back from either. Here's the table files:
json_table.tar.gz

I'd expect to get back the value 2

[dnanuti]

Thanks! I'll check tomorrow!

[dnanuti]

I didn't get the chance to look into this today, as there are other priorities right now. I'll open an issue in our backlog for further investigation. Thanks for calling this out! 👍

[dnanuti]

There are some Docker tests define that were running as part of the CI after re-enabling Select pushdown, where you can find tables creation statements that are not defining all the separators you mentioned.

Tables creation here:
https://github.com/trinodb/trino/blob/master/plugin/trino-hive-hadoop2/bin/run_hive_s3_tests.sh
Tests implementation here:
https://github.com/trinodb/trino/tree/master/plugin/trino-hive-hadoop2/src/test/java/io/trino/plugin/hive/s3select

Please enhance this if possible with the scenarios that were not covered before.

I'll double check about encoding tomorrow, but sounds reasonable.

[alexjo2144]

@findepi @electrum there are some lingering issues to fix but they're all preexisting things continued by #17775

When you get a chance another round of tests with secrets and review would be great.

[alexjo2144]

Thanks @findepi , all set

[alexjo2144]

Rebased for conflicts.

[findepi]

"fixup! Fix correctness issues in S3 Select pushdown" lgtm

[findepi]

/test-with-secrets sha=5ca6c5b9fdc468f3e180138f3f1fb216256d759b

[findepi]

i expect the above to fail per #17563 (comment)

[github-actions]

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

[findepi]

are ci / pt (default, suite-delta-lake-oss, ) failures related?