Add documentation for Iceberg support in PrestoCPP #24741

agrawalreetika · 2025-03-17T18:46:17Z

Description

Add documentation for Iceberg support in PrestoCPP

Motivation and Context

Add documentation for Iceberg support in PrestoCPP

Impact

Iceberg documentation improvement

Test Plan

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

== NO RELEASE NOTE ==

auden-woolfson

lgtm

steveburnett

Thanks for the doc! Some nits and suggestions, nothing major.

presto-docs/src/main/sphinx/connector/iceberg.rst

presto-docs/src/main/sphinx/presto-cpp.rst

presto-docs/src/main/sphinx/connector/iceberg.rst

yingsu00

Thanks @agrawalreetika! I see PrestoCPP and Presto C++ were mixed. Let's unify them to Presto C++

yingsu00 · 2025-03-18T01:24:18Z

presto-docs/src/main/sphinx/connector/iceberg.rst

                                                        are ``HIVE``, ``HADOOP``, and ``NESSIE`` and ``REST``.

-``iceberg.hadoop.config.resources``                     The path(s) for Hadoop configuration resources.
+``iceberg.hadoop.config.resources``                     The path(s) for Hadoop configuration resources.                                                  Yes


The Presto C++ Support value is missing

yingsu00 · 2025-03-18T01:26:32Z

presto-docs/src/main/sphinx/connector/iceberg.rst

                                                        as a join with the data of the equality delete files.

-``iceberg.enable-parquet-dereference-pushdown``         Enable parquet dereference pushdown.                          ``true``
+``iceberg.enable-parquet-dereference-pushdown``         Enable parquet dereference pushdown.                          ``true``                           Yes


The Presto C++ Support value is missing

yingsu00 · 2025-03-18T01:29:00Z

presto-docs/src/main/sphinx/connector/iceberg.rst

                                                        statistics file cache.
-======================================================= ============================================================= ============
+======================================================= ============================================================= ================================== =================== =========================================

 Table Properties


Can you please also add a section for the C++ support for Table Properties? Just saying it's not suported because write is not implemented yet

yingsu00 · 2025-03-18T01:36:20Z

presto-docs/src/main/sphinx/connector/iceberg.rst

@@ -1511,13 +1587,23 @@ schema evolution, such as adding, dropping, and renaming columns. With schema
 evolution, users can evolve a table schema with SQL after enabling the Presto
 Iceberg connector.

+Presto C++ Support


This is for Partition Column Transform. Could you please add Presto C++ support as well? Thanks!

yingsu00 · 2025-03-18T01:44:07Z

presto-docs/src/main/sphinx/presto-cpp.rst


-* Supports reading and writing of DWRF and PARQUET file formats, supports reading ORC file format.
+Hive Connector - Iceberg Table Support


I think the following structure would be clearer
Supported Connectors

Hive connector

Hive table support

Iceberg table support

TPCH connector

....

Fuzzer connector

agrawalreetika · 2025-03-18T09:15:34Z

@steveburnett @yingsu00 Thanks for your review. I have made the changes based on your comments. Please have a look at your convenience.

hantangwangd

Thanks for the doc, it's very helpful. Only a couple of nit and little thing.

hantangwangd · 2025-03-18T09:50:27Z

presto-docs/src/main/sphinx/connector/iceberg.rst

+
+``SELECT``                     Yes           Yes          Read is supported in Presto C++ including those with positional delete files.
+
+``INSERT INTO``                Yes           No


nit: duplicate with line 1155.

hantangwangd · 2025-03-18T10:01:30Z

presto-docs/src/main/sphinx/presto-cpp.rst

+
+* Only read operations are supported for Iceberg tables.
+
+* The Iceberg connector supports both V1 and V2 tables, including those with positional delete files, but does not support equality delete files.


From a reader's perspective, I'm a little confused about the Iceberg connector here, do you think it's more appropriate to use something like Hive connector for Iceberg or something else?

So we need to create an iceberg catalog in Prestissimo as well for accessing Iceberg tables but the underline implementation on Velox is via Hive connector itself which is why I thought of keeping Hive Connector as main header and then different table support.

I am open for suggestions here whatever we all think is more clear.
@yingsu00 What's your opinion on this?

