Add support to be able to query new Delta protocols by mblanco-denodo · Pull Request #22596 · prestodb/presto

mblanco-denodo · 2024-04-24T11:49:20Z

Description

Use of delta-kernel-api instead of delta-standalone-api in order to be able to query delta tables in newer protocols (delta reader 3, delta writer 7)
Added new configurable parameter (delta.case-sensitive-partitions-enabled) to allow case sensitive partition matching, allowing to query tables with partition names in uppercase. By default it is set to true.

Motivation and Context

Solves #22543 and #21828.

Impact

Based on some query executions over some data in S3 (times in seconds):

Table	master	branch
nation (tpch100)	63.06	18.92
region (tpch100)	50.25	16.05
deltatbl_partition_prune	33.63	4.98

Also experienced huge improvement on some queries during testing. Here are the HTML reports of the tests suites in master and in the branch:
master_vs_branch_test_execution_performance.zip

Test Plan

Test refactor so that the current tests are all executed on older and newer versions of the same test data.

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 ==

Delta Connector Changes

Added support to read tables up to Delta Protocol Version 3,7 through the use of the new Delta-Kernel api. :pr:22596
Added new boolean configuration parameter delta.case-sensitive-partitions-enabled to be able to query data with partitioned columns with column names in uppercase. This property is set to true by default. :pr:22596

mblanco-denodo · 2024-04-24T12:07:10Z

PR set to draft while waiting for the delta kernel api to release v3.2.0. https://github.com/delta-io/delta/milestone/26
Current limitations that will be solved on 3.2.0:

Move to an snapshot different from the latest.
Move to an snapshot by timestamp
Reading partition columns of TimestampType is unsupported

steveburnett · 2024-06-18T17:52:59Z

Suggest adding the PR # to the items in the release note entry, for example:

== RELEASE NOTES ==

Delta Connector Changes
* Added support to read tables up to Delta Protocol Version 3,7 through the use of the new Delta-Kernel API. :pr:`22596`
* Added new boolean configuration parameter ``delta.case-sensitive-partitions-enabled`` to be able to query data with partitioned columns with column names in uppercase. This property is set to ``true`` by default. :pr:`22596`

mblanco-denodo · 2024-06-28T10:30:27Z

After the release of Delta Kernel 3.2.0, all limitations are resolved. Also, it seems that there is an improvement in performance reading both delta v1 and delta v3 (perfomance local tests attached in the PR description). PR is ready for review.

tdcmeehan

Just some initial minor comments, great work!

tdcmeehan · 2024-06-28T13:26:45Z

presto-delta/pom.xml

            <scope>test</scope>
        </dependency>
+        <dependency>
+            <groupId>org.jetbrains</groupId>


What is this dependency used for?

It was used to annotate sql queries in testing. I removed the annotations and the dependency.

tdcmeehan · 2024-06-28T13:27:01Z

presto-delta/src/main/java/com/facebook/presto/delta/DeltaClient.java

+        Optional<Engine> deltaEngine = loadDeltaTableClient(session, location,
+                schemaTableName);


Suggested change

Optional<Engine> deltaEngine = loadDeltaTableClient(session, location,

schemaTableName);

Optional<Engine> deltaEngine = loadDeltaTableClient(session, location, schemaTableName);

tdcmeehan · 2024-06-28T13:28:17Z

presto-delta/src/main/java/com/facebook/presto/delta/DeltaClient.java

