feat: Add pmod (positive modulo) scalar function#27543
Open
natashasehgal wants to merge 1 commit intoprestodb:masterfrom
Open
feat: Add pmod (positive modulo) scalar function#27543natashasehgal wants to merge 1 commit intoprestodb:masterfrom
natashasehgal wants to merge 1 commit intoprestodb:masterfrom
Conversation
Summary: Add `pmod` function to Presto's MathFunctions, matching Velox's implementation (D99229614). `pmod(a, n)` returns the positive remainder of `a` divided by `n`, ensuring non-negative results for positive divisors. Overloads for TINYINT, SMALLINT, INTEGER, BIGINT, DOUBLE, and REAL. Differential Revision: D99449450
Contributor
Reviewer's GuideAdds a new scalar SQL function Sequence diagram for query execution using pmod delegated to native enginesequenceDiagram
actor User
participant PrestoClient
participant PrestoCoordinator
participant FunctionRegistry
participant NativeEngine
User->>PrestoClient: Submit SQL query
PrestoClient->>PrestoCoordinator: Send query text
PrestoCoordinator->>FunctionRegistry: Resolve function pmod
FunctionRegistry-->>PrestoCoordinator: Return MathFunctions.pmod* overload metadata
PrestoCoordinator->>NativeEngine: Plan and execute pmod with resolved types
NativeEngine-->>PrestoCoordinator: Return pmod results
PrestoCoordinator-->>PrestoClient: Return query results
PrestoClient-->>User: Display results
Class diagram for new pmod scalar overloads in MathFunctionsclassDiagram
class MathFunctions {
<<utility>>
+long pmodTinyint(long num1, long num2)
+long pmodSmallint(long num1, long num2)
+long pmodInteger(long num1, long num2)
+long pmodBigint(long num1, long num2)
+double pmodDouble(double num1, double num2)
+long pmodFloat(long num1, long num2)
}
class ScalarFunctionAnnotation {
}
class DescriptionAnnotation {
}
MathFunctions .. ScalarFunctionAnnotation : uses
MathFunctions .. DescriptionAnnotation : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since these Java stubs are registered as scalar functions but always throw, consider either providing a simple Java implementation or gating registration so non-native engine paths don’t unexpectedly hit
UnsupportedOperationExceptionat runtime. - If this pattern of delegating to the native engine is intentional, align the error message and annotations with other native-only scalar functions in
MathFunctions(or a shared helper) for consistency and easier debugging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since these Java stubs are registered as scalar functions but always throw, consider either providing a simple Java implementation or gating registration so non-native engine paths don’t unexpectedly hit `UnsupportedOperationException` at runtime.
- If this pattern of delegating to the native engine is intentional, align the error message and annotations with other native-only scalar functions in `MathFunctions` (or a shared helper) for consistency and easier debugging.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary: Add pmod (positive modulo) as a native Presto scalar function, registered across all numeric types: TINYINT, SMALLINT, INTEGER, BIGINT, DOUBLE, and REAL.
Motivation
pmod is already implemented in the Velox native engine but has no corresponding function registration in the Java coordinator. Without these stubs, the coordinator cannot resolve pmod in query plans, so queries using
it fail during analysis.
Design
Test Plan
== RELEASE NOTES ==
General Changes
query planning.
Differential Revision: D99449450
Summary by Sourcery
New Features: