refactor: Add Iceberg connector #15581

PingLiuPing · 2025-11-20T03:41:39Z

Refactors the Iceberg connector implementation to separate it from Hive connector. Sets the foundation for independent evolution of both connectors.

netlify · 2025-11-20T03:41:45Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`e967787`
🔍 Latest deploy log	https://app.netlify.com/projects/meta-velox/deploys/69206f0513e31c0008f5cb2a

PingLiuPing · 2025-11-20T04:04:16Z

@mbasmanova This is a follow up PR from discussion #15440 (comment). Code changes in Prestissimo: prestodb/presto#26661

mbasmanova · 2025-11-20T10:13:30Z

velox/connectors/hive/iceberg/tests/IcebergTestBase.cpp


+  // Register IcebergConnector.
+  IcebergConnectorFactory icebergFactory;
+  auto icebergConnector = icebergFactory.newConnector(


Can we add a way to specify the prefix to use for transform functions? This way, the application can control the contents of the function registry better.

Thanks. This makes sense.
But newConnector is an interface from ConnectorFactory, we cannot adding a new parameter to this method to specifying the prefix since the parameter doesn't mean anything to other connector. And I see Prestissimo calls this method for all types of connector with same parametes, so we cannot adding a new method either otherwise it requires special handling from caller.
Instead I hard-code the prefix in IcebergConnectorFactory::newConnector and pass it to IcebergConnector, then pass it to IcebergDataSink. This prefix is for internal use only.

I think it is desirable to avoid hard-coding this prefix. Can we define connector-specific configuration property and use that instead?

Yes, I looked at this too. But seems this way requires a new entry in the catalog property file.

See https://github.com/prestodb/presto/blob/9374cd8672d8e76a2c9db0ee296a5c8a320e5b58/presto-native-execution/presto_cpp/main/PrestoServer.cpp#L1270-L1300

Is this a problem?

It seems to me that this prefix is primarily for internal use, and therefore the application may not need to specify it explicitly in the catalog property file. And we still need some logic to amend this if the property is missing.

The problem is that currently a single application (Prestissimo) imposes restrictions on the Connector which may be used in other applications. If these other applications impose their own restrictions that conflict with restrictions imposed by Prestissimo there will be no way to satisfy these. Hence, it would be best to avoid such restrictions. Makes sense?

Thank you. Yes, understood now.
Update the code to retrieve this prefix from config and setting the default prefix if not present.

mbasmanova · 2025-11-20T10:19:01Z

velox/connectors/hive/iceberg/tests/IcebergInsertTest.cpp

 }
 #endif

+TEST_F(IcebergInsertTest, connectorConfiguration) {


Why does this test belongs to 'insert' test? How do we verify that configuration properties do take effect and not just ignored?

Thanks.
Moved them to a new test file.

mbasmanova · 2025-11-20T10:19:09Z

velox/connectors/hive/iceberg/tests/IcebergInsertTest.cpp

+  test(rowType);
+}
+
+TEST_F(IcebergInsertTest, connectorProperties) {


Does this test belong here?

Thanks, moved.

mbasmanova · 2025-11-20T10:19:26Z

velox/connectors/hive/iceberg/tests/IcebergInsertTest.cpp

    exec::test::AssertQueryBuilder(plan).splits(splits).assertResults(vectors);
  }
