Refactor Java Hive and Iceberg Connector for Prestissimo Iceberg Connector #21662

imjalpreet · 2024-01-09T20:24:17Z

Description

Related to #21584

Motivation and Context

facebookincubator/velox#5977

Impact

N.A.

Test Plan

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.

== NO RELEASE NOTE ==

yingsu00 · 2024-01-10T14:44:24Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

@@ -3666,6 +3667,7 @@ public TableLayoutFilterCoverage getTableLayoutFilterCoverage(ConnectorTableLayo
    {
        HiveTableLayoutHandle tableHandle = (HiveTableLayoutHandle) connectorTableLayoutHandle;
        Set<String> relevantColumns = tableHandle.getPartitionColumns().stream()
+                .map(HiveColumnHandle.class::cast)


You don't need to cast to HiveColumnHandle

Set<String> relevantColumns = tableHandle.getPartitionColumns().stream() .map(BaseHiveColumnHandle::getName)

yingsu00 · 2024-01-10T14:45:20Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

@@ -2565,6 +2565,7 @@ public boolean supportsMetadataDelete(ConnectorSession session, ConnectorTableHa
                .collect(toImmutableSet());

        Set<String> partitionColumnNames = layoutHandle.getPartitionColumns().stream()
+                .map(HiveColumnHandle.class::cast)


You don't need to cast to HiveColumnHandle

Set<String> relevantColumns = tableHandle.getPartitionColumns().stream() .map(BaseHiveColumnHandle::getName)

yingsu00 · 2024-01-10T14:55:39Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveTableLayoutHandle.java

@@ -129,11 +129,16 @@ protected HiveTableLayoutHandle(
            boolean footerStatsUnreliable,
            Optional<HiveTableHandle> hiveTableHandle)
    {
-        super(domainPredicate, remainingPredicate, pushdownFilterEnabled, partitionColumnPredicate, partitions);
+        super(
+                partitionColumns.stream().map(BaseHiveColumnHandle.class::cast).collect(toList()),


We don't usually do upcasts in the child classes. I think it's better to change the parameter partitionColumns to List<BaseColumnHandle>

yingsu00 · 2024-01-10T14:58:10Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveTableLayoutHandle.java

@@ -333,7 +332,7 @@ public Builder builder()
        return new Builder()
                .setSchemaTableName(getSchemaTableName())
                .setTablePath(getTablePath())
-                .setPartitionColumns(getPartitionColumns())
+                .setPartitionColumns(getPartitionColumns().stream().map(HiveColumnHandle.class::cast).collect(toList()))


Change
public Builder setPartitionColumns(List<HiveColumnHandle> partitionColumns)
to
public Builder setPartitionColumns(List<BaseHiveColumnHandle> partitionColumns)
and do downcast inside.

yingsu00 · 2024-01-10T14:58:26Z

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

@@ -78,21 +79,20 @@ protected IcebergTableLayoutHandle(
            Optional<List<HivePartition>> partitions,
            IcebergTableHandle table)
    {
-        super(domainPredicate, remainingPredicate, pushdownFilterEnabled, partitionColumnPredicate, partitions);
+        super(
+                partitionColumns.stream().map(BaseHiveColumnHandle.class::cast).collect(toList()),


yingsu00 · 2024-01-11T16:17:03Z

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

@@ -80,12 +74,12 @@ public Optional<String> getTableSchemaJson()

    public SchemaTableName getSchemaTableName()
    {
-        return new SchemaTableName(schemaName, tableName.getTableName());
+        return new SchemaTableName(getSchemaName(), getTableName());


getSchemaTableName() is already in BaseHiveTableHandle. Do we still need it here?

yingsu00 · 2024-01-11T16:30:43Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

@@ -2633,7 +2633,7 @@ public List<ConnectorTableLayoutResult> getTableLayouts(ConnectorSession session
                        new HiveTableLayoutHandle.Builder()
                                .setSchemaTableName(handle.getSchemaTableName())
                                .setTablePath(table.getStorage().getLocation())
-                                .setPartitionColumns(hivePartitionResult.getPartitionColumns())
+                                .setPartitionColumns(ImmutableList.copyOf(hivePartitionResult.getPartitionColumns()))


We can make HivePartitionResult.partitionColumns List<HiveColumnHandle>, and make hivePartitionResult.getPartitionColumns() return List<BaseHiveColumnHandle>, and also make the constructor of HivePartitionResult take List<BaseHiveColumnHandle> parameter.

yingsu00 · 2024-01-11T16:38:18Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveTableLayoutHandle.java

@@ -89,7 +89,7 @@ public HiveTableLayoutHandle(
        this(
                schemaTableName,
                tablePath,
-                partitionColumns,
+                partitionColumns.stream().map(BaseHiveColumnHandle.class::cast).collect(toList()),


change the constructor parameter partitionColumns to List<BaseHiveColumnHandle> instead of doing upcasting here

As discussed offline, we won't be able to make this change in the JSON constructor since we don't want the information to get lost while serializing.

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

Refactor HiveTableHandle and IcebergTableHandle to extend BaseHiveTableHandle

imjalpreet self-assigned this Jan 9, 2024

imjalpreet requested a review from a team as a code owner January 9, 2024 20:24

imjalpreet requested a review from presto-oss January 9, 2024 20:24

imjalpreet mentioned this pull request Jan 9, 2024

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

Merged

6 tasks

imjalpreet added the iceberg Apache Iceberg related label Jan 9, 2024

imjalpreet requested a review from yingsu00 January 9, 2024 21:16

imjalpreet force-pushed the icebergPrestissimoJavaChanges branch from 66cffe5 to 9e2428b Compare January 10, 2024 06:44

yingsu00 reviewed Jan 10, 2024

View reviewed changes

imjalpreet force-pushed the icebergPrestissimoJavaChanges branch from 9e2428b to 9c7bd6b Compare January 11, 2024 10:29

imjalpreet marked this pull request as draft January 11, 2024 11:05

imjalpreet force-pushed the icebergPrestissimoJavaChanges branch from 9c7bd6b to c67f699 Compare January 11, 2024 12:26

imjalpreet marked this pull request as ready for review January 11, 2024 13:17

imjalpreet requested a review from yingsu00 January 11, 2024 16:21

yingsu00 reviewed Jan 13, 2024

View reviewed changes

imjalpreet force-pushed the icebergPrestissimoJavaChanges branch from c67f699 to 2cef922 Compare January 18, 2024 10:03

yingsu00 reviewed Jan 18, 2024

View reviewed changes

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergTableHandle.java Outdated Show resolved Hide resolved

imjalpreet added 4 commits January 18, 2024 17:47

Annotate all parameters of DeleteFile constructor with @JsonProperty

e7b1fda

Rename TableType to IcebergTableType in Iceberg Connector

e640255

Move partitionColumns to BaseHiveTableLayoutHandle

ea35d57

Introduce BaseHiveTableHandle

b39ce48

Refactor HiveTableHandle and IcebergTableHandle to extend BaseHiveTableHandle

imjalpreet force-pushed the icebergPrestissimoJavaChanges branch from 2cef922 to 9be76e6 Compare January 18, 2024 12:20

Convert BaseHiveColumnHandle interface into a class

a3ebbb4

imjalpreet force-pushed the icebergPrestissimoJavaChanges branch from 9be76e6 to a3ebbb4 Compare January 18, 2024 12:25

yingsu00 approved these changes Jan 18, 2024

View reviewed changes

yingsu00 merged commit f75f9db into prestodb:master Jan 18, 2024
56 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Java Hive and Iceberg Connector for Prestissimo Iceberg Connector #21662

Refactor Java Hive and Iceberg Connector for Prestissimo Iceberg Connector #21662

imjalpreet commented Jan 9, 2024

yingsu00 Jan 10, 2024

yingsu00 Jan 10, 2024

yingsu00 Jan 10, 2024

yingsu00 Jan 10, 2024

yingsu00 Jan 10, 2024

yingsu00 Jan 11, 2024

yingsu00 Jan 11, 2024

yingsu00 Jan 11, 2024

imjalpreet Jan 18, 2024

Refactor Java Hive and Iceberg Connector for Prestissimo Iceberg Connector #21662

Refactor Java Hive and Iceberg Connector for Prestissimo Iceberg Connector #21662

Conversation

imjalpreet commented Jan 9, 2024

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

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