Add planner support to prevent mixed CPP/Java execution by tdcmeehan · Pull Request #25384 · prestodb/presto

tdcmeehan · 2025-06-19T20:10:40Z

Description

System table scans must execute on the coordinator, but CPP (native/non-Java) functions cannot run on the coordinator - they must execute on workers. When a query contains both system table scans and non-Java functions, the planner needs to ensure that there's an exchange boundary between the scan and filter/projection, and undo any filter pushdown that may have occured.

Motivation and Context

This problem was discovered during testing support of only CPP function validation, without supporting constant folding. This is an inevitable problem, as with the Presto sidecar built-in functions are now being reported as being C++, which are ineligible for execution on the coordinator.

Impact

Fix the above issue.

Test Plan

Tests have been added in this PR.

Contributor checklist

== NO RELEASE NOTE ==

Copilot

Pull Request Overview

This PR adds planner support to prevent mixed CPP/Java execution by ensuring that system table scans (which run on the coordinator) are separated by an exchange from non-Java (CPP) functions that must run on workers. Key changes include updating test expectations in the native sidecar plugin, adding utility methods in RowExpressionUtils, and modifying multiple iterative rule sets (RemoveRedundantExchangeRuleSet, ExtractSystemTableFilterRuleSet, and PickTableLayout) to properly handle function eligibility and exchange boundaries.

Reviewed Changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
presto-native-sidecar-plugin/src/test/java/com/facebook/presto/sidecar/TestNativeSidecarPlugin.java	Updated test for information_schema tables to reflect new behavior
presto-main-base/src/main/java/com/facebook/presto/sql/relational/RowExpressionUtils.java	Added method to detect non-coordinator-eligible call expressions
presto-main-base/src/main/java/com/facebook/presto/sql/planner/iterative/rule/RemoveRedundantExchangeRuleSet.java	Added rules to remove unnecessary exchanges for system table scans
presto-main-base/src/main/java/com/facebook/presto/sql/planner/iterative/rule/PickTableLayout.java	Updated predicate handling to avoid partition pruning on ineligible expressions
presto-main-base/src/main/java/com/facebook/presto/sql/planner/iterative/rule/ExtractSystemTableFilterRuleSet.java	Added extraction rules for system table filters
presto-main-base/src/main/java/com/facebook/presto/sql/planner/PlanOptimizers.java	Registered the new rules into the plan optimizer pipeline

Comments suppressed due to low confidence (2)

presto-native-sidecar-plugin/src/test/java/com/facebook/presto/sidecar/TestNativeSidecarPlugin.java:199

The test was modified from expecting a failure to asserting a successful query. Please verify that this change is intended and that the expected query result is asserted appropriately in accompanying test validations.

presto-main-base/src/main/java/com/facebook/presto/sql/planner/iterative/rule/PickTableLayout.java:277

[nitpick] It would be helpful to add an inline comment explaining the purpose of assigning TRUE_CONSTANT to ineligiblePredicate and how it interacts with the subsequent filtering logic to separate ineligible expressions.

        RowExpression ineligiblePredicate = TRUE_CONSTANT;

aaneja · 2025-08-01T08:36:29Z

Why doesn't this if-block trigger during AddExchanges and pre-add the Exchange above the System table scan -

if (nativeExecution && containsSystemTableScan(plan)) {
                plan = gatheringExchange(idAllocator.getNextId(), REMOTE_STREAMING, plan);
            }

tdcmeehan · 2025-08-01T20:13:12Z

@aaneja good feedback, it should. I designed this to ignore the nativeExecution flag, as even with nativeExecution disabled this should kick in. But it made the scope of the changes larger, so in the interest of making this PR smaller I'm working on presuming that the above rule will always kick in.

tdcmeehan · 2025-08-04T20:19:12Z

@aaneja I removed RemoveRedundantExchangeRuleSet after making sure AddExchanges is solely responsible for inserting the exchange for system tables. This requires that nativeExecutionEnabled be enabled, which is a reasonable assumption. I also modified ExtractSystemTableFilterRuleSet to match the exchange inserted by AddExchanges. Instead of inserting an exchange, it will move the projection and filter around the exchange already inserted.

aaneja · 2025-08-05T11:42:44Z

@tdcmeehan With nativeExecutionEnabled enabled, and AddExchanges running before the ExtractSystemTableFilterRuleSet ruleset, all system tables should have an Exchange added, i.e -

Exchange -> TableScan (system)

So how does ExtractSystemTableFilterRuleSet help ? I'm confused

 * Patterns handled:
 * 1. Exchange -> Project -> Filter -> TableScan (system) => Project -> Filter -> Exchange -> TableScan
 * 2. Exchange -> Project -> TableScan (system) => Project -> Exchange -> TableScan
 * 3. Exchange -> Filter -> TableScan (system) => Filter -> Exchange -> TableScan
 */

