Fix filter pushdown for DELETE queries

[mbasmanova]

== NO RELEASE NOTE ==

[bhhari]

LGTM

[bhhari]

@mbasmanova i guess you need to rebase, unrelated build issue

[ajaygeorge]

do we need a requireNonNull check for the arguments here or are they handled in the caller?

[mbasmanova]

From what I've seen in Presto codebase, requireNonNull is used only in constructors.

[ajaygeorge]

sure, the question was more geared towards do we need to null check using either requireNonNull or explicit arg==null .

[ajaygeorge]

can you please explain what this //TODO means.
Is this a failing test that needs to be fixed later on or some functionality that needs to be added.

[mbasmanova]

@ajaygeorge This is missing functionality. This query should work fine, but it doesn't. Since this is existing behavior, I'm keeping it like that for pushdown-query code path.

[ajaygeorge]

As a side note, I have always wondered what is the reasoning behind presto's choice behind accepting Optionals as arguments. As referenced in this sonar code quality check rule Optionals as arguments was probably not intended by the authors.

The Java language authors have been quite frank that Optional was intended for use only as a return type, as a way to convey that a method may or may not return a value.

And for that, it's valuable but using Optional on the input side increases the work you have to do in the method without really increasing the value. 

With an Optional parameter, you go from having 2 possible inputs: null and not-null, to three: null, non-null-without-value, and non-null-with-value. Add to that the fact that overloading has long been available to convey that some parameters are optional, and there's really no reason to have Optional parameters.

[mbasmanova]

@ajaygeorge

As a side note,

I don't have an answer to that.

[ajaygeorge]

@ajaygeorge

As a side note,

I don't have an answer to that.

Do we even need an Optional argument here.? 🤔
The usage of the Optional in this context is basically checking for isPresent and then take an action which is very similar to a null check and then proceeding.

If it has been the standard that we have been following then it makes sense to keep it as is.

LGTM as well !

[mbasmanova]

@ajaygeorge

If it has been the standard that we have been following then it makes sense to keep it as is.

Yes, this is how it is currently done in the codebase.

[highker]

one minor comment

[highker]

Can we have an enumeration of comments to explain the cases when metadata delete can happen and when it can't? By looking at the code, it seems that it's not an all or nothing call given the filter could be "simple" or very "complicated" but turning to be simple if we do constant folding or even evaluation. Of course, there is a limit we can push. We can only inference the delete if the filter is not "that complicated" .

[mbasmanova]

@highker James, thank you review.

Strictly speaking we should be able to support metadata delete for any filter that doesn't touch non-partition columns. Today we only support it for range filters on partition columns though (regardless of whether filter pushdown is on or off). I'll add a comment and a TODO.

// Allow metadata delete for range filters on partition columns.
// TODO Add support for metadata delete for any filter on partition columns.