Sorry, I didn't describe it clearly. What I mean is, the content structure Hive connector/Iceberg Table Support is no problem, just the words Iceberg connector here may bring a little confusion. If I were a newer, I might wonder whether there is an Iceberg connector for presto C++ that I haven't noticed.

@agrawalreetika : If we use Java side Iceberg connector or catalog code, then its better to call it Iceberg Connector. The reuse of HiveConnector is an internal detail imo.

I think we could call it Iceberg connector because it's in Presto C++ point of view, not Velox point of view. But it would be helpful to explain that the Presto C++ Iceberg connector and catalog is backed up by Velox HiveConnector.

@hantangwangd Thanks for the clarification. So here as @aditi-pandit & @yingsu00 mentioned from Presto C++ point of view it's going to be a different iceberg connector with something like this -

connector.name=iceberg

That is why I thought calling it iceberg connector would be better in the Presto C++ document.

@hantangwangd @aditi-pandit @yingsu00 Please take a look at the document, its already updated based on the points we discussed here.

Got the point, thank you all for your perspective @aditi-pandit @yingsu00 @agrawalreetika.

steveburnett

Thanks for the update! I pulled the updated branch, ran a new local doc build, and reviewed again.

I found a few nits that aren't directly related to your new content here that I hope we can fix here, I don't think that fixing them will add a lot of work or delay the PR.

presto-docs/src/main/sphinx/connector/iceberg.rst

presto-docs/src/main/sphinx/presto-cpp.rst

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build, looks good. Thank you!

yingsu00 · 2025-03-25T01:26:40Z

presto-docs/src/main/sphinx/connector/iceberg.rst

                                                        as a join with the data of the equality delete files.

-``iceberg.enable-parquet-dereference-pushdown``         Enable parquet dereference pushdown.                          ``true``
+``iceberg.enable-parquet-dereference-pushdown``         Enable parquet dereference pushdown.                          ``true``                           Yes                 NA


Why is it NA? If it's fails now, just say No

yingsu00 · 2025-03-25T01:27:46Z

presto-docs/src/main/sphinx/connector/iceberg.rst


                                                        Example: ``/etc/hadoop/conf/core-site.xml.`` This property
                                                        is required if the iceberg.catalog.type is ``hadoop``.
                                                        Otherwise, it will be ignored.

-``iceberg.file-format``                                 The storage file format for Iceberg tables. The available     ``PARQUET``
+``iceberg.file-format``                                 The storage file format for Iceberg tables. The available     ``PARQUET``                        Yes                 NA


Can we add a little explanation here why it's NA? We can say "NA, write is not supported yet"

Same for other NA items. Thanks!

yingsu00 · 2025-03-25T01:30:24Z

presto-docs/src/main/sphinx/connector/iceberg.rst

                                                        updates.

-``iceberg.delete-as-join-rewrite-enabled``              When enabled, equality delete row filtering is applied        ``true``
+``iceberg.delete-as-join-rewrite-enabled``              When enabled, equality delete row filtering is applied        ``true``                           Yes                 Yes


I don't think Presto C++ supports it. Presto C++ do not interpret the equality delete file as joins, but would compile it into domain filters or filter functions. Also, reading equality delete file PR is not merged yet. Just say No for now.

Sure, I missed equality delete here.

yingsu00 · 2025-03-25T01:31:46Z

presto-docs/src/main/sphinx/connector/iceberg.rst


-``iceberg.rows-for-metadata-optimization-threshold``    The maximum number of partitions in an Iceberg table to       ``1000``
+``iceberg.rows-for-metadata-optimization-threshold``    The maximum number of partitions in an Iceberg table to       ``1000``                           Yes                 Yes


Is this one on coordinator only?

As I checked in code its usage is in IcebergMetadataOptimizer so I am assuming this is coordinator only. Do you think, I should check anything more on this?

yingsu00 · 2025-03-25T01:35:31Z

presto-docs/src/main/sphinx/connector/iceberg.rst

+===================================================== ======================================================================= =================== ==================
+Property Name                                         Description                                                             Presto Java Support Presto C++ Support
+===================================================== ======================================================================= =================== ==================
+``iceberg.delete_as_join_rewrite_enabled``            Overrides the behavior of the connector property                        Yes                 Yes


Should be No, or NA