Are there rules after AddExchanges that move the Exchange 'up' ?

aaneja · 2025-08-05T12:03:27Z

I'm wondering if it would instead make sense to set nativeExecution to true during AddExchanges#visitProject and AddExchanges#visitFilter if we encounter a RowExpression which satisfies containsNonCoordinatorEligibleCallExpression

tdcmeehan · 2025-08-05T16:05:46Z

@aaneja I initially went with this approach, but found that there were rules that further pushed down projections such as PredicatePushDown and ProjectionPushDown which pushed filters and projections past the exchange. It seemed simpler to create a rule which undoes whatever has been added specifically for system tables, rather than systematically ensure each rule respects the language of the functions when pushing down projections and filters.

aaneja

I'm fine with the current approach. Can you also add a plan checker, similar to (or inside of) CheckUnsupportedPrestissimoTypes to ensure that there are no nodes 'under' a coordinator-only exchange other than a TableScanNode when nativeExecution is turned on ?

In the long term, we should move the point we set the COORDINATOR_DISTRIBUTION on the Exchange partitioning handle to AddExchanges. This way we can reason about if a Project/Filter/Aggregate can be pushed through the Exchange more clearly

aaneja · 2025-08-06T04:10:12Z

+                        ruleStats,
+                        statsCalculator,
+                        costCalculator,
+                        new ExtractSystemTableFilterRuleSet(metadata.getFunctionAndTypeManager()).rules()));


Should we move this to L992 above the // DO NOT add optimizers that change the plan shape (computations) after this point comment

tdcmeehan · 2025-08-12T12:20:35Z

@aaneja I added the plan checker, and addressed your other feedback. Would you please take another look?

aaneja

LGTM % some nits

yingsu00

LGTM

tdcmeehan requested a review from a team as a code owner June 19, 2025 20:10

tdcmeehan requested a review from ZacBlanco June 19, 2025 20:10

prestodb-ci added the from:IBM PR from IBM label Jun 19, 2025

prestodb-ci requested review from a team, jp-sivaprasad and xin-zhang2 and removed request for a team June 19, 2025 20:10

tdcmeehan mentioned this pull request Jun 19, 2025

Always evaluate built-in functions using the presto.default namespace #25135

Merged

6 tasks

tdcmeehan force-pushed the planner branch from b5c0aaa to 238a28e Compare June 19, 2025 20:43

tdcmeehan requested review from jaystarshot and pdabre12 June 20, 2025 13:27

tdcmeehan force-pushed the planner branch from 238a28e to 1a77422 Compare June 20, 2025 13:36

tdcmeehan requested a review from a team as a code owner June 20, 2025 13:36

tdcmeehan requested review from Copilot and rschlussel June 20, 2025 19:52

Copilot AI reviewed Jun 24, 2025

View reviewed changes

tdcmeehan force-pushed the planner branch from 1a77422 to d5dcfd8 Compare July 31, 2025 23:39

aaneja self-requested a review August 1, 2025 08:36

tdcmeehan force-pushed the planner branch 2 times, most recently from ba5ab4f to 2a40f08 Compare August 4, 2025 20:15

aaneja requested changes Aug 6, 2025

View reviewed changes

tdcmeehan force-pushed the planner branch 3 times, most recently from 364b161 to 283327b Compare August 11, 2025 21:54

aaneja previously approved these changes Aug 13, 2025

View reviewed changes

Add planner support to prevent mixed CPP/Java execution

4705a25

tdcmeehan dismissed aaneja’s stale review via 4705a25 August 13, 2025 20:01

tdcmeehan force-pushed the planner branch from 283327b to 4705a25 Compare August 13, 2025 20:01

yingsu00 approved these changes Aug 14, 2025

View reviewed changes

tdcmeehan merged commit 5c36ee5 into prestodb:master Aug 14, 2025
68 of 69 checks passed

tdcmeehan mentioned this pull request Dec 1, 2025

Add planner support to prevent CPP functions from executing on coordinator fragments #25335

Closed

Conversation

tdcmeehan commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

aaneja commented Aug 1, 2025

Uh oh!

tdcmeehan commented Aug 1, 2025

Uh oh!

tdcmeehan commented Aug 4, 2025

Uh oh!

aaneja commented Aug 5, 2025

Uh oh!

aaneja commented Aug 5, 2025

Uh oh!

tdcmeehan commented Aug 5, 2025

Uh oh!

aaneja left a comment • 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!

aaneja Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tdcmeehan commented Aug 12, 2025

Uh oh!

aaneja left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yingsu00 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tdcmeehan commented Jun 19, 2025 •

edited

Loading

aaneja left a comment •

edited

Loading