test: Refactor DeltaQueryRunner and add NativeDeltaQueryRunner support by mohsaka · Pull Request #26815 · prestodb/presto

mohsaka · 2025-12-16T22:42:31Z

Description

To prepare for delta prestissimo support and following the organization of hive and iceberg, separate out a DeltaQueryRunner from existing delta test cases. Also add in delta native query runner execution support.

Motivation and Context

We will be adding in delta connector support in Prestissimo. This is to run the current Java delta tests against Prestissimo in a future PR.

Impact

None

Test Plan

Ran test cases:
IncrementalUpdateQueriesTest
TestUppercasePartitionColumns
TestDeltaScanOptimizations
TestDeltaIntegration

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.
If adding new dependencies, verified they have an OpenSSF Scorecard score of 5.0 or higher (or obtained explicit TSC approval for lower scores).

Release Notes

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

== NO RELEASE NOTE ==

sourcery-ai · 2025-12-16T22:42:54Z

Reviewer's Guide

Refactors Delta connector tests to share a reusable DeltaQueryRunner, and introduces a native-execution-aware DeltaQueryRunner builder wired into PrestoNativeQueryRunnerUtils, plus required test dependencies for running Delta tests against both Java and native engines.

Class diagram for refactored DeltaQueryRunner and native integration

classDiagram

class AbstractDeltaDistributedQueryTestBase {
  -QueryRunner queryRunner
  +void createQueryRunner()
  +void tearDown()
}

class DeltaQueryRunner {
  +QueryRunner createDeltaQueryRunner()
  +QueryRunner createDeltaQueryRunnerWithFiles(String catalogName, String schemaName)
  +QueryRunner createDeltaQueryRunnerWithNativeSupport()
}

class NativeDeltaQueryRunner {
  +QueryRunner createNativeDeltaQueryRunner()
}

class PrestoNativeQueryRunnerUtils {
  +QueryRunner createDeltaNativeQueryRunner()
}

AbstractDeltaDistributedQueryTestBase --> DeltaQueryRunner : uses
PrestoNativeQueryRunnerUtils --> NativeDeltaQueryRunner : uses
NativeDeltaQueryRunner --> DeltaQueryRunner : delegates_configuration

File-Level Changes

Change	Details	Files
Introduce reusable DeltaQueryRunner test utility with configurable timezone, node count, external worker launcher, and case-sensitive partition support.	Added DeltaQueryRunner class with nested Builder to encapsulate DistributedQueryRunner setup for Delta tests, including session, catalogs, and plugins. Moved Delta catalog/Hive catalog bootstrap logic from individual tests into DeltaQueryRunner.Builder.build(). Added configuration flags for external worker launcher, case-sensitive partitions, node count, and extra properties for flexibility across Java and native runners.	`presto-delta/src/test/java/com/facebook/presto/delta/DeltaQueryRunner.java`
Refactor existing Delta tests to use DeltaQueryRunner instead of inlined DistributedQueryRunner setup.	Updated AbstractDeltaDistributedQueryTestBase to create the query runner via DeltaQueryRunner.builder().setExtraProperties(...).build().getQueryRunner(). Exposed DELTA_TEST_TABLE_LIST as public so it can be reused by other test harnesses. Removed the private createDeltaQueryRunner helper from AbstractDeltaDistributedQueryTestBase and inlined responsibilities into DeltaQueryRunner. Updated TestUppercasePartitionColumns to construct its QueryRunner via DeltaQueryRunner.builder() with a specific TimeZoneKey and caseSensitivePartitions flag, removing its local createDeltaQueryRunner implementation and related imports.	`presto-delta/src/test/java/com/facebook/presto/delta/AbstractDeltaDistributedQueryTestBase.java` `presto-delta/src/test/java/com/facebook/presto/delta/TestUppercasePartitionColumns.java`
Add native and Java Delta query runner builders and wiring for native execution, including external worker launcher support.	Introduced DeltaQueryRunnerBuilder in PrestoNativeQueryRunnerUtils with support for native and Java QueryRunnerType, configuring default properties and connector properties appropriately. Implemented nativeDeltaQueryRunnerBuilder() and javaDeltaQueryRunnerBuilder() factory methods to construct DeltaQueryRunnerBuilder instances. Wired DeltaQueryRunnerBuilder.build() to optionally create an external worker launcher (for native runs) and delegate to DeltaQueryRunner.Builder with node count, extra properties, timezone, and case-sensitive partitions. Added DELTA_DEFAULT_STORAGE_FORMAT constant for consistency with existing storage format constants.	`presto-native-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java`
Wire Delta test artifacts into the Maven build to allow native-execution tests to depend on Delta test utilities and data.	Added presto-delta test-jar and main test-scope dependencies, plus snappy-java test dependency, to presto-native-execution/pom.xml so native tests can use DeltaQueryRunner and Delta test data. Registered presto-delta test-jar as a managed dependency in the top-level pom.xml for reuse by modules.	`presto-native-execution/pom.xml` `pom.xml`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey there - I've reviewed your changes - here's some feedback:

