Filter Iceberg splits based on $path column predicates

[ebyhr]

Description

Filter Iceberg splits based on $path column predicates
Fixes #12785

Documentation

(x) No documentation is needed.

Release notes

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

# Iceberg
* Support filtering splits based on `$path` column predicates. ({issue}`12785`)
* Return `$path` column without encoding when the path contains double slashes on S3. ({issue}`13012`)

[alexjo2144]

Nice

[ebyhr]

CI hit #12950

[findepi]

This should make only $path column enforced, and if it is any other metadata column, we should still consider this unsupported.

[findepi]

That doesn't need to return Optional.
it's either Domain.none(), Domain.all(), or some specific Domain

[findepi]

I don't think hadoopPath is appropriate here. It appends some #... in case of s3://,
but that suffix is not -- i hope so -- returned when doing SELECT "$path" FROM t

[findepi]

can we have some test coverage with s3?

sth like

• create a table with two files
• get "$path" for one of the files
• select where $path = selected_path
• verify we get data from that one file only

[ebyhr]

I think we should use hadoopPath() unless fixing $path result.

IcebergSplitSource
    private IcebergSplit toIcebergSplit(FileScanTask task)
    {
        return new IcebergSplit(
                hadoopPath(task.file().path().toString()),
↓

IcebergPageSourceProvider
        ReaderPageSource dataPageSource = createDataPageSource(
                session,
                hdfsContext,
                new Path(split.getPath()),

...
                else if (column.isPathColumn()) {
                    columnAdaptations.add(ColumnAdaptation.constantColumn(nativeValueToBlock(FILE_PATH.getType(), utf8Slice(path.toString()))));
                }

It appends # when the bucket starts with / likes s3:///bucket (not always) as you already know (#11998)

I tried if we can create such bucket in S3 & Minio, but failing by invalid bucket name.

~ aws s3api create-bucket --bucket /ebyhr-test

Parameter validation failed:
Invalid bucket name "/ebyhr-test": Bucket name must match the regex "^[a-zA-Z0-9.\-_]{1,255}$" or be an ARN matching the regex "^arn:(aws).*:(s3|s3-object-lambda):[a-z\-0-9]*:[0-9]{12}:accesspoint[/:][a-zA-Z0-9\-.]{1,63}$|^arn:(aws).*:s3-outposts:[a-z\-0-9]+:[0-9]{12}:outpost[/:][a-zA-Z0-9\-]{1,63}[/:]accesspoint[/:][a-zA-Z0-9\-]{1,63}$"

https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html

[findepi]

the #... suffix is internal thing, not to be exposed to users.

[findepi]

Let's move this logic to the place which applies conditions on metadata columns, so that we know no constraint gets ignored.

thus, here it would be checkArgument(! isMetadataColumnId(columnHandle.getId()))
and IcebergSplitSource would divide TupleDomain into

1. $path domain --> handled directly there
2. the reset --> passed to toIcebergExpression

[findinpath]

The PR seems to be in a better shape now.
Please reorganize the commits to have them without fixups to continue the review process.

[phd3]

nit: we can defensively check if all metadata column predicates are consumed here, so that for any future additions for metadata-column predicates are actually applied in the splitsource. but don't feel too strongly - as we'll add tests anyway.

[phd3]

Q are we changing the behavior here? i.e. before this PR, effectively hadoopPath() was being returned here since that was propagated from the split, but now it's the actual path without encoding.

[ebyhr]

Yes, it's changing. I will mention in a release note. Relates to #13012 (comment)

[alexjo2144]

@homar added some code in #12704 which relies on an assumption that enforced predicates always fall on partition boundaries. After this change that will no longer be true. We need to figure out how to resolve that before merging this

[alexjo2144]

@homar how do you feel about changing finishOptimize to only remove delete files if the whole table is being optimized? I know we spent a while trying to figure out exactly when we could remove the files in a partition, but maintaining that logic feels pretty error prone.

[alexjo2144]

Maybe rename to dataColumnPredicate or nonMetadataPredicate to signal that it's a filtered version

[ebyhr]

Renamed to dataColumnPredicate.

[homar]

@homar how do you feel about changing finishOptimize to only remove delete files if the whole table is being optimized? I know we spent a while trying to figure out exactly when we could remove the files in a partition, but maintaining that logic feels pretty error prone.

I am fine with that tough I am afraid that if we do that then there a lot of clients will run into situation when delete files are never removed as they never optimize entire table.

[alexjo2144]

They should eventually get cleaned up by remove_orphan_files, I believe. As long as all of the data files referenced by the delete file have been optimized away.

[alexjo2144]

@ebyhr we can either merge this first, or you can include it in this PR. Whatever you'd like #13343

[ebyhr]

Just rebased on upstream to resolve conflicts. Let's merge #13343 first.

[alexjo2144]

We should do this for the file_modified_time column as well. Want to do that here, or in a separate PR?

[ebyhr]

I want to separate a PR for file_modified_time column.

[ebyhr]

@findepi Could you please review when you have time?

[alexjo2144]

Can you add more more test? Specifically

• Insert rows 4 times, resulting in files f1, f2, f3, f4
• Run an optimize WHERE $path = f1 OR $path = f2
• Run an optimize WHERE $path = f3 OR $path = f4
• Show table contains files f5 and f6

[ebyhr]

@alexjo2144 Added another test case.

[findepi]

should "Return $path without URL encoding in Iceberg" commit have any test changes/additions?

[ebyhr]

Ideally it should, but let me handle in #13457

[findepi]

url encoding (or lack of) should be exercisable independently from double slashes.
eg path containing %.

[ebyhr]

Not sure if I understood your suggestion correctly, but just adding % to file or directory name wouldn't work because it doesn't pass in !path.equals(hadoopPath.toString()) in hadoopPath().

[findepi]

Add a message

 "Constraint on an unexpected column %s", columnHandle

[findepi]

nit: can inline pathMatchesPredicate

[findepi]

if effective predicate is none, this method returns ALL domain.
The NONE tuple domain needs to be handled explicitly.

[findepi]

OTOH, the none is not expected here (should be filtered out earlier), so sth like

IcebergColumnHandle pathColumn = pathColumnHandle();
Domain domain = effectivePredicate.getDomains().orElseThrow(() -> new IllegalArgumentException("Unexpected NONE tuple domain"))
        .get(pathColumn);
if (domain == null) {
    return Domain.all(pathColumn.getType());
}
return domain;