[Iceberg] Add table and session property for split size

[ZacBlanco]

Description

Adds a session property for target split size.

Closes #24419

Motivation and Context

Makes it easier to do performance debugging by setting the desired split size on a per-query basis.

Impact

New configuration property.

Test Plan

unit tests

Contributor checklist

• Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
• PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
• Documented new properties (with its default value), SQL syntax, functions, or other functionality.
• If release notes are required, they follow the release notes guidelines.
• Adequate tests were added if applicable.
• CI passed.

Release Notes

== RELEASE NOTES ==

Iceberg Connector Changes
* Add ``target_split_size_bytes`` session property
* Add ``read.split.target-size`` table property

[steveburnett]

Thanks for the release note! Nit formatting suggestion.

== RELEASE NOTES ==

Iceberg Connector Changes
* Add ``target_split_size session`` property. :pr:`24417`

Consider documenting the new session property, perhaps in
https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/connector/iceberg.rst#session-properties .

[steveburnett]

LGTM! (docs)

Pull branch, local doc build, looks good. Thanks!

[hantangwangd]

Bring one thing for discussion. From a high level perspective, considering that a complex sql (like tpcds sqls) may involve multiple tables, is it a suitable way to set a unified session level target split size to override their own split size? Or should we respect each table's own split size (if exist) more than the session level uniform value?

[yingsu00]

In hive, the counter part is MAX_SPLIT_SIZE = "max_split_size";. Can we rename this one the same as Hive? Introducing different names for the same thing would make the users confused. We could also name both as "target_split_size", and the good thing about the "target_split_size" name is that it conforms with the Iceberg library, but "hive.max_split_size" is more accurate that its split sizes cannot exceed this number. For Iceberg, it is also the max split size even though it's named as "target" split size. (See org/apache/iceberg/FixedSizeSplitScanTaskIterator.java)

[ZacBlanco]

Iceberg and Hive function differently. Splits are not calculated in the same way. As such, the target size property does not refer to the absolute maximum size of the split. FixedSizeSplitScanTaskIterator only applies to files which don't support offsets. Parquet and ORC do support offsets, so they most likely won't use that class, but instead the OffsetsWareSplitScanTaskIterator from the iceberg library.

Either way, these are implementation details. If the property meant "max split size" it would be documented as such. This property does not represent the max split size and does not guarantee that splits will be below this size

[yingsu00]

Ok. In this case, can we add some clarification here: "Unlike hive.max_split_size, this can be smaller or greater than the actual split size"

[ZacBlanco]

I've added this to the description for the table property. The session property refers to the table property, so I think it should be clear enough

[yingsu00]

It seems Presto convention does not use the prefixes from the Iceberg lib. E.g. "write.metadata.metrics.max-inferred-column-defaults" was just named "metrics_max_inferred_column" table property. While I do think the notion with prefixes is clearer, I think it's better to name it "split_target_size" for now. Please also add explanation here this correspond to read.split.target-size table property in Iceberg library.

Later on, we can send a proposal to use full iceberg property names since it will affect the users.

[ZacBlanco]

@hantangwangd and I have discussed this in another PR and decided that moving forward for new properties we will use the iceberg property names. We will introduce backwards-compatible property names for the properties which were already introduced and slowly phase the old names out over the next few releases. I have filed this issue to address it: #24483 It includes the relevant context

[yingsu00]

I agree that using Iceberg names is clearer. But ideally we should make changes for #24483 before directly using the new name. When and who will be working on it? Do you think you can send PR for that sooner?

[ZacBlanco]

Sure, I can try to get that PR out this week. I can definitely have it in soon, but since most users won't see any changes until the 292 release, I am in the camp that it would be fine to not hold up this PR to align the old property names

[yingsu00]

Sounds good! It should be good as long as it's before the 292 release.

[ZacBlanco]

Here is a draft PR for introducing the deprecation of table property names: #24581

It is still WIP. Just needs some tests

[yingsu00]

Here is a draft PR for introducing the deprecation of table property names: #24581

It is still WIP. Just needs some tests

Great! Ping me when it's ready.

[steveburnett]

New release note guidelines. Please remove the manual PR link in the following format from the release note entries for this PR.

:pr:`12345`

I have updated the Release Notes Guidelines to remove the examples of manually adding the PR link.

[yingsu00]

@ZacBlanco @steveburnett Shall we also mention we're adding a Iceberg table property read.split.target-size?

[steveburnett]

@ZacBlanco @steveburnett Shall we also mention we're adding a Iceberg table property read.split.target-size?

Thanks for the catch @yingsu00! Yes, doc should be added for all new properties introduced in a PR.

[ZacBlanco]

@yingsu00 there is already a new entry in the doc for read.split.target-size in the table properties. I will add it to the release note though.