+        try {
+            CloseableIterator<FilteredColumnarBatch> columnBatches = snapshot.getScanBuilder(deltaEngine)
+                    .build().getScanFiles(deltaEngine);
+            Row row = null;
+            while (columnBatches.hasNext()) {
+                CloseableIterator<Row> rows = columnBatches.next().getRows();
+                if (rows.hasNext()) {
+                    row = rows.next();
+                    break;
+                }
+            }
+            Map<String, String> partitionValues = row != null ?
+                    InternalScanFileUtils.getPartitionValues(row) : new HashMap<>(0);
+            columnBatches.close();


Suggested change

try {

CloseableIterator<FilteredColumnarBatch> columnBatches = snapshot.getScanBuilder(deltaEngine)

.build().getScanFiles(deltaEngine);

Row row = null;

while (columnBatches.hasNext()) {

CloseableIterator<Row> rows = columnBatches.next().getRows();

if (rows.hasNext()) {

row = rows.next();

break;

}

}

Map<String, String> partitionValues = row != null ?

InternalScanFileUtils.getPartitionValues(row) : new HashMap<>(0);

columnBatches.close();

try (CloseableIterator<FilteredColumnarBatch> columnBatches = snapshot.getScanBuilder(deltaEngine).build().getScanFiles(deltaEngine)) {

Row row = null;

while (columnBatches.hasNext()) {

CloseableIterator<Row> rows = columnBatches.next().getRows();

if (rows.hasNext()) {

row = rows.next();

break;

}

}

Map<String, String> partitionValues = row != null ?

InternalScanFileUtils.getPartitionValues(row) : new HashMap<>(0);

columnBatches.close();

tdcmeehan · 2024-06-28T13:29:56Z

presto-delta/src/main/java/com/facebook/presto/delta/DeltaExpressionUtils.java

+        List<DeltaColumnHandle> partitionColumns = columnDomains.map(domains -> domains.stream()
+                .filter(entry -> entry.getColumn().getColumnType() == PARTITION)
+                .map(TupleDomain.ColumnDomain::getColumn)
+                .collect(Collectors.toList())).orElse(Collections.emptyList());


Suggested change

.collect(Collectors.toList())).orElse(Collections.emptyList());

.collect(toImmutableList())).orElse(ImmutableList.of());

tdcmeehan · 2024-06-28T13:30:18Z

presto-delta/src/main/java/com/facebook/presto/delta/DeltaExpressionUtils.java

+                row = nextFile.getRows();
+            }
+            else {
+                logger.debug("There are still rows remaining int the iterator, not advancing the iterator");


Perhaps remove this

steveburnett

A single nit of punctuation, otherwise LGTM (docs). Thanks!

steveburnett · 2024-06-28T14:26:36Z

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

@@ -41,6 +41,8 @@ Property Name                                   Description
                                                In order for this option to work, also set
                                                ``experimental.pushdown-dereference-enabled`` to
                                                ``true``.
+``delta.case-sensitive-partitions-enabled``     Allows matching the names of partitioned columns in a     ``true``
+                                                case-sensitive manner


Suggested change

case-sensitive manner

case-sensitive manner.

agrawalreetika

Thanks for the PR. Had few tables created with new Protocol earlier but wasn't accessible before this new Protocol changes support. Now able to access those tables.
Dropped in few minor comments, please check!

You mentioned about performance improvement, Just curious which part of the new changes are contributing to performance improvement while reading the table?

agrawalreetika · 2024-06-30T19:04:57Z

presto-delta/src/main/java/com/facebook/presto/delta/DeltaClient.java

+                        format("Could not move to latest snapshot on table '%s.%s'", schemaTableName.getSchemaName(),
+                                schemaTableName.getTableName()));
+            }
        }


For better readability, should we move this code block into a method something like getSnapshot instead?

agrawalreetika · 2024-06-30T19:10:34Z

presto-delta/src/main/java/com/facebook/presto/delta/DeltaClient.java

     */