yingsu00 · 2025-03-25T01:41:01Z

presto-docs/src/main/sphinx/connector/iceberg.rst

+Presto C++ Support
+^^^^^^^^^^^^^^^^^^
+
+All above extra hidden metadata tables are supported in Presto C++.


Just want to double confirm that you have verified they are working, right?

Apart from$changelog everything works, $changelog throws an error for C++ which I missed adding here. I opened an issue for the same. #24816

agrawalreetika · 2025-03-27T05:55:59Z

@yingsu00 I have updated all the NA with No to keep the consistency and also updated the changes based on your last few comments. Please review when ever you get a chance. Thanks!

ZacBlanco · 2025-04-01T00:18:15Z

presto-docs/src/main/sphinx/connector/iceberg.rst

+Presto C++ Support
+^^^^^^^^^^^^^^^^^^
+
+Table properties are not supported in Presto C++ because write operations have not been implemented.


I think we should consider re-wording this. It's not quite clear what the statement "Table properties are not supported ..." means. Does that mean reading them? writing them? considering them during writes? I think we should be a bit more specific

I think we should consider re-wording this. It's not quite clear what the statement "Table properties are not supported ..." means. Does that mean reading them? writing them? considering them during writes? I think we should be a bit more specific

I think what @ZacBlanco said makes sense. @agrawalreetika Will you be able to try them and update the doc separating the read and write

ZacBlanco · 2025-04-01T00:20:01Z

presto-docs/src/main/sphinx/connector/iceberg.rst

+Presto C++ Support
+^^^^^^^^^^^^^^^^^^
+
+All above extra hidden metadata columns are supported in Presto C++.


Suggested change

All above extra hidden metadata columns are supported in Presto C++.

All above metadata columns are supported in Presto C++.

above extra hidden

Three adjectives in a row feels like a lot. I think we can reduce

Here existing extra hidden metadata columns is taken from the main heading. But if we think we should just rephrase it saying - All above metadata columns are supported in Presto C++ that's fine with me, LMK.

It feel overly description to have three adjectives in the sentence - I think this gets the point across. Let's remove them

ZacBlanco · 2025-04-01T00:20:18Z

presto-docs/src/main/sphinx/connector/iceberg.rst

+Presto C++ Support
+^^^^^^^^^^^^^^^^^^
+
+All above extra hidden metadata tables, except `$changelog`, are supported in Presto C++.


Suggested change

All above extra hidden metadata tables, except `$changelog`, are supported in Presto C++.

All above metadata tables, except `$changelog`, are supported in Presto C++.

Here existing extra hidden metadata tables is taken from the main heading. But if we think we should just rephrase it saying - All above metadata tables, except $changelog, are supported in Presto C++. that's fine with me, LMK.

ZacBlanco · 2025-04-01T00:20:42Z

presto-docs/src/main/sphinx/connector/iceberg.rst

+Presto C++ Support
+~~~~~~~~~~~~~~~~~~
+
+Read from the tables with Partition column transform is supported in Presto C++.


Suggested change

Read from the tables with Partition column transform is supported in Presto C++.

Reads from tables with partition column transforms is supported in Presto C++.

ZacBlanco · 2025-04-01T00:20:51Z

presto-docs/src/main/sphinx/connector/iceberg.rst

+Presto C++ Support
+^^^^^^^^^^^^^^^^^^
+
+Schema Evolution is supported in Presto C++.


Suggested change

Schema Evolution is supported in Presto C++.

Schema evolution is supported in Presto C++.

ZacBlanco · 2025-04-01T00:21:10Z

presto-docs/src/main/sphinx/connector/iceberg.rst

+Presto C++ Support
+^^^^^^^^^^^^^^^^^^
+
+Presto C++ supports Parquet writer versions V1.


This is a little confusing because earlier the properties section stated that writes are not supported?

Thanks for pointing it point.
It's about actually reading, so I think I can rephrase it something like - Presto C++ supports reading Parquet data written with Parquet writer version V1. ?

ZacBlanco · 2025-04-01T00:21:50Z

presto-docs/src/main/sphinx/connector/iceberg.rst

+Presto C++ Support
+^^^^^^^^^^^^^^^^^^
+
+Time Travel is supported in Presto C++.


Suggested change

