You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2026/04/11 11:55:31 UTC
[PR] Add polymorphic arithmetic scalar functions [pinot]
xiangfu0 opened a new pull request, #18171:
URL: https://github.com/apache/pinot/pull/18171
## Summary
- move arithmetic scalar implementations for abs/mod/moduloOrZero/positiveModulo/negate/least/greatest into `org.apache.pinot.common.function.scalar.arithmetic`
- make these functions polymorphic across Pinot numeric types, including `BIG_DECIMAL`, while preserving legacy double fallback coercions
- register the moved functions for Calcite/MSE, handle `min`/`max` alias collisions safely, and add parser/planner/integration regression coverage
## Testing
- `./mvnw -pl pinot-common -Dtest=CalciteSqlCompilerTest,ArithmeticScalarFunctionTest test -Dsurefire.failIfNoSpecifiedTests=false`
- `./mvnw -pl pinot-query-planner -Dtest=QueryCompilationTest test -Dsurefire.failIfNoSpecifiedTests=false`
- `./mvnw -pl pinot-integration-tests -Dtest=ArithmeticFunctionsIntegrationTest -Dsurefire.failIfNoSpecifiedTests=false test`
- `./mvnw spotless:apply -pl pinot-common,pinot-core,pinot-query-runtime,pinot-query-planner,pinot-integration-tests -DskipTests`
- `./mvnw checkstyle:check -pl pinot-common,pinot-core,pinot-query-runtime,pinot-query-planner,pinot-integration-tests -DskipTests`
- `./mvnw license:format -pl pinot-common,pinot-core,pinot-query-runtime,pinot-query-planner,pinot-integration-tests -DskipTests`
- `./mvnw license:check -pl pinot-common,pinot-core,pinot-query-runtime,pinot-query-planner,pinot-integration-tests -DskipTests`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add polymorphic arithmetic scalar functions [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #18171:
URL: https://github.com/apache/pinot/pull/18171#discussion_r3068902734
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/arithmetic/PolymorphicBinaryArithmeticScalarFunction.java:
##########
@@ -25,7 +25,9 @@
/**
- * Base class for polymorphic binary arithmetic scalar functions
+ * Base class for polymorphic binary arithmetic scalar functions.
+ *
+ * <p>Instances are immutable and thread-safe.
*/
public abstract class PolymorphicBinaryArithmeticScalarFunction implements PinotScalarFunction {
Review Comment:
(minor) Rename to `BaseBinaryArithmeticScalarFunction`
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ArithmeticFunctions.java:
##########
@@ -74,53 +81,42 @@ public static int isNaN(double value) {
return Double.isNaN(value) ? 1 : 0;
}
- @ScalarFunction
public static double mod(double a, double b) {
Review Comment:
Are these methods used? Should we remove them?
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/arithmetic/GreatestScalarFunction.java:
##########
@@ -0,0 +1,97 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.function.scalar.arithmetic;
+
+import java.math.BigDecimal;
+import java.util.EnumMap;
+import java.util.Map;
+import org.apache.pinot.common.function.FunctionInfo;
+import org.apache.pinot.common.function.sql.PinotSqlFunction;
+import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
+import org.apache.pinot.spi.annotations.ScalarFunction;
+
+
+/**
+ * Polymorphic greatest/max scalar function implementation.
+ *
+ * <p>Instances are immutable and thread-safe.
+ */
+@ScalarFunction(names = {"greatest", "max"})
Review Comment:
`max` is an aggregate function. Can we share the same function name?
Same for other places
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add polymorphic arithmetic scalar functions [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18171:
URL: https://github.com/apache/pinot/pull/18171#discussion_r3068986780
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/arithmetic/PolymorphicBinaryArithmeticScalarFunction.java:
##########
@@ -25,7 +25,9 @@
/**
- * Base class for polymorphic binary arithmetic scalar functions
+ * Base class for polymorphic binary arithmetic scalar functions.
+ *
+ * <p>Instances are immutable and thread-safe.
*/
public abstract class PolymorphicBinaryArithmeticScalarFunction implements PinotScalarFunction {
Review Comment:
Renamed the shared base to `BaseBinaryArithmeticScalarFunction` in `ccfebfbb15`, and renamed the unary sibling to `BaseUnaryArithmeticScalarFunction` for consistency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add polymorphic arithmetic scalar functions [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18171:
URL: https://github.com/apache/pinot/pull/18171#issuecomment-4229439161
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18171?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `64.59144%` with `91 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.36%. Comparing base ([`34d3fd6`](https://app.codecov.io/gh/apache/pinot/commit/34d3fd6f5bb23d161f65de638391c4875f684c23?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`94fb8f3`](https://app.codecov.io/gh/apache/pinot/commit/94fb8f33a604f7b2d55867bb4d57c21d824485bf?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18171?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...calar/arithmetic/PositiveModuloScalarFunction.java](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Ffunction%2Fscalar%2Farithmetic%2FPositiveModuloScalarFunction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL2FyaXRobWV0aWMvUG9zaXRpdmVNb2R1bG9TY2FsYXJGdW5jdGlvbi5qYXZh) | 50.00% | [18 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ion/scalar/arithmetic/ArithmeticFunctionUtils.java](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Ffunction%2Fscalar%2Farithmetic%2FArithmeticFunctionUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL2FyaXRobWV0aWMvQXJpdGhtZXRpY0Z1bmN0aW9uVXRpbHMuamF2YQ==) | 58.33% | [11 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../function/scalar/arithmetic/AbsScalarFunction.java](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Ffunction%2Fscalar%2Farithmetic%2FAbsScalarFunction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL2FyaXRobWV0aWMvQWJzU2NhbGFyRnVuY3Rpb24uamF2YQ==) | 70.37% | [6 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ot/common/function/scalar/ArithmeticFunctions.java](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Ffunction%2Fscalar%2FArithmeticFunctions.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL0FyaXRobWV0aWNGdW5jdGlvbnMuamF2YQ==) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...tion/scalar/arithmetic/GreatestScalarFunction.java](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Ffunction%2Fscalar%2Farithmetic%2FGreatestScalarFunction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL2FyaXRobWV0aWMvR3JlYXRlc3RTY2FsYXJGdW5jdGlvbi5qYXZh) | 72.00% | [6 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...unction/scalar/arithmetic/LeastScalarFunction.java](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Ffunction%2Fscalar%2Farithmetic%2FLeastScalarFunction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL2FyaXRobWV0aWMvTGVhc3RTY2FsYXJGdW5jdGlvbi5qYXZh) | 72.00% | [6 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../scalar/arithmetic/ModuloOrZeroScalarFunction.java](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Ffunction%2Fscalar%2Farithmetic%2FModuloOrZeroScalarFunction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL2FyaXRobWV0aWMvTW9kdWxvT3JaZXJvU2NhbGFyRnVuY3Rpb24uamF2YQ==) | 72.00% | [6 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../function/scalar/arithmetic/ModScalarFunction.java](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Ffunction%2Fscalar%2Farithmetic%2FModScalarFunction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL2FyaXRobWV0aWMvTW9kU2NhbGFyRnVuY3Rpb24uamF2YQ==) | 75.00% | [5 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...nction/scalar/arithmetic/NegateScalarFunction.java](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Ffunction%2Fscalar%2Farithmetic%2FNegateScalarFunction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL2FyaXRobWV0aWMvTmVnYXRlU2NhbGFyRnVuY3Rpb24uamF2YQ==) | 75.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...etic/PolymorphicUnaryArithmeticScalarFunction.java](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Ffunction%2Fscalar%2Farithmetic%2FPolymorphicUnaryArithmeticScalarFunction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL2FyaXRobWV0aWMvUG9seW1vcnBoaWNVbmFyeUFyaXRobWV0aWNTY2FsYXJGdW5jdGlvbi5qYXZh) | 55.55% | [2 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| ... and [3 more](https://app.codecov.io/gh/apache/pinot/pull/18171?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
> :exclamation: There is a different number of reports uploaded between BASE (34d3fd6) and HEAD (94fb8f3). Click for more details.
>
> <details><summary>HEAD has 20 uploads less than BASE</summary>
>
>| Flag | BASE (34d3fd6) | HEAD (94fb8f3) |
>|------|------|------|
>|java-21|3|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|6|1|
>|java-11|3|0|
>|unittests2|2|0|
>|integration|2|0|
>|integration2|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18171 +/- ##
============================================
- Coverage 63.13% 55.36% -7.78%
+ Complexity 1610 826 -784
============================================
Files 3213 2527 -686
Lines 195730 144943 -50787
Branches 30240 23269 -6971
============================================
- Hits 123583 80253 -43330
+ Misses 62281 57790 -4491
+ Partials 9866 6900 -2966
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18171/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/18171/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/18171/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/18171/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18171/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.36% <64.59%> (-7.74%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18171/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.36% <64.59%> (-7.78%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18171/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.36% <64.59%> (-7.78%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18171/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.36% <64.59%> (-0.01%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18171/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/18171?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add polymorphic arithmetic scalar functions [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #18171:
URL: https://github.com/apache/pinot/pull/18171#issuecomment-4233907842
A documentation PR has been opened for this change: https://github.com/pinot-contrib/pinot-docs/pull/733
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add polymorphic arithmetic scalar functions [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #18171:
URL: https://github.com/apache/pinot/pull/18171
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add polymorphic arithmetic scalar functions [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18171:
URL: https://github.com/apache/pinot/pull/18171#discussion_r3069280725
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/arithmetic/BaseUnaryArithmeticScalarFunction.java:
##########
@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.function.scalar.arithmetic;
+
+import javax.annotation.Nullable;
+import org.apache.pinot.common.function.FunctionInfo;
+import org.apache.pinot.common.function.PinotScalarFunction;
+import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
+
+
+/**
+ * Base class for unary arithmetic scalar functions.
+ *
+ * <p>Instances are immutable and thread-safe.
+ */
+public abstract class BaseUnaryArithmeticScalarFunction implements PinotScalarFunction {
+
+ @Nullable
+ @Override
+ public FunctionInfo getFunctionInfo(ColumnDataType[] argumentTypes) {
+ if (argumentTypes.length != 1) {
+ return null;
+ }
+
+ FunctionInfo functionInfo = functionInfoForType(argumentTypes[0].getStoredType());
Review Comment:
`getFunctionInfo(ColumnDataType[])` dispatches on `argumentTypes[0].getStoredType()`, which will treat non-numeric logical types like BOOLEAN (stored as INT) and TIMESTAMP (stored as LONG) as numeric and select INT/LONG overloads. This can produce return types/behavior that diverge from Calcite validation (which treats BOOLEAN/TIMESTAMP as non-numeric in `ArithmeticFunctionUtils` and infers DOUBLE) and from the stated goal of polymorphism only across Pinot numeric types. Consider dispatching on the logical `ColumnDataType` (and only using stored type for legacy coercions/fallback) so non-numeric types consistently hit the DOUBLE fallback.
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/arithmetic/PositiveModuloScalarFunction.java:
##########
@@ -0,0 +1,114 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.function.scalar.arithmetic;
+
+import java.math.BigDecimal;
+import java.util.EnumMap;
+import java.util.Map;
+import org.apache.pinot.common.function.FunctionInfo;
+import org.apache.pinot.common.function.sql.PinotSqlFunction;
+import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
+import org.apache.pinot.spi.annotations.ScalarFunction;
+
+
+/**
+ * Polymorphic positive-modulo scalar function implementation.
+ *
+ * <p>Instances are immutable and thread-safe.
+ */
+@ScalarFunction
+public class PositiveModuloScalarFunction extends BaseBinaryArithmeticScalarFunction {
+
+ private static final Map<ColumnDataType, FunctionInfo> TYPE_FUNCTION_INFO_MAP = new EnumMap<>(ColumnDataType.class);
+
+ static {
+ try {
+ TYPE_FUNCTION_INFO_MAP.put(ColumnDataType.INT, new FunctionInfo(
+ PositiveModuloScalarFunction.class.getMethod("intPositiveModulo", int.class, int.class),
+ PositiveModuloScalarFunction.class, false));
+ TYPE_FUNCTION_INFO_MAP.put(ColumnDataType.LONG, new FunctionInfo(
+ PositiveModuloScalarFunction.class.getMethod("longPositiveModulo", long.class, long.class),
+ PositiveModuloScalarFunction.class, false));
+ TYPE_FUNCTION_INFO_MAP.put(ColumnDataType.FLOAT, new FunctionInfo(
+ PositiveModuloScalarFunction.class.getMethod("floatPositiveModulo", float.class, float.class),
+ PositiveModuloScalarFunction.class, false));
+ TYPE_FUNCTION_INFO_MAP.put(ColumnDataType.DOUBLE, new FunctionInfo(
+ PositiveModuloScalarFunction.class.getMethod("doublePositiveModulo", double.class, double.class),
+ PositiveModuloScalarFunction.class, false));
+ TYPE_FUNCTION_INFO_MAP.put(ColumnDataType.BIG_DECIMAL, new FunctionInfo(
+ PositiveModuloScalarFunction.class.getMethod("bigDecimalPositiveModulo", BigDecimal.class, BigDecimal.class),
+ PositiveModuloScalarFunction.class, false));
+ } catch (NoSuchMethodException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @Override
+ protected FunctionInfo functionInfoForType(ColumnDataType argumentType) {
+ FunctionInfo functionInfo = TYPE_FUNCTION_INFO_MAP.get(argumentType);
+ return functionInfo != null ? functionInfo : defaultFunctionInfo();
+ }
+
+ @Override
+ public String getName() {
+ return "positiveModulo";
+ }
+
+ @Override
+ public PinotSqlFunction toPinotSqlFunction() {
+ return ArithmeticFunctionUtils.binaryArithmeticSqlFunction(getName());
+ }
+
+ public static int intPositiveModulo(int a, int b) {
+ int result = a % b;
+ if (result >= 0) {
+ return result;
+ }
+ if (b == Integer.MIN_VALUE) {
+ return Integer.MAX_VALUE + result + 1;
+ }
+ return result + Math.abs(b);
+ }
Review Comment:
`intPositiveModulo()` / `longPositiveModulo()` (and the BIG_DECIMAL overload via `remainder`) will throw when the divisor is 0. Historically `positiveModulo` was evaluated via DOUBLE arithmetic, so `positiveModulo(x, 0)` produced `NaN` instead of failing the query. If this function can be used with data that includes zero divisors, consider adding explicit handling for a zero divisor (or documenting/testing the new failure behavior) to avoid unexpected query failures.
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/arithmetic/ModScalarFunction.java:
##########
@@ -0,0 +1,91 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.function.scalar.arithmetic;
+
+import java.math.BigDecimal;
+import java.util.EnumMap;
+import java.util.Map;
+import org.apache.pinot.common.function.FunctionInfo;
+import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
+import org.apache.pinot.spi.annotations.ScalarFunction;
+
+
+/**
+ * Polymorphic modulo scalar function implementation.
+ *
+ * <p>Instances are immutable and thread-safe.
+ */
+@ScalarFunction
+public class ModScalarFunction extends BaseBinaryArithmeticScalarFunction {
+
+ private static final Map<ColumnDataType, FunctionInfo> TYPE_FUNCTION_INFO_MAP = new EnumMap<>(ColumnDataType.class);
+
+ static {
+ try {
+ TYPE_FUNCTION_INFO_MAP.put(ColumnDataType.INT,
+ new FunctionInfo(ModScalarFunction.class.getMethod("intMod", int.class, int.class), ModScalarFunction.class,
+ false));
+ TYPE_FUNCTION_INFO_MAP.put(ColumnDataType.LONG,
+ new FunctionInfo(ModScalarFunction.class.getMethod("longMod", long.class, long.class),
+ ModScalarFunction.class, false));
+ TYPE_FUNCTION_INFO_MAP.put(ColumnDataType.FLOAT,
+ new FunctionInfo(ModScalarFunction.class.getMethod("floatMod", float.class, float.class),
+ ModScalarFunction.class, false));
+ TYPE_FUNCTION_INFO_MAP.put(ColumnDataType.DOUBLE,
+ new FunctionInfo(ModScalarFunction.class.getMethod("doubleMod", double.class, double.class),
+ ModScalarFunction.class, false));
+ TYPE_FUNCTION_INFO_MAP.put(ColumnDataType.BIG_DECIMAL,
+ new FunctionInfo(ModScalarFunction.class.getMethod("bigDecimalMod", BigDecimal.class, BigDecimal.class),
+ ModScalarFunction.class, false));
+ } catch (NoSuchMethodException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @Override
+ protected FunctionInfo functionInfoForType(ColumnDataType argumentType) {
+ FunctionInfo functionInfo = TYPE_FUNCTION_INFO_MAP.get(argumentType);
+ return functionInfo != null ? functionInfo : defaultFunctionInfo();
+ }
+
+ @Override
+ public String getName() {
+ return "mod";
+ }
+
+ public static int intMod(int a, int b) {
+ return a % b;
+ }
Review Comment:
The INT/LONG overloads use Java `%`, which throws `ArithmeticException` when the divisor is 0 (and the BIG_DECIMAL overload will throw on `remainder(0)`). Previously, `mod` was implemented as a DOUBLE operation (`a % b`) so `mod(x, 0)` evaluated to `NaN` instead of failing the query. If query-time divisors can be 0, this is a behavior regression that can turn previously non-fatal expressions into query failures; consider explicitly handling a zero divisor (or otherwise preserving the legacy behavior) for non-floating types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add polymorphic arithmetic scalar functions [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18171:
URL: https://github.com/apache/pinot/pull/18171#discussion_r3068986910
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ArithmeticFunctions.java:
##########
@@ -74,53 +81,42 @@ public static int isNaN(double value) {
return Double.isNaN(value) ? 1 : 0;
}
- @ScalarFunction
public static double mod(double a, double b) {
Review Comment:
Checked the call sites and these wrappers were dead after the move. I removed them in `ccfebfbb15` so the registry resolves the arithmetic package implementations directly.
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/arithmetic/GreatestScalarFunction.java:
##########
@@ -0,0 +1,97 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.function.scalar.arithmetic;
+
+import java.math.BigDecimal;
+import java.util.EnumMap;
+import java.util.Map;
+import org.apache.pinot.common.function.FunctionInfo;
+import org.apache.pinot.common.function.sql.PinotSqlFunction;
+import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
+import org.apache.pinot.spi.annotations.ScalarFunction;
+
+
+/**
+ * Polymorphic greatest/max scalar function implementation.
+ *
+ * <p>Instances are immutable and thread-safe.
+ */
+@ScalarFunction(names = {"greatest", "max"})
Review Comment:
Dropped the scalar `max`/`min` aliases in `ccfebfbb15`. Only `greatest`/`least` remain registered as scalar functions now, so they no longer overlap with aggregate `MAX`/`MIN`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org