roundDecimal() fails for null FLOAT values #15377

mghildiy · 2025-03-26T14:33:40Z

This change includes:

For null value for FLOAT column, use NaN as transformation value

bugfix.

mghildiy · 2025-03-27T13:30:23Z

I woud work on unit test once code approach is approved.

Jackie-Jiang · 2025-03-27T18:34:58Z

...in/java/org/apache/pinot/core/operator/transform/function/RoundDecimalTransformFunction.java

-        _doubleValuesSV[i] = BigDecimal.valueOf(leftValues[i])
-            .setScale(_scale, RoundingMode.HALF_UP).doubleValue();
+        if (Double.NEGATIVE_INFINITY == leftValues[i]) {
+          _doubleValuesSV[i] = Double.NaN;


Can you check the standard SQL behavior when rounding on -Inf? Does it throw exception, return -Inf or NaN?
We should put the same check in other if branches

I would take a look.

It is DB specific behaviour. Some throw errors, some result to -infinity.

How about PostgreSQL? We usually use it as a reference

PostgreSQL returns -Infinity.

Let's try to match that behavior. We want to handle all special values, including Inf, -Inf, NaN

But I undrstand that currently pinot stores null values as -Inf only. Do we need to handle other two?

Which layer eventually writes to response to a SQL query?

Pinot does use -Inf as the placeholder for null value, but Inf is also valid value and we should be able to handle it without throwing exception. NaN is debatable because they are converted into null within SpecialValueTransformer

codecov-commenter · 2025-03-28T07:17:54Z

Codecov Report

❌ Patch coverage is 33.33333% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.31%. Comparing base (59551e4) to head (9b41e37).
⚠️ Report is 2807 commits behind head on master.

Files with missing lines	Patch %	Lines
...nsform/function/RoundDecimalTransformFunction.java	33.33%	9 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #15377      +/-   ##
============================================
+ Coverage     61.75%   63.31%   +1.56%     
- Complexity      207     1379    +1172     
============================================
  Files          2436     3035     +599     
  Lines        133233   176732   +43499     
  Branches      20636    27121    +6485     
============================================
+ Hits          82274   111898   +29624     
- Misses        44911    56255   +11344     
- Partials       6048     8579    +2531

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (+99.99%)`	⬆️
integration	`100.00% <ø> (+99.99%)`	⬆️
integration1	`100.00% <ø> (+99.99%)`	⬆️
integration2	`0.00% <ø> (ø)`
java-11	`63.28% <33.33%> (+1.57%)`	⬆️
java-21	`63.27% <33.33%> (+1.65%)`	⬆️
skip-bytebuffers-false	`?`
skip-bytebuffers-true	`?`
temurin	`63.31% <33.33%> (+1.56%)`	⬆️
unittests	`63.31% <33.33%> (+1.56%)`	⬆️
unittests1	`56.45% <33.33%> (+9.56%)`	⬆️
unittests2	`33.11% <0.00%> (+5.38%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Jackie-Jiang · 2025-04-04T23:20:34Z

...in/java/org/apache/pinot/core/operator/transform/function/RoundDecimalTransformFunction.java

@@ -90,18 +90,30 @@ public double[] transformToDoubleValuesSV(ValueBlock valueBlock) {
    double[] leftValues = _leftTransformFunction.transformToDoubleValuesSV(valueBlock);
    if (_fixedScale) {
      for (int i = 0; i < length; i++) {
-        _doubleValuesSV[i] = BigDecimal.valueOf(leftValues[i])
-            .setScale(_scale, RoundingMode.HALF_UP).doubleValue();
+        if (Double.NEGATIVE_INFINITY == leftValues[i]) {


(nit) We usually put constant on the right hand side

Jackie-Jiang · 2025-04-04T23:21:51Z

...in/java/org/apache/pinot/core/operator/transform/function/RoundDecimalTransformFunction.java

-        _doubleValuesSV[i] = BigDecimal.valueOf(leftValues[i])
-            .setScale(_scale, RoundingMode.HALF_UP).doubleValue();
+        if (Double.NEGATIVE_INFINITY == leftValues[i]) {
+          _doubleValuesSV[i] = Double.NaN;


Let's try to match that behavior. We want to handle all special values, including Inf, -Inf, NaN

mghildiy · 2025-04-16T13:20:41Z

Please review the commit. I would add unit tests accordingly.

Jackie-Jiang · 2025-04-16T21:23:10Z

pinot-core/src/main/java/org/apache/pinot/core/operator/query/SelectionOnlyOperator.java

+              if (values[colId] instanceof Double) {
+                values[colId] = Double.NEGATIVE_INFINITY;
+              } else {
+                values[colId] = null;
+              }


I don't think this is correct. Do we need to change this?

I think we want this behaviour for decimals only. For other flows, behaviour must be as it is currently.

When null is enabled, we should be able to handle null value properly. Do you observe issues if reverting this?

You are right. This change is not needed.

Jackie-Jiang · 2025-04-16T21:26:16Z

...in/java/org/apache/pinot/core/operator/transform/function/RoundDecimalTransformFunction.java

-        _doubleValuesSV[i] = BigDecimal.valueOf(leftValues[i])
-            .setScale(_scale, RoundingMode.HALF_UP).doubleValue();
+        if (leftValues[i] == Double.NEGATIVE_INFINITY || leftValues[i] == Double.POSITIVE_INFINITY ||
+                leftValues[i] == Double.NaN) {


NaN is not comparable. We can probably use a try-catch over result computation, and set the result to be left value if exception is caught

Or we can check with Double.isNaN. It's documentation says:
'This method corresponds to the isNaN operation defined in IEEE 754.'

So if its NaN, what value we return? From postgres documentation:

In addition to ordinary numeric values, the numeric type allows the special value NaN, meaning "not-a-number". Any operation on NaN yields another NaN.

Sounds good. Let's follow the same behavior then. You may use a try-catch to avoid the overhead of value check for happy path

Currently Double.NaN, Double.NEGATIVE_INFINITY, Double.POSITIVE_INFINITY are mapped to null(in null bit map). I am debugging code to ensure that these 3 values are not part of null bit map.

1. For round function for decimal values, if value is infinity or NaN, assign as such with no rouding operation 2. If null bit map for column contains docid and it's a Double, set value to negative infinity

Jackie-Jiang · 2025-05-05T22:20:55Z

...in/java/org/apache/pinot/core/operator/transform/function/RoundDecimalTransformFunction.java

-            .setScale(_scale, RoundingMode.HALF_UP).doubleValue();
+        try {
+          _doubleValuesSV[i] = BigDecimal.valueOf(leftValues[i])
+                  .setScale(_scale, RoundingMode.HALF_UP).doubleValue();


Please apply Pinot Style and reformat the changes

Jackie-Jiang · 2025-05-05T22:23:59Z

...in/java/org/apache/pinot/core/operator/transform/function/RoundDecimalTransformFunction.java

      }
    } else {
      for (int i = 0; i < length; i++) {
-        _doubleValuesSV[i] = (double) Math.round(leftValues[i]);
+        if (leftValues[i] == Double.NEGATIVE_INFINITY || leftValues[i] == Double.POSITIVE_INFINITY ||
+                leftValues[i] == Double.NaN) {


Suggested change

leftValues[i] == Double.NaN) {

Double.isNaN(leftValues[i])) {

mghildiy mentioned this pull request Mar 26, 2025

roundDecimal() fails for null FLOAT values #15255

Closed

mghildiy changed the title ~~This commit includes:~~ roundDecimal() fails for null FLOAT values Mar 27, 2025

Jackie-Jiang reviewed Mar 27, 2025

View reviewed changes

mghildiy force-pushed the fix15255 branch from 2d04ad0 to c41e472 Compare March 30, 2025 10:43

Jackie-Jiang added the bugfix label Apr 4, 2025

Jackie-Jiang reviewed Apr 4, 2025

View reviewed changes

mghildiy force-pushed the fix15255 branch from c41e472 to 3edd1cf Compare April 13, 2025 12:56

Jackie-Jiang reviewed Apr 16, 2025

View reviewed changes

This commit:

6964adf

1. For round function for decimal values, if value is infinity or NaN, assign as such with no rouding operation 2. If null bit map for column contains docid and it's a Double, set value to negative infinity

mghildiy force-pushed the fix15255 branch from 3edd1cf to 6964adf Compare May 3, 2025 09:12

Jackie-Jiang reviewed May 5, 2025

View reviewed changes

Address comments

9b41e37

Jackie-Jiang approved these changes Aug 22, 2025

View reviewed changes

Jackie-Jiang merged commit d3c322d into apache:master Aug 22, 2025
30 of 36 checks passed

	leftValues[i] == Double.NaN) {
	Double.isNaN(leftValues[i])) {

roundDecimal() fails for null FLOAT values #15377

roundDecimal() fails for null FLOAT values #15377

Uh oh!

Conversation

mghildiy commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mghildiy commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

mghildiy Apr 5, 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!

codecov-commenter commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mghildiy commented Apr 16, 2025

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!

mghildiy Apr 20, 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!

mghildiy May 3, 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!

Uh oh!

Uh oh!

mghildiy commented Mar 26, 2025 •

edited

Loading

mghildiy commented Mar 27, 2025 •

edited

Loading

mghildiy Apr 5, 2025 •

edited

Loading

codecov-commenter commented Mar 28, 2025 •

edited

Loading

mghildiy Apr 20, 2025 •

edited

Loading

mghildiy May 3, 2025 •

edited

Loading