In DeltaQueryRunnerBuilder the fields dataDirectory, catalogType, extraConnectorProperties, and DELTA_DEFAULT_STORAGE_FORMAT are never used; either wire them into the builder logic or remove them to keep the delta runner configuration minimal and clearer.
The comment above useExternalWorkerLauncher in DeltaQueryRunnerBuilder still refers to "the native iceberg query runner" and should be updated to accurately describe the delta-specific behavior.
There is a typo in DeltaQueryRunnerBuilder: the field caseSensitiveParitions and its uses should be renamed to caseSensitivePartitions for consistency and readability.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `DeltaQueryRunnerBuilder` the fields `dataDirectory`, `catalogType`, `extraConnectorProperties`, and `DELTA_DEFAULT_STORAGE_FORMAT` are never used; either wire them into the builder logic or remove them to keep the delta runner configuration minimal and clearer.
- The comment above `useExternalWorkerLauncher` in `DeltaQueryRunnerBuilder` still refers to "the native iceberg query runner" and should be updated to accurately describe the delta-specific behavior.
- There is a typo in `DeltaQueryRunnerBuilder`: the field `caseSensitiveParitions` and its uses should be renamed to `caseSensitivePartitions` for consistency and readability.

## Individual Comments

### Comment 1
<location> `presto-delta/src/test/java/com/facebook/presto/delta/DeltaQueryRunner.java:76-78` </location>
<code_context>
+            return this;
+        }
+
+        public Builder setExtraProperties(Map<String, String> extraProperties)
+        {
+            this.extraProperties = extraProperties;
+            return this;
+        }
</code_context>

<issue_to_address>
**nitpick (bug_risk):** Defensive copy of extraProperties would make the shared test query runner more robust

This stores the `extraProperties` map reference directly, so later mutations of the original map could affect other tests using the shared runner. To keep tests isolated, make a defensive copy here (e.g., `this.extraProperties = new HashMap<>(extraProperties);` or `ImmutableMap.copyOf(extraProperties)`).
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

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

aditi-pandit

Thanks @mohsaka for this refactoring. Have few comments.

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

...e-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java

aditi-pandit · 2025-12-17T04:08:21Z

...e-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java

        }
    }

+    public static DeltaQueryRunnerBuilder nativeDeltaQueryRunnerBuilder()


This use of DeltaQueryRunnerBuilder in both places is hard to read. Does the nested DeltaQueryRunnerBuilder really need to be a class ? Can these be just functions instead ?

Currently following what both Hive and Iceberg have through

public static IcebergQueryRunnerBuilder nativeIcebergQueryRunnerBuilder() { return new IcebergQueryRunnerBuilder(QueryRunnerType.NATIVE); } public static IcebergQueryRunnerBuilder javaIcebergQueryRunnerBuilder() { return new IcebergQueryRunnerBuilder(QueryRunnerType.JAVA); }

and

public static HiveQueryRunnerBuilder nativeHiveQueryRunnerBuilder() { return new HiveQueryRunnerBuilder(QueryRunnerType.NATIVE); } public static HiveQueryRunnerBuilder javaHiveQueryRunnerBuilder() { return new HiveQueryRunnerBuilder(QueryRunnerType.JAVA); }

