Skip to content

Conversation

@yingsu00
Copy link
Contributor

Description

Motivation and Context

Impact

Test Plan

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.

== RELEASE NOTES ==

General Changes
* ... 
* ... 

Hive Connector Changes
* ... 
* ... 

If release note is NOT required, use:

== NO RELEASE NOTE ==

@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Nov 19, 2025
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Nov 19, 2025

Reviewer's Guide

This PR introduces a new native execution session property to push down integer upcasts to the scan operator, wiring it through both the Java and C++ layers, updating connector interfaces and tests, and also tweaks build configuration defaults for spatial support.

ER diagram for session property metadata changes

erDiagram
    SESSION_PROPERTY {
        string name
        string description
        string type
        bool defaultValue
        string configKey
        string value
    }
    SESSION_PROPERTY ||--o| NATIVE_PUSHDOWN_INTEGER_UPCASTS_TO_SCAN : contains
Loading

Class diagram for new and updated session property handling

classDiagram
    class NativeWorkerSessionPropertyProvider {
        +NATIVE_PUSHDOWN_INTEGER_UPCASTS_TO_SCAN : String
        ...
    }
    class SessionProperties {
        +kPushdownIntegerUpcastsToScan : const char*
        +addSessionProperty(...)
        ...
    }
    NativeWorkerSessionPropertyProvider --> SessionProperties : uses
Loading

File-Level Changes

Change Details Files
Add new native session property in Java layer
  • Define NATIVE_PUSHDOWN_INTEGER_UPCASTS_TO_SCAN constant
  • Register booleanProperty in NativeWorkerSessionPropertyProvider
presto-main-base/src/main/java/com/facebook/presto/sessionpropertyproviders/NativeWorkerSessionPropertyProvider.java
Register session property in native C++ config
  • Add kPushdownIntegerUpcastsToScan constant in SessionProperties.h
  • Invoke addSessionProperty for the new flag in SessionProperties.cpp
presto-native-execution/presto_cpp/main/SessionProperties.h
presto-native-execution/presto_cpp/main/SessionProperties.cpp
Extend connector API to accept pushdown flag
  • Add pushdownCasts parameter to SystemConnector::read override
presto-native-execution/presto_cpp/main/connectors/SystemConnector.h
Update unit tests for new session property
  • Add expected mapping in SessionPropertiesTest
  • Verify pushdownIntegerUpcastsToScan in QueryContextManagerTest
presto-native-execution/presto_cpp/main/tests/SessionPropertiesTest.cpp
presto-native-execution/presto_cpp/main/tests/QueryContextManagerTest.cpp
Adjust build configuration defaults for spatial support
  • Disable spatial support by default in CMakeLists.txt
  • Add status messages for CMake flags
  • Comment out git submodule commands in Makefile
presto-native-execution/CMakeLists.txt
presto-native-execution/Makefile
Clarify comment in PartitionAndSerialize operator
  • Update comment to reference outputWithoutUpcasts
presto-native-execution/presto_cpp/main/operators/PartitionAndSerialize.cpp

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

option(PRESTO_ENABLE_ARROW_FLIGHT_CONNECTOR "Enable Arrow Flight connector" OFF)

option(PRESTO_ENABLE_SPATIAL "Enable spatial support" ON)
option(PRESTO_ENABLE_SPATIAL "Enable spatial support" OFF)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should be reverted.

set(CMAKE_CXX_STANDARD_REQUIRED True)
message("Appending CMAKE_CXX_FLAGS with ${SCRIPT_CXX_FLAGS}")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${SCRIPT_CXX_FLAGS}")
#set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address -g -O0 ")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove these debug messages.

cmake_policy(SET CMP0104 NEW)
endif()

set(PRESTO_ENABLE_SPATIAL OFF)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. This is switching off SPATIAL needed for Bolt.

CACHE BOOL
"Enable Velox Geometry (aka spatial) support"
)
message(STATUS "PRESTO_ENABLE_SPATIAL: ${PRESTO_ENABLE_SPATIAL}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove these debug messages.

velox-submodule: #: Check out code for velox submodule
git submodule sync --recursive
git submodule update --init --recursive
# git submodule sync --recursive
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove these changes.

@yingsu00 yingsu00 force-pushed the pushdownCastToSourceLocal branch 6 times, most recently from ed39537 to 9287947 Compare November 28, 2025 06:48
@yingsu00 yingsu00 force-pushed the pushdownCastToSourceLocal branch from 9287947 to e650316 Compare December 2, 2025 08:05
This commit adds a new native session property native_pushdown_integer_
upcasts_to_scan, which when set to true, would rewrite the local plan
and push down integer upcasts to the source operaters.
@yingsu00 yingsu00 force-pushed the pushdownCastToSourceLocal branch from e650316 to fb32a04 Compare December 2, 2025 08:17
@steveburnett
Copy link
Contributor

Please include documentation for the new session property, likely in https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/presto_cpp/properties-session.rst.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

from:IBM PR from IBM

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants