[native] Add support for Iceberg connector in presto_protocol #21765

imjalpreet · 2024-01-24T10:15:05Z

Description

Add Java classes from the Iceberg connector in presto_protocol.yml file.
Specialize IcebergColumnHandle to support 'operator<()'.

Depends on #21764

Motivation and Context

facebookincubator/velox#5977

Impact

With these changes, we will be able to read iceberg tables from Prestissimo

Test Plan

Successfully ran all TPC-H queries with iceberg tables (both partitioned and non-partitioned)

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
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 support for Iceberg connector in presto_protocol

majetideepak · 2024-01-30T13:56:17Z

@imjalpreet is this ready for review?

imjalpreet · 2024-01-30T14:06:22Z

@majetideepak Yes the changes are all there in this PR. But need to rebase on master and make some minor updates based on new changes added in master. But I was facing a build error in my local most likely related to #21797 so I haven't pushed the rebased version of this branch yet.

Once I verify the changes after a successful build, I will push them as well. You can still review the PR since it mostly has all the changes or we can wait for the rebased version which has minor additions.

majetideepak · 2024-01-30T14:08:48Z

Why do we need so many special changes? Are we not able to translate the Java source files directly?
We should avoid adding to special as they are harder to maintain.

yingsu00 · 2024-01-30T14:09:52Z

presto-native-execution/presto_cpp/main/PrestoServer.cpp