These are functions and can be called via the Utility class ex.
QueryRunner queryRunner = PrestoNativeQueryRunnerUtils.nativeDeltaQueryRunnerBuilder().build();

So what was hard for me to get my head around was that there is DeltaQueryRunner and DeltaQueryRunner.Builder and then there is DeltaQueryRunnerBuilder.build as well. And DeltaQueryRunnerBuilder.build returns a DeltaQueryRunner.

You are right that this degree of indirection is there for Hive and Iceberg as well. Though would it be possible to eliminate it ?

We could eliminate it, but I believe the reason we have it is for two things. Maybe I can add a comment explaining this.

We have a DeltaQueryRunner. This comes with a builder DeltaQueryRunner builder. This is used for Java test cases.

We have a NativeDeltaQueryRunner builder. This one is used for native tests and can create both Java and Native query runners so that they can do comparisons.

The NativeDeltaQueryRunner has a builder that calls DeltaQueryRunner builder. We mainly don't want to have native code in the java code. So thats why theres this separation of classes.

If we want to have one, we can replace the DeltaQueryRunner with one that does native and java. But then we will be going through native code on the java code path. When it should only be the other way around. We would also have to move over all of the native utilities for the setup into the presto side somewhere.

I think the reason it is not called NativeDeltaQueryRunnerBuilder is because it can make both native and Java. Also its already under presto-native-execution.

Added comment explaining this in more detail.

Thanks for explaining. I see the distinction now... We can leave the original DeltaQueryRunner as be.

aditi-pandit

Thanks @mohsaka

aditi-pandit · 2025-12-19T06:56:11Z

...e-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java

        }
    }

+    public static DeltaQueryRunnerBuilder nativeDeltaQueryRunnerBuilder()


Thanks for explaining. I see the distinction now... We can leave the original DeltaQueryRunner as be.

aditi-pandit

Thanks @mohsaka

tdcmeehan

Just some nits, LGTM, thanks for this refactoring

tdcmeehan · 2026-01-06T16:35:06Z

.github/workflows/prestocpp-linux-build-and-unit-test.yml

@@ -39,6 +42,22 @@ jobs:
    permissions:
      contents: read
    steps:
+      # We cannot use the github action to free disk space from the runner
+      # because we are in the container and not on the runner anymore.
+      - name: Free Disk Space
+        run: |
+          # Re-used from free-disk-space github action.
+          getAvailableSpace() { echo $(df -a $1 | awk 'NR > 1 {avail+=$4} END {print avail}'); }
+          # Show before
+          echo "Original available disk space: " $(getAvailableSpace)
+          # Remove DotNet.
+          rm -rf /host_usr/share/dotnet || true
+          # Remove android
+          rm -rf /host_usr/local/lib/android || true
+          # Remove CodeQL
+          rm -rf /host_opt/hostedtoolcache/CodeQL || true
+          # Show after
+          echo "New available disk space: " $(getAvailableSpace)


Can you please put this in a separate PR?

Removed as it's no longer necessary per #26876

tdcmeehan · 2026-01-06T16:35:42Z

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