+
+  static void resetIcebergConnector(


Should this go into xxxTestBase?

Moved to new test file.

mbasmanova · 2025-11-20T10:20:02Z

velox/connectors/hive/iceberg/IcebergDataSink.h

-/// Upstream engines such as Prestissimo and Gluten should register the same
-/// functions with different prefixes to avoid conflicts.
-void registerIcebergInternalFunctions(const std::string_view& prefix);
+static constexpr std::string_view kDefaultIcebergFunctionPrefix{


I think we want to remove this constant altogether and let the application specify the prefix when registering the connector.

mbasmanova · 2025-11-20T10:22:18Z

velox/connectors/hive/iceberg/IcebergConnector.h

+
+  IcebergConnectorFactory() : ConnectorFactory(kIcebergConnectorName) {}
+
+  std::shared_ptr<Connector> newConnector(


Would you document this API? Specifically, what are ioExecutor and cpuExecutor are for and why they are optional?

Thanks. Sure. cpuExecutor is not used by Hive and Iceberg connector.

mbasmanova · 2025-11-20T10:23:44Z

velox/connectors/hive/iceberg/IcebergConnector.cpp

+  auto icebergInsertHandle =
+      std::dynamic_pointer_cast<const IcebergInsertTableHandle>(
+          connectorInsertTableHandle);
+  VELOX_USER_CHECK_NOT_NULL(


Use checked_poiner_cast from velox/common/Casts.h

mbasmanova · 2025-11-20T10:25:00Z

velox/connectors/hive/HiveConnector.cpp

    ConnectorInsertTableHandlePtr connectorInsertTableHandle,
    ConnectorQueryCtx* connectorQueryCtx,
    CommitStrategy commitStrategy) {
-  if (auto icebergInsertHandle =


mbasmanova · 2025-11-21T02:38:19Z

velox/connectors/hive/iceberg/IcebergConnector.h

+
+namespace facebook::velox::connector::hive::iceberg {
+
+static constexpr std::string_view kIcebergFunctionPrefix{


We probably want something similar to velox/connectors/hive/HiveConfig.h

Yes, agree. I actually added IcebergConfig.h and then removed it since I only need one property in this PR.
Can I open another PR to add IcebergConfig.h? It requires some changes on other Iceberg classes too.

Sounds good. Let's then add a TODO here.

The names kIcebergFunctionPrefix and kDefaultIcebergFunctionPrefix suggest that these are both prefixes... however one is the name of the config property whose value is the prefix and the other is the prefix. Perhaps, rename kIcebergFunctionPrefix to kIcebergFunctionPrefixConfig to avoid confusion.

mbasmanova · 2025-11-21T12:38:38Z

velox/connectors/hive/iceberg/IcebergConnector.h

+      RowTypePtr inputType,
+      ConnectorInsertTableHandlePtr connectorInsertTableHandle,
+      ConnectorQueryCtx* connectorQueryCtx,
+      CommitStrategy commitStrategy) override;


Does Iceberg have support for all commit strategies? If not, let's clarify what is supported.

Thanks, only kNoCommit is supported.

mbasmanova · 2025-11-21T12:38:52Z

velox/connectors/hive/iceberg/IcebergConnector.h

+  /// @param connectorQueryCtx Query context for the write operation.
+  /// @param commitStrategy Strategy for committing the write operation.
+  /// @return IcebergDataSink instance configured for the write operation.
+  /// @throws VeloxUserError if connectorInsertTableHandle is not an


This comment is redundant. Let's remove.

mbasmanova · 2025-11-21T12:39:04Z

velox/connectors/hive/iceberg/IcebergConnector.h

+  const std::string functionPrefix_;
+};
+
+/// Factory for creating IcebergConnector instances.


Redundant comment.

mbasmanova · 2025-11-21T12:40:23Z

velox/connectors/hive/iceberg/IcebergConnector.h

+      const std::string& id,
+      std::shared_ptr<const config::ConfigBase> config,
+      folly::Executor* ioExecutor = nullptr,
+      folly::Executor* cpuExecutor = nullptr) override {


linter will complain about this parameter being unused. Add [[maybe_unused]].

Thanks. Added this to HiveConnector as well.

mbasmanova · 2025-11-21T12:41:29Z

velox/connectors/hive/iceberg/tests/IcebergTestBase.cpp

  opPool_.reset();
  root_.reset();
  queryCtx_.reset();
+  // Unregister IcebergConnector


redundant comment

mbasmanova

Thanks.

meta-codesync · 2025-11-21T20:28:21Z

@kagamiori has imported this pull request. If you are a Meta employee, you can view this in D87666371.

PingLiuPing requested a review from majetideepak as a code owner November 20, 2025 03:41

meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 20, 2025

PingLiuPing requested review from mbasmanova and removed request for majetideepak November 20, 2025 03:41

mbasmanova reviewed Nov 20, 2025

View reviewed changes

PingLiuPing force-pushed the lp_iceberg_connector branch 2 times, most recently from b7f5029 to 80633b7 Compare November 21, 2025 02:25

mbasmanova reviewed Nov 21, 2025

View reviewed changes

mbasmanova approved these changes Nov 21, 2025

View reviewed changes

mbasmanova added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Nov 21, 2025

Add IcebergConnector

e967787

PingLiuPing force-pushed the lp_iceberg_connector branch from 80633b7 to e967787 Compare November 21, 2025 13:54


		IcebergConnectorFactory() : ConnectorFactory(kIcebergConnectorName) {}

		std::shared_ptr<Connector> newConnector(


		namespace facebook::velox::connector::hive::iceberg {

		static constexpr std::string_view kIcebergFunctionPrefix{

refactor: Add Iceberg connector #15581

Are you sure you want to change the base?

refactor: Add Iceberg connector #15581

Uh oh!

Conversation

PingLiuPing commented Nov 20, 2025

Uh oh!

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

Uh oh!

✅ Deploy Preview for meta-velox canceled.

Uh oh!

PingLiuPing commented Nov 20, 2025

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!

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!

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!

Choose a reason for hiding this comment

netlify bot commented Nov 20, 2025 •

edited

Loading