Time Travel is supported in Presto C++.

Time travel queries are supported in Presto C++.

ZacBlanco

One comment on consistency in our grammar, otherwise I think this looks great. Thanks!

ZacBlanco · 2025-04-07T22:51:59Z

presto-docs/src/main/sphinx/connector/iceberg.rst

+Schema evolution is supported in Presto C++.
+
 Parquet Writer Version
 ----------------------

 Presto now supports Parquet writer versions V1 and V2 for the Iceberg catalog.
 It can be toggled using the session property ``parquet_writer_version`` and the config property ``hive.parquet.writer.version``.
 Valid values for these properties are ``PARQUET_1_0`` and ``PARQUET_2_0``. Default is ``PARQUET_1_0``.

+Presto C++ Support
+^^^^^^^^^^^^^^^^^^
+
+Presto C++ supports reading Parquet data written with Parquet writer version V1.
+


I see that across these Presto C++ Support sections we have varying forms

... is supported in Presto C++

and

Presto C++ supports ....

Two comments on this

We should be consistent

I prefer that we standardize on the 2nd as it's in active voice, which is generally the form we want to write for our documentation (@steveburnett can you confirm the 2nd is the form that we should prefer?)

Yes, we should prefer the use of active voice, but exceptions where passive voice is better can happen. Sometimes active voice can be more awkward depending on the emphasis you want to communicate.

See

the Gitlab Documentation Style Guide entry for Active Voice (shorter)

the Google Developer Documentation Style Guide entry for Active Voice (longer and more examples)

tl;dr: use active voice, but if active voice is awkward then passive voice is okay.

In this context of the "Presto C++ Support" subtopics, I would argue that passive voice is better.

The statements immediately follow the heading "Presto C++ Support" so "Presto C++ supports" repeat the information without adding to it, and

Passive voice here follows the guideline in the Google Developer Documentation Style Guide entry for Active Voice "To emphasize an object over an action."

Using passive voice here emphasizes the object "HIVE, NESSIE, REST, and HADOOP Iceberg catalogs" and makes it easier for the user to find the information they need.

See the screenshots I made in a local doc build to show the difference. I think the second one is the better one.

steveburnett

Thanks for the change and the new consistency! One nit.

presto-docs/src/main/sphinx/connector/iceberg.rst

ZacBlanco

Thanks for the changes!

steveburnett

LGTM! (docs)

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

agrawalreetika requested a review from yingsu00 March 17, 2025 18:46

agrawalreetika self-assigned this Mar 17, 2025

agrawalreetika requested review from steveburnett, elharo, hantangwangd, ZacBlanco and a team as code owners March 17, 2025 18:46

agrawalreetika requested a review from jaystarshot March 17, 2025 18:46

prestodb-ci added the from:IBM PR from IBM label Mar 17, 2025

prestodb-ci requested review from a team, nmahadevuni and auden-woolfson and removed request for a team March 17, 2025 18:46

github-actions bot added the docs label Mar 17, 2025

prestodb-ci requested a review from a team March 17, 2025 18:49

github-project-automation bot added this to Presto Documentation Mar 17, 2025

github-project-automation bot moved this to 🆕 Unprioritized in Presto Documentation Mar 17, 2025

auden-woolfson previously approved these changes Mar 17, 2025

View reviewed changes

steveburnett requested changes Mar 17, 2025

View reviewed changes

github-project-automation bot moved this from 🆕 Unprioritized to 🏗 In progress in Presto Documentation Mar 17, 2025

yingsu00 reviewed Mar 18, 2025

View reviewed changes

agrawalreetika dismissed auden-woolfson’s stale review via aa5b2d3 March 18, 2025 09:09

agrawalreetika force-pushed the iceberg-native-doc branch 2 times, most recently from aa5b2d3 to 76c9f55 Compare March 18, 2025 09:14

hantangwangd reviewed Mar 18, 2025

View reviewed changes

steveburnett requested changes Mar 18, 2025

View reviewed changes

agrawalreetika force-pushed the iceberg-native-doc branch from 76c9f55 to ae78e53 Compare March 18, 2025 15:07

hantangwangd previously approved these changes Mar 19, 2025

View reviewed changes

steveburnett previously approved these changes Mar 19, 2025

View reviewed changes