+
+        public Builder setExternalWorkerLauncher(Optional<BiFunction<Integer, URI, Process>> externalWorkerLauncher)
+        {
+            this.externalWorkerLauncher = externalWorkerLauncher;


Suggested change

this.externalWorkerLauncher = externalWorkerLauncher;

this.externalWorkerLauncher = requireNonNull(externalWorkerLauncher, "externalWorkerLauncher is null");

Please do this for the other methods too, and consider the Sourcery feedback to make a defensive copy for extraProperties.

tdcmeehan · 2026-01-06T16:37:56Z

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

+import static java.util.Locale.US;
+import static java.util.Objects.requireNonNull;
+
+public class DeltaQueryRunner


Perhaps let's add a setupLogging method as IcebergQueryRunner does, so we don't spam the logs.

Co-authored-by: unidevel <unidevel@hotmail.com>

mohsaka · 2026-01-07T18:44:19Z

@tdcmeehan Thanks for the review! I have addressed the comments.

prestodb#26815) ## Description  To prepare for delta prestissimo support and following the organization of hive and iceberg, separate out a DeltaQueryRunner from existing delta test cases. Also add in delta native query runner execution support. ## Motivation and Context   We will be adding in delta connector support in Prestissimo. This is to run the current Java delta tests against Prestissimo in a future PR. ## Impact  None ## Test Plan  Ran test cases: IncrementalUpdateQueriesTest TestUppercasePartitionColumns TestDeltaScanOptimizations TestDeltaIntegration ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#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](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. - [ ] If adding new dependencies, verified they have an [OpenSSF Scorecard](https://securityscorecards.dev/#the-checks) score of 5.0 or higher (or obtained explicit TSC approval for lower scores). ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == NO RELEASE NOTE == ``` Co-authored-by: unidevel <unidevel@hotmail.com>

mohsaka assigned unidevel and mohsaka Dec 16, 2025

mohsaka added the tests label Dec 16, 2025

mohsaka requested review from a team as code owners December 16, 2025 22:42

prestodb-ci added the from:IBM PR from IBM label Dec 16, 2025

prestodb-ci requested review from a team, BryanCutler and ScrapCodes and removed request for a team December 16, 2025 22:42

sourcery-ai bot reviewed Dec 16, 2025

View reviewed changes

presto-delta/src/test/java/com/facebook/presto/delta/DeltaQueryRunner.java Outdated Show resolved Hide resolved

mohsaka force-pushed the delta-test-refactor branch from 9bc84a3 to 6d743b4 Compare December 16, 2025 23:46

aditi-pandit reviewed Dec 17, 2025

View reviewed changes

mohsaka force-pushed the delta-test-refactor branch from e6de878 to c2c9dab Compare December 17, 2025 08:52

unidevel requested review from czentgr and unidevel as code owners December 17, 2025 11:12

mohsaka force-pushed the delta-test-refactor branch 3 times, most recently from 8d6329e to c78cb7e Compare December 17, 2025 19:33

aditi-pandit previously approved these changes Dec 19, 2025

View reviewed changes

mohsaka dismissed aditi-pandit’s stale review via ba0d0ea January 2, 2026 16:40

mohsaka force-pushed the delta-test-refactor branch 4 times, most recently from 42a7092 to 7e0d554 Compare January 2, 2026 20:06

mohsaka requested a review from aditi-pandit January 3, 2026 00:31

aditi-pandit previously approved these changes Jan 3, 2026

View reviewed changes

tdcmeehan self-assigned this Jan 5, 2026

tdcmeehan reviewed Jan 6, 2026

View reviewed changes

mohsaka dismissed aditi-pandit’s stale review via c6bd48b January 6, 2026 17:57

mohsaka force-pushed the delta-test-refactor branch 2 times, most recently from c6bd48b to 5cb0b9d Compare January 6, 2026 17:59

Refactor DeltaQueryRunner and add native support

7e89f82

Co-authored-by: unidevel <unidevel@hotmail.com>

mohsaka force-pushed the delta-test-refactor branch from 5cb0b9d to 7e89f82 Compare January 6, 2026 19:19

mohsaka requested a review from tdcmeehan January 6, 2026 21:53

tdcmeehan approved these changes Jan 8, 2026

View reviewed changes

mohsaka merged commit d110d02 into prestodb:master Jan 8, 2026
109 of 113 checks passed

This was referenced Mar 31, 2026

docs: Add release notes for 0.297 unix280/presto#51

Closed

docs: Add release notes for 0.297 unix280/presto#52

Open

	this.externalWorkerLauncher = externalWorkerLauncher;
	this.externalWorkerLauncher = requireNonNull(externalWorkerLauncher, "externalWorkerLauncher is null");

Conversation

mohsaka commented Dec 16, 2025

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

sourcery-ai bot commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide

Class diagram for refactored DeltaQueryRunner and native integration

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aditi-pandit left a comment

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!

mohsaka Dec 17, 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!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

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!

mohsaka commented Jan 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sourcery-ai bot commented Dec 16, 2025 •

edited

Loading

mohsaka Dec 17, 2025 •

edited

Loading