@@ -852,6 +853,11 @@ std::vector<std::string> PrestoServer::registerConnectors(
      PRESTO_STARTUP_LOG(INFO) << "Registering catalog " << catalogName
                               << " using connector " << connectorName;

+      // TODO: Temporary Change to use HiveDataSource for Iceberg Connector


Moving the discussion in #21584 to here:

Add an iceberg.properties in presto-native-execution/etc/catalog with the following

# The Presto "iceberg" catalog is handled by the hive connector in Presto native execution. connector.name=hive cache.enabled=true

@yingsu00 I have added the catalog file in #21584 since the final changes are part of that PR

yingsu00 · 2024-01-30T14:13:57Z

presto-native-execution/presto_cpp/presto_protocol/Connectors.cpp

@@ -44,6 +44,10 @@ void registerHiveConnectors() {
  registerConnector("hive-hadoop2", "hive");
 }

+void registerIcebergConnector() {
+  registerConnector("iceberg", "iceberg");


Let's keep this as is, but add a comment saying the connectorName and connectorKey are for Presto, not Presto native.

@yingsu00 I have made the change as we had discussed earlier and moved the iceberg registration as part of the hive connectors

mbasmanova · 2024-01-30T14:16:03Z

Why do we need so many special changes? Are we not able to translate the Java source files directly? We should avoid adding to special as they are harder to maintain.

@majetideepak @tdcmeehan Deepak, Tim, do we have any ongoing efforts to switch from JSON to Thrift for communication? JSON-based protocols are pretty challenging to maintain.

majetideepak · 2024-01-30T15:04:05Z

do we have any ongoing efforts to switch from JSON to Thrift for communication?

@mbasmanova There is no activity on this. Someone at Meta added the initial bits, that's all. Do you know who at Meta worked on the initial design? From the README, the incremental transition from JSON to thrift seems to be very involved. I wonder if there is a simpler way.
https://github.com/prestodb/presto/tree/master/presto-native-execution/presto_cpp/main/thrift

mbasmanova · 2024-01-30T15:09:18Z

@majetideepak Tim and @ajaygeorge would have most context on this.

tdcmeehan · 2024-01-30T15:22:27Z

@mbasmanova For clarity, there are a few reasons this is not scalable:

We must manually update the protocol every time connectors change or there is a new connector.
This protocol must be built-in to the Prestissimo binaries.

While 1 is problematic, I believe 2 is the problem that should be solved first (if we must choose between the two), because while 1 is an inconvenience, 2 is more than an inconvenience: it's a blocker from real connector SPI support in C++.

Consider that, to solve 2, we must make it so that connector data structures are not built, but deserialized in a custom way (which is how it works in Java, albeit tied JSON). The idea is to make it so that connector data structures are serialized in a custom manner, and allow connectors to self-specify how to serialize and deserialize them. This may include Thrift, but it will be up to the connector how to accomplish that.

When thought of in this way, you're left with two parts to serialize and deserialize: the outer shell of the task update request, and the inner structures used by connectors. The outer shell may be moved to Thrift. We would love to get community help to migrate this as well, but I don't think it's the top priority since it changes more slowly and it doesn't block federation use cases in Prestissimo.

tdcmeehan · 2024-01-30T15:31:10Z

To answer your question, we'll be working on 2, and I'll be directing community members toward 1 (see: #19839), but I think this PR can continue as it is for now as a little extra code is definitely worth it to open up Prestissimo for Iceberg benchmarking.

steveburnett · 2024-02-05T14:58:49Z

Nit: following the release note guidelines, perhaps change the release note entry to "Add" instead of "Introduce".

mbasmanova · 2024-02-05T15:02:33Z

presto-native-execution/presto_cpp/presto_protocol/special/BaseHiveColumnHandle.hpp.inc

+ */
+namespace facebook::presto::protocol {
+
+struct BaseHiveColumnHandle : public ColumnHandle {


Would you add comments to explain why all these classes cannot be auto-generated?

@mbasmanova I am working with @imjalpreet to try and automate these as well. We will add comments if we are unable to auto-generate.

@majetideepak Got it. Thank you for sharing. Great to know that you are trying to avoid manual overrides. Thanks.

aditi-pandit

We should try to automate the Column(Table)Handle and TableLayout classes.

Are we blocked on them because of the use of inheritance e.g. struct HiveTableHandle : public BaseHiveTableHandle ?

presto-native-execution/presto_cpp/presto_protocol/special/ColumnType.cpp.inc

imjalpreet · 2024-02-06T21:35:02Z

Are we blocked on them because of the use of inheritance e.g. struct HiveTableHandle : public BaseHiveTableHandle ?

@aditi-pandit Yes, we are trying to see if it would be possible to somehow incorporate multi-level inheritance in these classes (ColumnHandle, TableHandle, and TableLayoutHandle) without the introduction of special classes.

yingsu00 · 2024-02-09T16:38:07Z

@majetideepak @mbasmanova @tdcmeehan @aditi-pandit Thanks everyone for chiming in. @imjalpreet and I had some offline discussion today. We think there are two ways to solve this problem:

Make presto protocol to support multi-level inheritance (This PR)
In the last a couple of days, @imjalpreet has been trying to modify the Ser/De code to make it support multi-level inheritance and was able to reduce most extra specials. In this approach, presto_protocol.yml was modified to add a "super" member in the subclasses lists in addition to the current "name" and "key" members. We also had to move the partitionColumns back from BaseHiveTableLayoutHandle to HiveTableLayoutHandle and IcebergTableLayoutHandle to resolve the problem the partitionColumns could not be deserialized because it was List and the deserializer doesn't know its actual type. Jalpreet has updated this PR with this approach.
Only serialize leaf classes
In this approach , BaseHiveTableLayoutHandle and BaseHiveColumnHandle will be removed from presto_protocol, and only HiveTableLayoutHandle, IcebergTableLayoutHandle and HiveColumnHandle, IcebergColumnHandle would be serialized/deserialized. Then we will remove the references of these base classes in presto-native-execution (there is only one real callsite and it can be removed). @imjalpreet can also send a PR on this.

Both approachs need additional work. Before we proceed, we'd like to get your opinions which route we are going to take. IMHO the first approach is more generic and may benefit future changes. But we may need to use a slightly different approach there: instead of adding a "super" member in presto_protocol.yml, I think it's better to list the classes in a separate entry:

    ConnectorTableHandle:
      super: JsonEncodedSubclass
      subclasses:
        - { name: BaseHiveTableHandle,      key: base-hive }
        - { name: TpchTableHandle,          key: tpch }
        -
    BaseHiveTableHandle:
      super: JsonEncodedSubclass
      subclasses:
        - { name: HiveTableHandle,          key: hive }
        - { name: IcebergTableHandle,       key: hive-iceberg }

This needs additional work in order to build the tree structure. Your opinions on which way to go is very appreciated.

majetideepak · 2024-02-10T00:39:05Z

@imjalpreet and @yingsu00 thanks for working on this and finding solutions to avoid manual overrides.
Among the two approaches, I prefer the latter Only serialize leaf classes since I see no use in maintaining the inheritance information in the protocol.
In the future, we can add support for' super' when there is a use-case for the inheritance details.

Both approachs need additional work.

Can you share what additional work is required for the second approach?

imjalpreet · 2024-02-12T16:11:28Z

@majetideepak I have updated the PR to remove the usage of Base classes and excluded inheritance in the protocol.

Based on these changes I have also updated the implementation in the follow-up PR: #21584

Can you share what additional work is required for the second approach?

I think Ying was referring to the implementation changes for the current PRs. There are no changes required in protocol generation scripts like the inheritance approach.

Please have a look at the updated changes and let me know if these look good or in case there are any questions.

yingsu00 · 2024-02-12T16:23:32Z

@majetideepak Jalpreet has created a PR for each way:

Multiple inheritance: [iceberg] [native] Introduce Iceberg Connector in Prestissimo (with multi-level inheritance)
Only leaf classes: This PR.

tdcmeehan · 2024-02-12T18:44:13Z

I agree with @majetideepak. It sounds like for option 1, that's like trying to replicate Jackson's polymorphic type support into our protocol generation scripts, but it doesn't really solve the problem that these data structures must be known upfront in the engine. Rather than build more sophistication into the protocol, I'd rather fix the underlying issue and try to leave protocol generation scripts as unchanged as possible until we do, which makes me favor option 2 more.

majetideepak

@imjalpreet Thank you for extending the protocol without a lot of specialization.
Can you confirm the presto_protocol.h and presto_protocol.cpp files have not been manually edited?

majetideepak

@imjalpreet please fix the commit title and description. Thanks!

mbasmanova

@imjalpreet This version is looking nice, clean and simple. Curious, what was the trick that allowed to remove the specializations.

imjalpreet · 2024-02-13T08:56:09Z

Thank you for extending the protocol without a lot of specialization. Can you confirm the presto_protocol.h and presto_protocol.cpp files have not been manually edited?

@majetideepak yes, the files have not been manually edited.

please fix the commit title and description. Thanks!

Can you please let me know what would you like to fix in the commit message?

imjalpreet · 2024-02-13T08:56:33Z

This version is looking nice, clean and simple. Curious, what was the trick that allowed to remove the specializations.

@mbasmanova There are some enums from the iceberg library that are required in the protocol. We have created wrappers for them on the java side so that they can be automated using the yaml file. Apart from that, earlier we were trying to replicate the multi-level inheritance present on the java side in prestissimo as well. But since currently we don't have a big usage of the inheritance details in Prestissimo, we have updated our implementation to remove their usage in Prestissimo. If in the future, we see that inheritance will be useful, we can have a look at #21905 for one of the ways we can update the protocol generation process to simplify the inclusion of multi-level inheritance.

mbasmanova · 2024-02-13T09:06:44Z

@imjalpreet Got it. Thank you for explaining.

majetideepak · 2024-02-13T09:16:03Z

Can you please let me know what would you like to fix in the commit message?

@imjalpreet I updated the PR title and description. We can use the same for the commit title and description as well.

majetideepak · 2024-02-13T09:23:21Z

@imjalpreet can you add the description to the commit body as well? You need to add an empty line after the title and add the body.
See example below.

commit 1bd7843cd321023d04c6cba734b136b6c552ea96
Author: Deepak Majeti <[email protected]>
Date:   Mon Feb 5 01:49:44 2024 -0500

    [native] Remove RE2 dependency from centos setup script
    
    RE2 is installed in the Velox centos setup script

majetideepak · 2024-02-13T09:24:23Z

Just these 2 lines in the commit body.

Add Java classes from the Iceberg connector in presto_protocol.yml file.
Specialize IcebergColumnHandle to support 'operator<()'.

majetideepak

Thanks, @imjalpreet

yingsu00

@imjalpreet
I see that the Json constructor for BaseHiveTableLayoutHandle is still there. Is it still needed? I don't see it's being used. Can you please add a separate commit or PR to remove it If it's no longer needed?

BaseHiveColumnHandle fields are still json annotated. Are the annotations required?

presto-native-execution/presto_cpp/main/PrestoServer.cpp

yingsu00 · 2024-02-13T10:26:43Z

presto-native-execution/presto_cpp/presto_protocol/presto_protocol.yml

@@ -177,6 +181,7 @@ JavaClasses:
  - presto-hive-metastore/src/main/java/com/facebook/presto/hive/BucketFunctionType.java
  - presto-hive-common/src/main/java/com/facebook/presto/hive/CacheQuotaRequirement.java
  - presto-hive-common/src/main/java/com/facebook/presto/hive/CacheQuotaScope.java
+  - presto-hive-common/src/main/java/com/facebook/presto/hive/BaseHiveColumnHandle.java


Is this still needed?

Yes, this is still needed. ColumnType enum is part of this class and it is required to be included in the protocol.

yingsu00 · 2024-02-13T10:31:58Z

presto-native-execution/presto_cpp/presto_protocol/special/HiveColumnHandle.hpp.inc

@@ -12,6 +12,9 @@
 * limitations under the License.
 */

+// HiveColumnHandle is special since we require an implementation of


nit:
HiveColumnHandle is special since it needs an implementation of

Sure, I will make this change.

I updated the comment

imjalpreet · 2024-02-13T10:53:06Z

I see that the Json constructor for BaseHiveTableLayoutHandle is still there. Is it still needed? I don't see it's being used. Can you please add a separate commit or PR to remove it If it's no longer needed?

BaseHiveColumnHandle fields are still json annotated. Are the annotations required?

@yingsu00 Since, this PR only includes protocol changes I haven't refactored any presto classes here.

I will have a look at the Base* classes in Presto, verify the need for JSON constructors and refactor them as needed in a separate PR.

Add Java classes from the Iceberg connector in presto_protocol.yml file. Specialize IcebergColumnHandle to support 'operator<()'.

imjalpreet self-assigned this Jan 24, 2024

imjalpreet requested review from a team as code owners January 24, 2024 10:15

imjalpreet requested a review from presto-oss January 24, 2024 10:15

imjalpreet force-pushed the icebergPrestissimoProtocolChanges branch from 5206127 to f08f85c Compare January 24, 2024 10:46

imjalpreet mentioned this pull request Jan 24, 2024

[iceberg] [native] Add support for V1 tables #21584

Merged

6 tasks

imjalpreet requested review from yingsu00, aditi-pandit and majetideepak January 24, 2024 10:57

yingsu00 added the iceberg Apache Iceberg related label Jan 25, 2024

yingsu00 reviewed Jan 30, 2024

View reviewed changes

imjalpreet force-pushed the icebergPrestissimoProtocolChanges branch 2 times, most recently from 07000e6 to 4bd39ae Compare February 1, 2024 21:07

imjalpreet requested a review from yingsu00 February 1, 2024 21:07

imjalpreet mentioned this pull request Feb 3, 2024

Refactor Iceberg Connector #21855

Merged

6 tasks

imjalpreet force-pushed the icebergPrestissimoProtocolChanges branch from 4bd39ae to e1ad4c3 Compare February 5, 2024 10:30

mbasmanova reviewed Feb 5, 2024

View reviewed changes

aditi-pandit reviewed Feb 6, 2024

View reviewed changes

presto-native-execution/presto_cpp/presto_protocol/special/ColumnType.cpp.inc Outdated Show resolved Hide resolved

imjalpreet force-pushed the icebergPrestissimoProtocolChanges branch 2 times, most recently from 63741c4 to 8294b87 Compare February 9, 2024 15:05

imjalpreet force-pushed the icebergPrestissimoProtocolChanges branch 2 times, most recently from 3277916 to 6280b7c Compare February 9, 2024 15:33

imjalpreet force-pushed the icebergPrestissimoProtocolChanges branch from 6280b7c to 8be0e31 Compare February 12, 2024 15:55

majetideepak approved these changes Feb 13, 2024

View reviewed changes

majetideepak requested changes Feb 13, 2024

View reviewed changes

mbasmanova reviewed Feb 13, 2024

View reviewed changes

majetideepak changed the title ~~[native] Iceberg prestissimo protocol changes~~ [native] Add support for Iceberg connector in presto_protocol Feb 13, 2024

imjalpreet force-pushed the icebergPrestissimoProtocolChanges branch from 8be0e31 to 1cf0223 Compare February 13, 2024 09:18

imjalpreet force-pushed the icebergPrestissimoProtocolChanges branch from 1cf0223 to 8def00f Compare February 13, 2024 09:25

majetideepak approved these changes Feb 13, 2024

View reviewed changes

yingsu00 reviewed Feb 13, 2024

View reviewed changes

[native] Add support for Iceberg connector in presto_protocol

2df519d

Add Java classes from the Iceberg connector in presto_protocol.yml file. Specialize IcebergColumnHandle to support 'operator<()'.

imjalpreet force-pushed the icebergPrestissimoProtocolChanges branch from 8def00f to 2df519d Compare February 13, 2024 16:28

yingsu00 approved these changes Feb 14, 2024

View reviewed changes

yingsu00 merged commit 1a4e945 into prestodb:master Feb 14, 2024
59 checks passed

wanglinsong mentioned this pull request May 1, 2024

Add release notes for 0.287 #22647

Merged

48 tasks

[native] Add support for Iceberg connector in presto_protocol #21765

[native] Add support for Iceberg connector in presto_protocol #21765

Conversation

imjalpreet commented Jan 24, 2024 • edited by majetideepak Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

majetideepak commented Jan 30, 2024

imjalpreet commented Jan 30, 2024

majetideepak commented Jan 30, 2024

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

mbasmanova commented Jan 30, 2024

majetideepak commented Jan 30, 2024

mbasmanova commented Jan 30, 2024

tdcmeehan commented Jan 30, 2024

tdcmeehan commented Jan 30, 2024 • edited Loading

steveburnett commented Feb 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditi-pandit left a comment

Choose a reason for hiding this comment

imjalpreet commented Feb 6, 2024

yingsu00 commented Feb 9, 2024

majetideepak commented Feb 10, 2024

imjalpreet commented Feb 12, 2024 • edited Loading

yingsu00 commented Feb 12, 2024

tdcmeehan commented Feb 12, 2024

majetideepak left a comment • edited Loading

Choose a reason for hiding this comment

majetideepak left a comment • edited Loading

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

imjalpreet commented Feb 13, 2024

imjalpreet commented Feb 13, 2024

mbasmanova commented Feb 13, 2024

majetideepak commented Feb 13, 2024

majetideepak commented Feb 13, 2024

majetideepak commented Feb 13, 2024

majetideepak left a comment

Choose a reason for hiding this comment

yingsu00 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

imjalpreet commented Feb 13, 2024

imjalpreet commented Jan 24, 2024 •

edited by majetideepak

Loading

tdcmeehan commented Jan 30, 2024 •

edited

Loading

imjalpreet commented Feb 12, 2024 •

edited

Loading

majetideepak left a comment •

edited

Loading

majetideepak left a comment •

edited

Loading