feat(native): Support insert data into iceberg table by PingLiuPing · Pull Request #25389 · prestodb/presto

PingLiuPing · 2025-06-20T04:10:43Z

Description

Support insertion for iceberg tables. This PR only contains the basic insertion and does NOT support partition transforms, sort order, statistics yet. Those feature will be addressed in following PR once this PR is merged.
The insertion logic is now been protected by MACRO PRESTO_ENABLE_ICEBERG_NATIVE_INSERTION. So, it is not depends on velox code change anymore.

The core implementation is inside velox, see PR facebookincubator/velox#10996

To enable this feature, run:

`make EXTRA_CMAKE_FLAGS="-DPRESTO_ENABLE_ICEBERG_NATIVE_INSERTION=ON"

Motivation and Context

Impact

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

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Add native insertion support for iceberg connector.

Hive Connector Changes
* Update Hive connector by adding a new Iceberg connector and implement basic Iceberg insertion.

steveburnett · 2025-06-24T14:09:52Z

Do we need any documentation update for this PR? If the doc exists already, consider linking to it in the release note entry - see Formatting in the Release Notes Guidelines for link examples.

yingsu00

@PingLiuPing I don't see why we need two commits here. Could you please re-organize the commits like this:

Format-only changes
Refactor toVeloxInsertTableHandle on Hive and extract helper function
Squashed commit that contains the two current commits

Thanks!

presto-native-execution/presto_cpp/main/connectors/PrestoToVeloxConnector.cpp

yingsu00 · 2025-08-06T06:09:08Z

presto-native-execution/presto_cpp/main/connectors/PrestoToVeloxConnector.cpp

+      velox::connector::hive::iceberg::IcebergInsertTableHandle>(
+      inputColumns,
+      std::make_shared<connector::hive::LocationHandle>(
+          icebergOutputTableHandle->outputPath,


Could you please confirm this doesn't need /data suffix?

Thanks.
I think it is reasonable to add /data suffix.
But I'm not sure when this function will be executed.
I tried create table, alter table, drop table and both of them do not execute this function.

presto-native-execution/presto_cpp/main/connectors/PrestoToVeloxConnector.cpp

presto-native-execution/presto_cpp/main/connectors/PrestoToVeloxConnector.h

yingsu00

hi Tony, have you checked the build failure? It seems related.

PingLiuPing · 2025-08-15T11:05:08Z

hi Tony, have you checked the build failure? It seems related.

Thanks @yingsu00 , let me rebase and run test again.

PingLiuPing · 2025-08-28T20:35:03Z

@yingsu00 Thanks for your comments, I have fixed them, can you have another look? Or do you prefer I move this PR it internal repo?

aditi-pandit

Thanks @PingLiuPing for this code. Have few comments.

aditi-pandit · 2025-10-01T21:36:52Z

presto-native-execution/presto_cpp/main/connectors/PrestoToVeloxConnector.cpp


+#ifdef PRESTO_ENABLE_ICEBERG_NATIVE_INSERTION
+
+std::unique_ptr<velox::connector::ConnectorInsertTableHandle>


Is it possible to move IcebergPrestoToVeloxConnector to its own file ?
We could do that to the remaining PrestoToVeloxConnectors as well as a follow-up cleanup.

@aditi-pandit Thanks for the comment.
Yes, we have same observations. And I opened a PR #25631 days ago.
I will follow-up this once the iceberg insertion related code merged in velox. And to split the iceberg code with Hive code.

@PingLiuPing : You can move IcebergPrestoToVeloxConnector in its current state to its own file as a first step already (before adding the write parts). That PR should get approved in OSS.

Thanks for the comment. Sure, I can open a separate PR and refactor this first.

Opened PR #26237

presto-native-execution/presto_cpp/main/connectors/PrestoToVeloxConnector.cpp

aditi-pandit · 2025-10-01T21:42:56Z

presto-native-execution/presto_cpp/main/connectors/PrestoToVeloxConnector.h

+    std::unique_ptr<protocol::ConnectorProtocol> createConnectorProtocol()
+    const final;
+
+#ifdef PRESTO_ENABLE_ICEBERG_NATIVE_INSERTION


Can we move this class to a separate include file ?

Thanks for the comment.
In #25631, I have separated this class with other classes.
Can I do it in this follow up PR? If you insist I can move it out in this PR.
After separate them, this macro is not needed as we can conditionally compile the file in CMakeFile.

aditi-pandit · 2025-10-01T21:46:12Z

presto-native-execution/presto_cpp/presto_protocol/connector/iceberg/IcebergConnectorProtocol.h

    IcebergTableLayoutHandle,
    IcebergColumnHandle,
+
+#ifdef PRESTO_ENABLE_ICEBERG_NATIVE_INSERTION


Don't think its worth adding these ifdefs in the protocol layer here. Should be fine to just include IcebergInsertTableHandle and IcebergOutputTableHandle in the protocol files nonetheless.

Thanks for the comment.
Let me test it and see if the error message are expected.

When the velox does not support iceberg insertion, and removed this macro I get following error messages:
using IcebergConnectorProtocol = ConnectorProtocolTemplate<
...
IcebergInsertTableHandle,
IcebergOutputTableHandle,
...

presto:iceberg> insert into year_t1 values (TIMESTAMP '2025-02-28 14:00:02', 11, DATE '2025-02-28', 125); Query 20251002_094514_00000_7gkvm, FAILED, 2 nodes Splits: 1 total, 0 done (0.00%) [Latency: client-side: 441ms, server-side: 427ms] [0 rows, 0B] [0 rows/s, 0B/s] Query 20251002_094514_00000_7gkvm failed: Unsupported table writer handle: {"@type":"InsertHandle","handle":{"connectorHandle":{"@type":"hive-iceberg","compressionCodec":"GZIP","fileFormat":"PARQUET","inputColumns":[{"@type":"hive-iceberg","columnIdentity":{"children":[],"id":1,"name":"c_timestamp","typeCategory":"PRIMITIVE"},"columnType":"REGULAR","requiredSubfields":[],"type":"timestamp"},{"@type":"hive-iceberg","columnIdentity":{"children":[],"id":2,"name":"c_int","typeCategory":"PRIMITIVE"},"columnType":"PARTITION_KEY","requiredSubfields":[],"type":"integer"},{"@type":"hive-iceberg","columnIdentity":{"children":[],"id":3,"name":"c_date","typeCategory":"PRIMITIVE"},"columnType":"REGULAR","requiredSubfields":[],"type":"date"},{"@type":"hive-iceberg","columnIdentity":{"children":[],"id":4,"name":"c_bigint","typeCategory":"PRIMITIVE"},"columnType":"REGULAR","requiredSubfields":[],"type":"bigint"}],"outputPath":"file:/Users/pingliu/work/opensource/presto/presto-native-execution/data/iceberg_data/HIVE/iceberg/year_t1","partitionSpec":{"fields":[{"fieldId":1000,"name":"c_int","sourceId":2,"transform":"IDENTITY"}],"schema":{"aliases":{},"columnNameToIdMapping":{"c_bigint":4,"c_date":3,"c_int":2,"c_timestamp":1},"columns":[{"id":1,"name":"c_timestamp","optional":true,"prestoType":"timestamp"},{"id":2,"name":"c_int","optional":true,"prestoType":"integer"},{"id":3,"name":"c_date","optional":true,"prestoType":"date"},{"id":4,"name":"c_bigint","optional":true,"prestoType":"bigint"}],"identifierFieldIds":[],"schemaId":0},"specId":0},"schema":{"aliases":{},"columnNameToIdMapping":{"c_bigint":4,"c_date":3,"c_int":2,"c_timestamp":1},"columns":[{"id":1,"name":"c_timestamp","optional":true,"prestoType":"timestamp"},{"id":2,"name":"c_int","optional":true,"prestoType":"integer"},{"id":3,"name":"c_date","optional":true,"prestoType":"date"},{"id":4,"name":"c_bigint","optional":true,"prestoType":"bigint"}],"identifierFieldIds":[],"schemaId":0},"schemaName":"iceberg","sortOrder":[],"storageProperties":{"commit.retry.num-retries":"4","read.split.target-size":"134217728","write.delete.mode":"merge-on-read","write.format.default":"PARQUET","write.metadata.delete-after-commit.enabled":"false","write.metadata.metrics.max-inferred-column-defaults":"100","write.metadata.previous-versions-max":"100","write.parquet.compression-codec":"GZIP","write.update.mode":"merge-on-read"},"tableName":{"snapshotId":5359319739731907098,"tableName":"year_t1","tableType":"DATA"}},"connectorId":"iceberg","transactionHandle":{"@type":"hive","uuid":"451ae372-1bbe-4410-9f93-40a24b60c27c"}},"schemaTableName":{"schema":"iceberg","table":"year_t1"}}

When using NotImplemented

using IcebergConnectorProtocol = ConnectorProtocolTemplate< ... NotImplemented, NotImplemented, ... >;

I got following error message:

presto:iceberg> insert into year_t1 values (TIMESTAMP '2025-02-28 14:00:02', 11, DATE '2025-02-28', 125); Query 20251002_095454_00000_mpr4s failed: Not implemented: N8facebook6presto8protocol26ConnectorInsertTableHandleE

Seems we should use the last one as it is more precise and shorter. What's your opinion?

I see where you are coming from, but I feel that's not how protocol conversion should be used. Protocol conversion from Java -> C++ artifacts should happen just as choosing the language backend. If we have some unsupported logic in the Presto -> Velox layer, then its reasonable to error in the PrestoToVelox code. The error message "Unsupported table writer handle" with the handle value is a reasonable behavior imo.

Just adding "IcebergInsertTableHandle,
IcebergOutputTableHandle" and updating the protocol layer can be submit independently imo.

Agreed with @aditi-pandit. We can document this limitation so it's easier for folks to understand.

@tdcmeehan @aditi-pandit Thanks for the comments. I will fix this.

I add this change to #26237

aditi-pandit · 2025-10-01T21:47:52Z

presto-native-execution/presto_cpp/presto_protocol/connector/iceberg/IcebergConnectorProtocol.h

Did you regenerate the presto_protocol after this change ?https://github.com/prestodb/presto/tree/master/presto-native-execution/presto_cpp/presto_protocol#presto-native-worker-protocol-code-generation

Thanks for the comment.
I didn't regenerate the presto_protocol because I didn't change .yml file in this PR.

FYI, I run make presto_protocol based on this PR though, and I do see some file been changed. But after careful check I think they are just format changes. And most likely has nothing to do with code change in this PR.

There are no changes in files in presto_protocol/connector/iceberg folder ?

Thanks for the comment.
When I run make presto_protocol there are files been changed in presto_protocol/connector/iceberg folder. But I deem all of those are just code format change. So, I choose not to commit them.

aditi-pandit · 2025-10-03T20:42:55Z

presto-native-execution/presto_cpp/main/connectors/PrestoToVeloxConnector.cpp


+#ifdef PRESTO_ENABLE_ICEBERG_NATIVE_INSERTION
+
+std::unique_ptr<velox::connector::ConnectorInsertTableHandle>


This file has a using namespace directive for the velox namespace. So "velox" namespace can be removed from variable names.

Thanks. Let me fix this.

aditi-pandit · 2025-10-03T20:53:31Z

presto-native-execution/presto_cpp/main/connectors/PrestoToVeloxConnector.cpp


+#ifdef PRESTO_ENABLE_ICEBERG_NATIVE_INSERTION
+
+std::unique_ptr<velox::connector::ConnectorInsertTableHandle>


@PingLiuPing : You can move IcebergPrestoToVeloxConnector in its current state to its own file as a first step already (before adding the write parts). That PR should get approved in OSS.

aditi-pandit · 2025-10-03T21:05:27Z

presto-native-execution/presto_cpp/presto_protocol/connector/iceberg/IcebergConnectorProtocol.h

    IcebergTableLayoutHandle,
    IcebergColumnHandle,
+
+#ifdef PRESTO_ENABLE_ICEBERG_NATIVE_INSERTION


I see where you are coming from, but I feel that's not how protocol conversion should be used. Protocol conversion from Java -> C++ artifacts should happen just as choosing the language backend. If we have some unsupported logic in the Presto -> Velox layer, then its reasonable to error in the PrestoToVelox code. The error message "Unsupported table writer handle" with the handle value is a reasonable behavior imo.

Just adding "IcebergInsertTableHandle,
IcebergOutputTableHandle" and updating the protocol layer can be submit independently imo.

aditi-pandit · 2025-10-03T21:08:09Z

presto-native-execution/presto_cpp/presto_protocol/connector/iceberg/IcebergConnectorProtocol.h

There are no changes in files in presto_protocol/connector/iceberg folder ?

aditi-pandit · 2025-10-03T21:14:35Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/PartitionData.java

                }
            case DECIMAL:
-                return partitionValue.decimalValue().setScale(((DecimalType) type).scale());
+                if (partitionValue.isLong()) {


Does this change need to be in the PR for Iceberg insertion ? We can make it an independent change I feel. Can you open a new PR for it ?

Yes, this is part of the insertion. This piece of code is used to handle the partition value which will be wrote to iceberg manifest file.

steveburnett · 2025-10-06T15:40:24Z

Should this make EXTRA_CMAKE_FLAGS="-DPRESTO_ENABLE_ICEBERG_NATIVE_INSERTION=ON be documented somewhere?

Perhaps in https://github.com/prestodb/presto/blob/master/presto-native-execution/README.md?

PingLiuPing · 2025-10-06T15:54:46Z

Should this make EXTRA_CMAKE_FLAGS="-DPRESTO_ENABLE_ICEBERG_NATIVE_INSERTION=ON be documented somewhere?

Perhaps in https://github.com/prestodb/presto/blob/master/presto-native-execution/README.md?

Thanks, actually this PR is not expected to be merged to OSS. The underlying code in velox has not been merged. The config option is just a workaround to make the code can compile when the code in velox is not merged.

steveburnett · 2025-10-06T16:09:55Z

Should this make EXTRA_CMAKE_FLAGS="-DPRESTO_ENABLE_ICEBERG_NATIVE_INSERTION=ON be documented somewhere?
Perhaps in https://github.com/prestodb/presto/blob/master/presto-native-execution/README.md?

Thanks, actually this PR is not expected to be merged to OSS. The underlying code in velox has not been merged. The config option is just a workaround to make the code can compile when the code in velox is not merged.

I appreciate you taking the time to explain! Thank you.

PingLiuPing self-assigned this Jun 20, 2025

PingLiuPing requested review from a team, ZacBlanco and hantangwangd as code owners June 20, 2025 04:10

prestodb-ci added the from:IBM PR from IBM label Jun 20, 2025

prestodb-ci requested review from a team, namya28 and sh-shamsan and removed request for a team June 20, 2025 04:10

PingLiuPing changed the title ~~(feat): Support insert data into iceberg table (Prestissimo)~~ (feat): [native] Support insert data into iceberg table Jun 20, 2025

PingLiuPing force-pushed the lp_iceberg_basic_insertion branch 3 times, most recently from a6aacfe to b810cef Compare June 24, 2025 07:16

PingLiuPing force-pushed the lp_iceberg_basic_insertion branch 2 times, most recently from e74926c to a8312fd Compare June 27, 2025 15:27

PingLiuPing force-pushed the lp_iceberg_basic_insertion branch from a8312fd to f731612 Compare July 7, 2025 21:33

PingLiuPing mentioned this pull request Jul 30, 2025

[native] Iceberg basic insertion #25659

Open

PingLiuPing force-pushed the lp_iceberg_basic_insertion branch 2 times, most recently from 1814cc8 to 23df781 Compare August 4, 2025 21:45

PingLiuPing requested a review from yingsu00 August 4, 2025 21:46

PingLiuPing force-pushed the lp_iceberg_basic_insertion branch from 23df781 to 0adb84f Compare August 4, 2025 21:50

PingLiuPing removed request for namya28 and sh-shamsan August 4, 2025 21:50

yingsu00 reviewed Aug 6, 2025

View reviewed changes

PingLiuPing force-pushed the lp_iceberg_basic_insertion branch 2 times, most recently from b732b15 to 7b07b3e Compare August 6, 2025 15:06

PingLiuPing mentioned this pull request Aug 12, 2025

[Native] Make velox submodule configurable #25498

Open

yingsu00 reviewed Aug 15, 2025

View reviewed changes

PingLiuPing force-pushed the lp_iceberg_basic_insertion branch from 7b07b3e to b8290c2 Compare August 15, 2025 13:28

PingLiuPing mentioned this pull request Sep 28, 2025

feat(native): Support Iceberg partition transforms #26174

Closed

6 tasks

PingLiuPing added 2 commits October 1, 2025 10:44

Support insert data into iceberg table.

e70b098

Auto detect velox iceberg insertion feature.

81f9dce

PingLiuPing force-pushed the lp_iceberg_basic_insertion branch from 5d3a0d7 to 81f9dce Compare October 1, 2025 09:45

aditi-pandit reviewed Oct 1, 2025

View reviewed changes

tdcmeehan changed the title ~~(feat): [native] Support insert data into iceberg table~~ feat(native): Support insert data into iceberg table Oct 2, 2025

aditi-pandit reviewed Oct 3, 2025

View reviewed changes

PingLiuPing mentioned this pull request Oct 6, 2025

refactor(native): Separate IcebergPrestoToVeloxConnector to standalone file #26237

Merged

6 tasks

PingLiuPing mentioned this pull request Oct 10, 2025

fix: Support more type conversion for decimal partition value #26240

Merged

6 tasks

PingLiuPing closed this Oct 26, 2025


		#ifdef PRESTO_ENABLE_ICEBERG_NATIVE_INSERTION

		std::unique_ptr<velox::connector::ConnectorInsertTableHandle>

Conversation

PingLiuPing commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

steveburnett commented Jun 24, 2025

Uh oh!

yingsu00 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yingsu00 left a comment

Choose a reason for hiding this comment

Uh oh!

PingLiuPing commented Aug 15, 2025

Uh oh!

PingLiuPing commented Aug 28, 2025

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PingLiuPing Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PingLiuPing commented Jun 20, 2025 •

edited

Loading

PingLiuPing Oct 2, 2025 •

edited

Loading