github-project-automation bot moved this from 🏗 In progress to ✅ Done in Presto Documentation Mar 19, 2025

yingsu00 reviewed Mar 25, 2025

View reviewed changes

agrawalreetika dismissed stale reviews from steveburnett and hantangwangd via 506ae37 March 27, 2025 05:54

agrawalreetika force-pushed the iceberg-native-doc branch from ae78e53 to 506ae37 Compare March 27, 2025 05:54

ZacBlanco requested changes Apr 1, 2025

View reviewed changes

github-project-automation bot moved this from ✅ Done to 🏗 In progress in Presto Documentation Apr 1, 2025

agrawalreetika force-pushed the iceberg-native-doc branch 3 times, most recently from 95463f3 to e2dfc98 Compare April 7, 2025 19:52

agrawalreetika requested review from yingsu00, ZacBlanco and steveburnett April 7, 2025 20:04

ZacBlanco requested changes Apr 7, 2025

View reviewed changes

agrawalreetika force-pushed the iceberg-native-doc branch from e2dfc98 to 4c57c31 Compare April 8, 2025 14:33

steveburnett requested changes Apr 8, 2025

View reviewed changes

presto-docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved

agrawalreetika force-pushed the iceberg-native-doc branch from 4c57c31 to dfb1896 Compare April 8, 2025 15:05

Add documentation for Iceberg support in PrestoCPP

956efe4

agrawalreetika force-pushed the iceberg-native-doc branch from dfb1896 to 956efe4 Compare April 8, 2025 15:07

ZacBlanco approved these changes Apr 8, 2025

View reviewed changes

steveburnett approved these changes Apr 8, 2025

View reviewed changes

github-project-automation bot moved this from 🏗 In progress to ✅ Done in Presto Documentation Apr 8, 2025

hantangwangd approved these changes Apr 8, 2025

View reviewed changes

steveburnett merged commit 7196add into prestodb:master Apr 8, 2025
94 checks passed

agrawalreetika deleted the iceberg-native-doc branch April 8, 2025 18:40

unidevel mentioned this pull request Apr 10, 2025

Add 0.292 docs prestodb/prestodb.github.io#294

Open


		* Supports reading and writing of DWRF and PARQUET file formats, supports reading ORC file format.
		Hive Connector - Iceberg Table Support


		``SELECT`` Yes Yes Read is supported in Presto C++ including those with positional delete files.

		``INSERT INTO`` Yes No


		* Only read operations are supported for Iceberg tables.

		* The Iceberg connector supports both V1 and V2 tables, including those with positional delete files, but does not support equality delete files.


		``iceberg.rows-for-metadata-optimization-threshold`` The maximum number of partitions in an Iceberg table to ``1000``
		``iceberg.rows-for-metadata-optimization-threshold`` The maximum number of partitions in an Iceberg table to ``1000`` Yes Yes

	All above extra hidden metadata columns are supported in Presto C++.
	All above metadata columns are supported in Presto C++.

	All above extra hidden metadata tables, except `$changelog`, are supported in Presto C++.
	All above metadata tables, except `$changelog`, are supported in Presto C++.

	Read from the tables with Partition column transform is supported in Presto C++.
	Reads from tables with partition column transforms is supported in Presto C++.

	Schema Evolution is supported in Presto C++.
	Schema evolution is supported in Presto C++.

	Time Travel is supported in Presto C++.
	Time travel queries are supported in Presto C++.

Add documentation for Iceberg support in PrestoCPP #24741

Add documentation for Iceberg support in PrestoCPP #24741

Conversation

agrawalreetika commented Mar 17, 2025

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

auden-woolfson left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

yingsu00 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawalreetika commented Mar 18, 2025

hantangwangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawalreetika Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yingsu00 Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawalreetika Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

agrawalreetika commented Mar 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawalreetika Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

ZacBlanco Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacBlanco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

ZacBlanco left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

yingsu00 left a comment •

edited

Loading

agrawalreetika Mar 18, 2025 •

edited

Loading

yingsu00 Mar 18, 2025 •

edited

Loading

agrawalreetika Mar 27, 2025 •

edited

Loading

agrawalreetika Apr 1, 2025 •

edited

Loading

ZacBlanco Apr 1, 2025 •

edited

Loading