Construct AddFileEntry instance only if necessary while reading the Delta Lake checkpoint

[findinpath]

Description

In case that there is checkpoint filtering applied and there are partition constraints which do not
match the partition values of the entry, avoid early to create the AddFileEntry instance.

Split the loading for the add entries from the Parquet checkpoint in two channels:

• one channels contains the partitionValues information
• the other channel contains everything else related to add

When building the add entry, check first the partition constraint to match against the partition values and only then load the add block to avoid unecessary resources spent on deserialization.

Used for testing a multi-part checkpoint file (25 parts , each around 12MB ~ 300MB in total) for testing this feature while storing the checkpoint in local MinIO and came up with the following results:

ADD retrieval of all entries
number of add entries: Optional[1235155]
checkpoint iterator completed positions: Optional[1235157]
checkpoint iterator completed bytes: Optional[323227977]
Elapsed Time in milliseconds: 17866

ADD partition pruning without current changes
number of add entries: Optional[18578]
checkpoint iterator completed positions: Optional[49290]
checkpoint iterator completed bytes: Optional[14099674]
Elapsed Time in milliseconds: 1056

ADD partition pruning with current changes (
number of add entries: Optional[18578]
checkpoint iterator completed positions: Optional[49290]
checkpoint iterator completed bytes: Optional[14099674]
Elapsed Time in milliseconds: 701

As can be seen from the analysis above, there can't be spotted any relevant improvement in terms of IO with this change.
That's because the Parquet page is already loaded because it contains at least one entry matching the partition predicate.
This is why the checkpoint iterator still does read the same amount of bytes as the baseline in case of applying the partition pruning.

However, it can be seen that the elapsed number of milliseconds decreases in case of using this change because there is less deserialization performed.

Tested as well with a more permissive filter and haven't actually spotted bigger improvements than ~ 0.5s in terms of elapsed time between the base line and the current changes.

ADD partition pruning without current changes
number of add entries: Optional[210575]
checkpoint iterator completed positions: Optional[248524]
checkpoint iterator completed bytes: Optional[66021553]
Elapsed Time in milliseconds: 3992

ADD partition pruning with current changes
number of add entries: Optional[210575]
checkpoint iterator completed positions: Optional[248524]
checkpoint iterator completed bytes: Optional[66021553]
Elapsed Time in milliseconds: 3532

Additional context and related issues

This change builds on top of #19588

Release notes

( ) This is not user-visible or is 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:

# Delta
* Improve query planning performance on delta lake tables. ({issue}`19795`)

[findinpath]

That's not enough - I'm seeing while debugging buildAddEntry that the blocks are all not lazy.

[findinpath]

Even if we use lazy blocks, the block behind the lazy block is the monolithic structure corresponding to the add entry.
If we want to actually avoid reading from parquet add related fields for the entries which are not relevant, we need to refactor the way we are reading the checkpoint.

[findepi]

Even if we use lazy blocks, the block behind the lazy block is the monolithic structure

yes

If we want to actually avoid reading from parquet add related fields for the entries which are not relevant,

i don't think the parquet reader supports that, or can support that, given how values are encoded in Parquet.

[findinpath]

i don't think the parquet reader supports that,

I'm pointing here towards using a similar method as for dereference pushdown.

[findepi]

Does this technically conflict with #19848 ?

[findinpath]

Does this technically conflict with #19848 ?

No, it shouldn't.

The stats to be read with #19848 are build statically.
This change is mostly about splitting in two separate channels the reading from Parquet for the add entries, specifically:

• partitionValues
• everything else

[findepi]

"Avoid early to construct the AddFileEntry"

[findepi]

"Create CheckPointFieldExtractor instance only if necessary"

[findepi]

requireNonNull becomes redundant

[findepi]

|| addPartitionValuesBlock.isNull(pagePosition) wasn't here, right?
why is it being added?

[findinpath]

The isNull check for addPartitionValuesBlock is being added because there are now 2 blocks (instead of initially 1) from which we build up the add entry and they need to be consistent.
Changing though slightly the logic. Thank you for raising this.

[findepi]

Is this useful, especually considering Block.toString?

or to be removed?

[findinpath]

Are you implying here to eventually remove all the debug statements from the CheckpointEntryIterator class?
Potential follow-up?

[raunaqmorarka]

We could log the field count of RowBlock once per Page somewhere, it looks unnecessary to log it for every position in a Page

[findinpath]

Rebased on master to handle conflicts with #19848

[findepi]

Rebased on master to handle conflicts with #19848

that's why i asked #19795 (comment) :)

[findinpath]

Rebased on master to adress code conflicts.

[raunaqmorarka]

Please reword the commit message to

Construct AddFileEntry lazily

When checkpoint filtering is applied
and there are partition constraints which do not
match the partition values of the entry, avoid
eagerly constructing `AddFileEntry`.

[findinpath]

Modified to

Construct AddFileEntry instance only if necessary

When checkpoint filtering is applied and there
are partition constraints which do not match the
partition values of the entry, avoid eagerly to
construct `AddFileEntry` instances.