-    public CloseableIterator<AddFile> listFiles(ConnectorSession session, DeltaTable deltaTable)
+    public CloseableIterator<FilteredColumnarBatch> listFiles(ConnectorSession session, DeltaTable deltaTable)
    {


We can do null check for deltaTable here -
requireNonNull(deltaTable, "deltaTable is null");

Also I think, we can shift deltaTable.getSnapshotId().isPresent() check in here in the start itself from line 148.

agrawalreetika · 2024-06-30T19:27:20Z

presto-delta/src/main/java/com/facebook/presto/delta/DeltaExpressionUtils.java

+                    row.close();
+                }
+                catch (IOException e) {
+                    throw new GenericInternalException("Cloud not close row batch", e);


Suggested change

throw new GenericInternalException("Cloud not close row batch", e);

throw new GenericInternalException("Could not close row batch", e);

agrawalreetika · 2024-06-30T19:37:06Z

presto-delta/src/test/java/com/facebook/presto/delta/AbstractDeltaDistributedQueryTestBase.java

     * @param hiveTableName Name of the Hive table that the Delta table is to be registered as in HMS
     */
-    private static void registerDeltaTableInHMS(QueryRunner queryRunner, String deltaTableName, String hiveTableName)
+    protected static void registerDeltaTableInHMS(QueryRunner queryRunner, String deltaTableName, String hiveTableName)


Don't see usage of this outside this class. Any reason for changing this from private?

I intended in use it in other test class but at the end I didn't and I forgot to change the method visibility back. Reverted to private.

mblanco-denodo · 2024-07-01T10:39:43Z

@agrawalreetika performance improvement is due to the new Kernel API.

steveburnett

LGTM! (docs)

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

agrawalreetika · 2024-07-03T14:47:09Z

@mblanco-denodo Could you please squash the commits?
I think here 2 commits would be good. One for changes for delta.case-sensitive-partitions-enabled and other for the changes in Delta protocols

mblanco-denodo · 2024-07-03T16:42:40Z

@agrawalreetika commits squashed into two as requested:

changes in delta protocol from standalone to kernel
changes with the new property for uppercase column processing

tdcmeehan · 2024-07-05T17:28:05Z

presto-delta/src/main/java/com/facebook/presto/delta/DeltaClient.java

+        Optional<Engine> deltaEngine = loadDeltaTableClient(session,
                new Path(deltaTable.getTableLocation()),
                new SchemaTableName(deltaTable.getSchemaName(), deltaTable.getTableName()));
+        if (!deltaEngine.isPresent()) {


Do you know under what circumstances this error could happen?

Right now it should not happen, as the method loadDeltaTableClient will return a non null Engine or throw an exception. The usage of Optional in that method is inherited from the previous code, and it is a good practice to check if deltaEngine.isPresent() as in future iterations the logic of loadDeltaTableClient could change (note that all the Delta Kernel API is annotated with the @Evolving annotation, and they are making big API changes even on minor versions).

imjalpreet · 2024-07-08T09:10:07Z

presto-delta/src/main/java/com/facebook/presto/delta/DeltaConfig.java

+        return this.caseSensitivePartitionsEnabled;
+    }
+
+    @Config("delta.case-sensitive-partitions-enabled")


To get a little clarity, was the issue of not being able to query mixed-case/upper-case columns only happening if the column is a partitioned column? Can we also have regular columns as mixed case or upper case, and if yes, does it work as expected?

We've experienced the problem only on partitioned columns, as the non partitioned columns are in parquet data and reading parquet the case issue does not happen. However the fix should work on both cases.

The reason I wanted to get an idea is we have removed the toLowerCase conversion for all columns not just the partitioned columns. That's why wanted to make sure it does not have any side effects for regular columns and if it's required even for regular columns, should we rename the config to delta.case-sensitive-columns-enabled?

I can add a test on regular columns with and without the property to be sure

I confirm that with delta.case-sensitive-partitions-enabled=false uppercase non partitioned columns are properly returning the data, so I think we should leave the property name as it is because it only changes the behavior to partitioned columns. I added the new test that compares the output between a table with a non partitioned column in lowercase and other table with the same column in uppercase to assure that there is no side effect and both tables return the same data.

Thanks for adding the tests and confirming.

steveburnett

LGTM! (docs)
Pull updated branch, local doc build, looks good. Thanks!

imjalpreet

LGTM 👍🏼

@mblanco-denodo Can you please rebase on master to fix the failing test suite?

elharo · 2024-07-19T13:51:30Z

presto-delta/src/main/java/com/facebook/presto/delta/DeltaClient.java

+                snapshot = deltaTable.getLatestSnapshot(deltaEngine); // get the latest snapshot
+            }
+            catch (TableNotFoundException e) {
+                throw new PrestoException(GENERIC_INTERNAL_ERROR,


embed the original exception as the cause here

Also this should be a user error. Can you check in StandardErrorCodes and DeltaErrorCode to check to see if there's an appropriate user exception for this? If not, please add one.

Done. Added new DeltaErrorCodes to candle these cases

elharo · 2024-07-19T13:52:15Z

presto-delta/src/main/java/com/facebook/presto/delta/DeltaClient.java

+            String format = ((SnapshotImpl) snapshot).getMetadata().getFormat().getProvider();
+            if (!PARQUET.name().equalsIgnoreCase(format)) {
+                throw new PrestoException(DELTA_UNSUPPORTED_DATA_FORMAT,
+                        format("Delta table %s has unsupported data format: %s. Currently only Parquet data format is supported", schemaTableName, format));


Currently only --> Only the

elharo · 2024-07-19T13:52:40Z

presto-delta/src/main/java/com/facebook/presto/delta/DeltaClient.java

+        catch (TableNotFoundException e) {
            throw new PrestoException(NOT_FOUND,
-                    format("Delta table (%s.%s) no longer exists.", deltaTable.getSchemaName(), deltaTable.getTableName()));
+                    format("Delta table not found in '%s'", deltaTable.getTableLocation()));


nest exception

elharo · 2024-07-19T13:54:39Z

presto-delta/src/test/java/com/facebook/presto/delta/TestDeltaScanOptimizations.java

            Map<String, Domain> expectedConstraint,
            Map<String, Domain> expectedEnforcedConstraint)
    {
+        System.out.println(tableScanWithConstraints(tableName, expectedConstraint, expectedEnforcedConstraint).toString());


remove this line

tdcmeehan · 2024-07-19T16:10:46Z

presto-delta/src/main/java/com/facebook/presto/delta/DeltaClient.java

                new Path(deltaTable.getTableLocation()),
                new SchemaTableName(deltaTable.getSchemaName(), deltaTable.getTableName()));
+        if (!deltaEngine.isPresent()) {
+            throw new PrestoException(GENERIC_INTERNAL_ERROR,


Please throw a system error here. GENERIC_INTERNAL_ERROR is an error for anything that is not categorized, so we shouldn't be intentionally throwing it.

tdcmeehan · 2024-07-19T16:11:18Z

presto-delta/src/main/java/com/facebook/presto/delta/DeltaClient.java

+                    format(TABLE_NOT_FOUND_ERROR_TEMPLATE, tableName.getSchemaName(), tableName.getTableName()));
+        }
+        catch (IOException e) {
+            throw new GenericInternalException("Could not close columnar batch row", e);


Please consider rethrowing an UncheckedIOException

abhiseksaikia · 2024-08-02T07:58:35Z

@mblanco-denodo The directory structure for the tests are pretty long and its breaking our build due to internal limit on path length (<490).

Can we have a fix for this? cc : @tdcmeehan

tdcmeehan · 2024-08-02T13:33:20Z

This refactoring only made the directory structure slightly larger, by introducing the delta_v1/delta_v3 prefix. Otherwise it's the same directory structure, which is what is specified by the Delta format. While we could collapse this to something smaller like v3-partition-values and remove that prefix directory, long term I'd recommend finding a way to increase the limit on the directory structure so you don't encounter this again in the future @abhiseksaikia.

mblanco-denodo force-pushed the issue_22543 branch 2 times, most recently from ee8caf0 to da3c3ae Compare April 24, 2024 12:01

tdcmeehan self-assigned this May 14, 2024

mblanco-denodo force-pushed the issue_22543 branch from 222fcb3 to 2118c6e Compare June 28, 2024 10:13

mblanco-denodo marked this pull request as ready for review June 28, 2024 10:13

mblanco-denodo requested review from a team and steveburnett as code owners June 28, 2024 10:13

mblanco-denodo requested a review from presto-oss June 28, 2024 10:13

tdcmeehan requested changes Jun 28, 2024

View reviewed changes

tdcmeehan requested review from agrawalreetika and imjalpreet June 28, 2024 13:32

steveburnett previously approved these changes Jun 28, 2024

View reviewed changes

agrawalreetika requested changes Jun 30, 2024

View reviewed changes

mblanco-denodo dismissed steveburnett’s stale review via cf45e29 July 1, 2024 10:29

mblanco-denodo force-pushed the issue_22543 branch from 2118c6e to cf45e29 Compare July 1, 2024 10:29

mblanco-denodo requested review from agrawalreetika, steveburnett and tdcmeehan July 1, 2024 11:31

steveburnett previously approved these changes Jul 1, 2024

View reviewed changes

mblanco-denodo force-pushed the issue_22543 branch from cf45e29 to 8b33456 Compare July 3, 2024 16:38

agrawalreetika previously approved these changes Jul 4, 2024

View reviewed changes

tdcmeehan reviewed Jul 5, 2024

View reviewed changes

imjalpreet reviewed Jul 8, 2024

View reviewed changes

mblanco-denodo dismissed agrawalreetika’s stale review via 0142f1e July 10, 2024 10:10

mblanco-denodo dismissed steveburnett’s stale review via 0142f1e July 10, 2024 10:10

mblanco-denodo force-pushed the issue_22543 branch from 8b33456 to 0142f1e Compare July 10, 2024 10:10

mblanco-denodo requested review from imjalpreet, steveburnett and tdcmeehan July 10, 2024 12:21

steveburnett previously approved these changes Jul 10, 2024

View reviewed changes

imjalpreet previously approved these changes Jul 19, 2024

View reviewed changes

mblanco-denodo force-pushed the issue_22543 branch from 0142f1e to 1608031 Compare July 19, 2024 13:10

mblanco-denodo requested a review from elharo as a code owner July 19, 2024 13:10

elharo reviewed Jul 19, 2024

View reviewed changes

tdcmeehan reviewed Jul 19, 2024

View reviewed changes

mblanco-denodo added 2 commits July 29, 2024 13:09

Add support to be able to query new Delta protocols

5b4956c

Add support to read uppercase partition columns

70601f6

mblanco-denodo dismissed stale reviews from steveburnett and imjalpreet via 70601f6 July 29, 2024 12:29

mblanco-denodo force-pushed the issue_22543 branch from 1608031 to 70601f6 Compare July 29, 2024 12:29

mblanco-denodo requested review from elharo and tdcmeehan July 30, 2024 07:01

tdcmeehan approved these changes Jul 31, 2024

View reviewed changes

tdcmeehan merged commit 2384402 into prestodb:master Jul 31, 2024

dnskr mentioned this pull request Jul 31, 2024

Upgrade Delta Standalone to 3.0.0 #20256

Closed

tdcmeehan mentioned this pull request Aug 23, 2024

Add release notes for 0.289 #23513

Merged

34 tasks

imjalpreet mentioned this pull request Nov 4, 2024

Be able to query uppercase partitioned columns on delta tables #21828

Closed

		Optional<Engine> deltaEngine = loadDeltaTableClient(session, location,
		schemaTableName);

	.collect(Collectors.toList())).orElse(Collections.emptyList());
	.collect(toImmutableList())).orElse(ImmutableList.of());

	throw new GenericInternalException("Cloud not close row batch", e);
	throw new GenericInternalException("Could not close row batch", e);

Conversation

mblanco-denodo commented Apr 24, 2024 • edited by steveburnett 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!

mblanco-denodo commented Apr 24, 2024

Uh oh!

steveburnett commented Jun 18, 2024

Uh oh!

mblanco-denodo commented Jun 28, 2024

Uh oh!

tdcmeehan 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!

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!

steveburnett 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!

agrawalreetika 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!

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!

mblanco-denodo commented Jul 1, 2024

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

agrawalreetika commented Jul 3, 2024

Uh oh!

mblanco-denodo commented Jul 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mblanco-denodo Jul 8, 2024 • 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!

mblanco-denodo commented Apr 24, 2024 •

edited by steveburnett

Loading

mblanco-denodo Jul 8, 2024 •

edited

Loading