You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2025/01/31 02:21:00 UTC
[PR] Canonicalize BigDecimal values during ingestion [pinot]
Jackie-Jiang opened a new pull request, #14958:
URL: https://github.com/apache/pinot/pull/14958
For the same value, `BigDecimal` could have multiple format with different trailing zeros. It could cause similar problems as Negative zero and NaN, where `compareTo() == 0` doesn't mean `equals()`. This could break sorting and binary search.
This PR solves the problem by stripping trailing zeros from `BigDecimal` values during ingestion
--
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] Canonicalize BigDecimal values during ingestion [pinot]
Posted by "jadami10 (via GitHub)" <gi...@apache.org>.
jadami10 commented on code in PR #14958:
URL: https://github.com/apache/pinot/pull/14958#discussion_r1937754676
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/SpecialValueTransformer.java:
##########
@@ -18,112 +18,119 @@
*/
package org.apache.pinot.segment.local.recordtransformer;
-import com.google.common.annotations.VisibleForTesting;
+import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
+import java.util.Set;
+import javax.annotation.Nullable;
import org.apache.pinot.spi.data.FieldSpec;
-import org.apache.pinot.spi.data.FieldSpec.DataType;
import org.apache.pinot.spi.data.Schema;
import org.apache.pinot.spi.data.readers.GenericRow;
import org.apache.pinot.spi.recordtransformer.RecordTransformer;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
* The {@code SpecialValueTransformer} class will transform special values according to the following rules:
* <ul>
- * <li>Negative zero (-0.0) should be converted to 0.0</li>
- * <li>NaN should be converted to default null</li>
+ * <li>
+ * For FLOAT and DOUBLE:
+ * <ul>
+ * <li>Negative zero (-0.0) should be converted to 0.0</li>
+ * <li>NaN should be converted to default null</li>
+ * </ul>
+ * </li>
+ * <li>
+ * For BIG_DECIMAL:
+ * <ul>
+ * <li>Remove trailing zeros</li>
Review Comment:
nit: if we're going to leave a comment, we might as well say why so folks don't have to come back to the PR
--
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] Canonicalize BigDecimal values during ingestion [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #14958:
URL: https://github.com/apache/pinot/pull/14958#discussion_r1936812149
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/SpecialValueTransformer.java:
##########
@@ -18,112 +18,119 @@
*/
package org.apache.pinot.segment.local.recordtransformer;
-import com.google.common.annotations.VisibleForTesting;
+import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
+import java.util.Set;
+import javax.annotation.Nullable;
import org.apache.pinot.spi.data.FieldSpec;
-import org.apache.pinot.spi.data.FieldSpec.DataType;
import org.apache.pinot.spi.data.Schema;
import org.apache.pinot.spi.data.readers.GenericRow;
import org.apache.pinot.spi.recordtransformer.RecordTransformer;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
* The {@code SpecialValueTransformer} class will transform special values according to the following rules:
* <ul>
- * <li>Negative zero (-0.0) should be converted to 0.0</li>
- * <li>NaN should be converted to default null</li>
+ * <li>
+ * For FLOAT and DOUBLE:
+ * <ul>
+ * <li>Negative zero (-0.0) should be converted to 0.0</li>
+ * <li>NaN should be converted to default null</li>
+ * </ul>
+ * </li>
+ * <li>
+ * For BIG_DECIMAL:
+ * <ul>
+ * <li>Remove trailing zeros</li>
+ * </ul>
+ * </li>
* </ul>
* <p>NOTE: should put this after the {@link DataTypeTransformer} so that we already have the values complying
* with the schema before handling special values and before {@link NullValueTransformer} so that it transforms
* all the null values properly.
*/
public class SpecialValueTransformer implements RecordTransformer {
+ private final static int NEGATIVE_ZERO_FLOAT_BITS = Float.floatToRawIntBits(-0.0f);
+ private final static long NEGATIVE_ZERO_DOUBLE_BITS = Double.doubleToLongBits(-0.0d);
- private static final Logger LOGGER = LoggerFactory.getLogger(SpecialValueTransformer.class);
- private final HashSet<String> _specialValuesKeySet = new HashSet<>();
- private int _negativeZeroConversionCount = 0;
- private int _nanConversionCount = 0;
+ private final Set<String> _columnsToCheck = new HashSet<>();
public SpecialValueTransformer(Schema schema) {
for (FieldSpec fieldSpec : schema.getAllFieldSpecs()) {
- if (!fieldSpec.isVirtualColumn() && (fieldSpec.getDataType() == DataType.FLOAT
- || fieldSpec.getDataType() == DataType.DOUBLE)) {
- _specialValuesKeySet.add(fieldSpec.getName());
+ if (!fieldSpec.isVirtualColumn()) {
+ FieldSpec.DataType dataType = fieldSpec.getDataType();
+ if (dataType == FieldSpec.DataType.FLOAT || dataType == FieldSpec.DataType.DOUBLE
+ || dataType == FieldSpec.DataType.BIG_DECIMAL) {
+ _columnsToCheck.add(fieldSpec.getName());
+ }
}
}
}
- private Object transformNegativeZero(Object value) {
- if ((value instanceof Float) && (Float.floatToRawIntBits((float) value) == Float.floatToRawIntBits(-0.0f))) {
- value = 0.0f;
- _negativeZeroConversionCount++;
- } else if ((value instanceof Double) && (Double.doubleToLongBits((double) value) == Double.doubleToLongBits(
- -0.0d))) {
- value = 0.0d;
- _negativeZeroConversionCount++;
- }
- return value;
- }
-
- private Object transformNaN(Object value) {
- if ((value instanceof Float) && ((Float) value).isNaN()) {
- value = null;
- _nanConversionCount++;
- } else if ((value instanceof Double) && ((Double) value).isNaN()) {
- _nanConversionCount++;
- value = null;
- }
- return value;
- }
-
@Override
public boolean isNoOp() {
- return _specialValuesKeySet.isEmpty();
+ return _columnsToCheck.isEmpty();
}
@Override
public GenericRow transform(GenericRow record) {
- for (String element : _specialValuesKeySet) {
- Object value = record.getValue(element);
+ for (String column : _columnsToCheck) {
+ Object value = record.getValue(column);
if (value instanceof Object[]) {
// Multi-valued column.
Object[] values = (Object[]) value;
- int numValues = values.length;
- List<Object> negativeZeroNanSanitizedValues = new ArrayList<>(numValues);
- for (Object o : values) {
- Object zeroTransformedValue = transformNegativeZero(o);
- Object nanTransformedValue = transformNaN(zeroTransformedValue);
- if (nanTransformedValue != null) {
- negativeZeroNanSanitizedValues.add(nanTransformedValue);
+ List<Object> transformedValues = new ArrayList<>(values.length);
+ boolean transformed = false;
+ for (Object v : values) {
+ Object transformedValue = transformValue(v);
+ if (transformedValue != v) {
+ transformed = true;
+ }
+ if (transformedValue != null) {
+ transformedValues.add(transformedValue);
+ }
+ if (transformed) {
+ record.putValue(column, !transformedValues.isEmpty() ? transformedValues.toArray() : null);
}
}
- record.putValue(element, negativeZeroNanSanitizedValues.toArray());
- } else {
+ } else if (value != null) {
// Single-valued column.
- Object zeroTransformedValue = transformNegativeZero(value);
- Object nanTransformedValue = transformNaN(zeroTransformedValue);
- if (nanTransformedValue != value) {
- record.putValue(element, nanTransformedValue);
+ Object transformedValue = transformValue(value);
+ if (transformedValue != value) {
+ record.putValue(column, transformedValue);
}
}
}
- if (_negativeZeroConversionCount > 0 || _nanConversionCount > 0) {
- LOGGER.debug("Converted {} -0.0s to 0.0 and {} NaNs to null", _negativeZeroConversionCount, _nanConversionCount);
- }
return record;
}
- @VisibleForTesting
- int getNegativeZeroConversionCount() {
- return _negativeZeroConversionCount;
- }
-
- @VisibleForTesting
- int getNanConversionCount() {
- return _nanConversionCount;
+ @Nullable
+ private Object transformValue(Object value) {
+ if (value instanceof Float) {
+ Float floatValue = (Float) value;
+ if (floatValue.isNaN()) {
+ return null;
+ }
+ if (Float.floatToRawIntBits(floatValue) == NEGATIVE_ZERO_FLOAT_BITS) {
+ return 0.0f;
+ }
+ } else if (value instanceof Double) {
+ Double doubleValue = (Double) value;
+ if (doubleValue.isNaN()) {
+ return null;
+ }
+ if (Double.doubleToRawLongBits(doubleValue) == NEGATIVE_ZERO_DOUBLE_BITS) {
+ return 0.0d;
+ }
+ } else if (value instanceof BigDecimal) {
+ BigDecimal bigDecimalValue = (BigDecimal) value;
+ BigDecimal stripped = bigDecimalValue.stripTrailingZeros();
+ if (!stripped.equals(bigDecimalValue)) {
+ return stripped;
+ }
Review Comment:
Hmm, big decimal is mostly used to store metric values for lossless aggregate. In case when `10.100` stands for different meaning from `10.1`, what is the actual use case? Does it make sense to store it as STRING?
--
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] Canonicalize BigDecimal values during ingestion [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #14958:
URL: https://github.com/apache/pinot/pull/14958
--
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] Canonicalize BigDecimal values during ingestion [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #14958:
URL: https://github.com/apache/pinot/pull/14958#issuecomment-2626202861
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/14958?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `97.61905%` with `1 line` in your changes missing coverage. Please review.
> Project coverage is 34.00%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`a65c961`)](https://app.codecov.io/gh/apache/pinot/commit/a65c9610f09a65ee233e34ee8bfe869b0f07bf60?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1652 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/14958?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...cal/recordtransformer/SpecialValueTransformer.java](https://app.codecov.io/gh/apache/pinot/pull/14958?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Frecordtransformer%2FSpecialValueTransformer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWNvcmR0cmFuc2Zvcm1lci9TcGVjaWFsVmFsdWVUcmFuc2Zvcm1lci5qYXZh) | 97.61% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/14958?src=pr&el=tree&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 (59551e4) and HEAD (a65c961). Click for more details.
>
> <details><summary>HEAD has 43 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (a65c961) |
>|------|------|------|
>|integration|7|1|
>|integration2|3|1|
>|temurin|12|3|
>|java-21|7|3|
>|skip-bytebuffers-true|3|1|
>|skip-bytebuffers-false|7|2|
>|unittests|5|2|
>|unittests1|2|0|
>|java-11|5|0|
>|unittests2|3|2|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #14958 +/- ##
=============================================
- Coverage 61.75% 34.00% -27.75%
- Complexity 207 667 +460
=============================================
Files 2436 2712 +276
Lines 133233 151999 +18766
Branches 20636 23493 +2857
=============================================
- Hits 82274 51691 -30583
- Misses 44911 96136 +51225
+ Partials 6048 4172 -1876
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/14958/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/14958/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/14958/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/14958/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/14958/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/14958/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/14958/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.00% <97.61%> (-27.62%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/14958/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.00% <97.61%> (-27.75%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/14958/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.99% <97.61%> (+6.26%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/14958/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.00% <97.61%> (-27.75%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/14958/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.00% <97.61%> (-27.74%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/14958/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/14958/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.00% <97.61%> (+6.27%)` | :arrow_up: |
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/14958?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).
--
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] Canonicalize BigDecimal values during ingestion [pinot]
Posted by "jadami10 (via GitHub)" <gi...@apache.org>.
jadami10 commented on code in PR #14958:
URL: https://github.com/apache/pinot/pull/14958#discussion_r1936749999
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/SpecialValueTransformer.java:
##########
@@ -18,112 +18,119 @@
*/
package org.apache.pinot.segment.local.recordtransformer;
-import com.google.common.annotations.VisibleForTesting;
+import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
+import java.util.Set;
+import javax.annotation.Nullable;
import org.apache.pinot.spi.data.FieldSpec;
-import org.apache.pinot.spi.data.FieldSpec.DataType;
import org.apache.pinot.spi.data.Schema;
import org.apache.pinot.spi.data.readers.GenericRow;
import org.apache.pinot.spi.recordtransformer.RecordTransformer;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
* The {@code SpecialValueTransformer} class will transform special values according to the following rules:
* <ul>
- * <li>Negative zero (-0.0) should be converted to 0.0</li>
- * <li>NaN should be converted to default null</li>
+ * <li>
+ * For FLOAT and DOUBLE:
+ * <ul>
+ * <li>Negative zero (-0.0) should be converted to 0.0</li>
+ * <li>NaN should be converted to default null</li>
+ * </ul>
+ * </li>
+ * <li>
+ * For BIG_DECIMAL:
+ * <ul>
+ * <li>Remove trailing zeros</li>
+ * </ul>
+ * </li>
* </ul>
* <p>NOTE: should put this after the {@link DataTypeTransformer} so that we already have the values complying
* with the schema before handling special values and before {@link NullValueTransformer} so that it transforms
* all the null values properly.
*/
public class SpecialValueTransformer implements RecordTransformer {
+ private final static int NEGATIVE_ZERO_FLOAT_BITS = Float.floatToRawIntBits(-0.0f);
+ private final static long NEGATIVE_ZERO_DOUBLE_BITS = Double.doubleToLongBits(-0.0d);
- private static final Logger LOGGER = LoggerFactory.getLogger(SpecialValueTransformer.class);
- private final HashSet<String> _specialValuesKeySet = new HashSet<>();
- private int _negativeZeroConversionCount = 0;
- private int _nanConversionCount = 0;
+ private final Set<String> _columnsToCheck = new HashSet<>();
public SpecialValueTransformer(Schema schema) {
for (FieldSpec fieldSpec : schema.getAllFieldSpecs()) {
- if (!fieldSpec.isVirtualColumn() && (fieldSpec.getDataType() == DataType.FLOAT
- || fieldSpec.getDataType() == DataType.DOUBLE)) {
- _specialValuesKeySet.add(fieldSpec.getName());
+ if (!fieldSpec.isVirtualColumn()) {
+ FieldSpec.DataType dataType = fieldSpec.getDataType();
+ if (dataType == FieldSpec.DataType.FLOAT || dataType == FieldSpec.DataType.DOUBLE
+ || dataType == FieldSpec.DataType.BIG_DECIMAL) {
+ _columnsToCheck.add(fieldSpec.getName());
+ }
}
}
}
- private Object transformNegativeZero(Object value) {
- if ((value instanceof Float) && (Float.floatToRawIntBits((float) value) == Float.floatToRawIntBits(-0.0f))) {
- value = 0.0f;
- _negativeZeroConversionCount++;
- } else if ((value instanceof Double) && (Double.doubleToLongBits((double) value) == Double.doubleToLongBits(
- -0.0d))) {
- value = 0.0d;
- _negativeZeroConversionCount++;
- }
- return value;
- }
-
- private Object transformNaN(Object value) {
- if ((value instanceof Float) && ((Float) value).isNaN()) {
- value = null;
- _nanConversionCount++;
- } else if ((value instanceof Double) && ((Double) value).isNaN()) {
- _nanConversionCount++;
- value = null;
- }
- return value;
- }
-
@Override
public boolean isNoOp() {
- return _specialValuesKeySet.isEmpty();
+ return _columnsToCheck.isEmpty();
}
@Override
public GenericRow transform(GenericRow record) {
- for (String element : _specialValuesKeySet) {
- Object value = record.getValue(element);
+ for (String column : _columnsToCheck) {
+ Object value = record.getValue(column);
if (value instanceof Object[]) {
// Multi-valued column.
Object[] values = (Object[]) value;
- int numValues = values.length;
- List<Object> negativeZeroNanSanitizedValues = new ArrayList<>(numValues);
- for (Object o : values) {
- Object zeroTransformedValue = transformNegativeZero(o);
- Object nanTransformedValue = transformNaN(zeroTransformedValue);
- if (nanTransformedValue != null) {
- negativeZeroNanSanitizedValues.add(nanTransformedValue);
+ List<Object> transformedValues = new ArrayList<>(values.length);
+ boolean transformed = false;
+ for (Object v : values) {
+ Object transformedValue = transformValue(v);
+ if (transformedValue != v) {
+ transformed = true;
+ }
+ if (transformedValue != null) {
+ transformedValues.add(transformedValue);
+ }
+ if (transformed) {
+ record.putValue(column, !transformedValues.isEmpty() ? transformedValues.toArray() : null);
}
}
- record.putValue(element, negativeZeroNanSanitizedValues.toArray());
- } else {
+ } else if (value != null) {
// Single-valued column.
- Object zeroTransformedValue = transformNegativeZero(value);
- Object nanTransformedValue = transformNaN(zeroTransformedValue);
- if (nanTransformedValue != value) {
- record.putValue(element, nanTransformedValue);
+ Object transformedValue = transformValue(value);
+ if (transformedValue != value) {
+ record.putValue(column, transformedValue);
}
}
}
- if (_negativeZeroConversionCount > 0 || _nanConversionCount > 0) {
- LOGGER.debug("Converted {} -0.0s to 0.0 and {} NaNs to null", _negativeZeroConversionCount, _nanConversionCount);
- }
return record;
}
- @VisibleForTesting
- int getNegativeZeroConversionCount() {
- return _negativeZeroConversionCount;
- }
-
- @VisibleForTesting
- int getNanConversionCount() {
- return _nanConversionCount;
+ @Nullable
+ private Object transformValue(Object value) {
+ if (value instanceof Float) {
+ Float floatValue = (Float) value;
+ if (floatValue.isNaN()) {
+ return null;
+ }
+ if (Float.floatToRawIntBits(floatValue) == NEGATIVE_ZERO_FLOAT_BITS) {
+ return 0.0f;
+ }
+ } else if (value instanceof Double) {
+ Double doubleValue = (Double) value;
+ if (doubleValue.isNaN()) {
+ return null;
+ }
+ if (Double.doubleToRawLongBits(doubleValue) == NEGATIVE_ZERO_DOUBLE_BITS) {
+ return 0.0d;
+ }
+ } else if (value instanceof BigDecimal) {
+ BigDecimal bigDecimalValue = (BigDecimal) value;
+ BigDecimal stripped = bigDecimalValue.stripTrailingZeros();
+ if (!stripped.equals(bigDecimalValue)) {
+ return stripped;
+ }
Review Comment:
ya, agreed that this messes up the index, but it's a problem with the index rather than the data. `10.100` actually has a different meaning than `10.1`, so we can't just transform one into the other. But `0` vs `0e-18` vs `0e18`, as far as I know, are all just 0.
--
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] Canonicalize BigDecimal values during ingestion [pinot]
Posted by "jadami10 (via GitHub)" <gi...@apache.org>.
jadami10 commented on PR #14958:
URL: https://github.com/apache/pinot/pull/14958#issuecomment-2629530401
looks good. The new flag is off by default it seems, so we will strip trailing zeroes. That seems like the right choice given it's much less likely someone will actually want to keep them.
--
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] Canonicalize BigDecimal values during ingestion [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #14958:
URL: https://github.com/apache/pinot/pull/14958#issuecomment-2626415405
@jadami10 For binary search, if the array is not sorted, there is no guarantee at all. We skipped several checks for performance reason, e.g. we don't allow binary search to return negative values because we know the searched value is within the smallest and largest value. I can try to fail the index creation when values are not properly sorted
--
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] Canonicalize BigDecimal values during ingestion [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #14958:
URL: https://github.com/apache/pinot/pull/14958#issuecomment-2628512617
@jadami10 I've introduced a new flag `allowTrailingZeros` in the schema so that user can set it if they don't want trailing zeros to be stripped, and they can guarantee the input data doesn't have same value but different trailing zeros.
--
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] Canonicalize BigDecimal values during ingestion [pinot]
Posted by "jadami10 (via GitHub)" <gi...@apache.org>.
jadami10 commented on code in PR #14958:
URL: https://github.com/apache/pinot/pull/14958#discussion_r1936729841
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/SpecialValueTransformer.java:
##########
@@ -18,112 +18,119 @@
*/
package org.apache.pinot.segment.local.recordtransformer;
-import com.google.common.annotations.VisibleForTesting;
+import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
+import java.util.Set;
+import javax.annotation.Nullable;
import org.apache.pinot.spi.data.FieldSpec;
-import org.apache.pinot.spi.data.FieldSpec.DataType;
import org.apache.pinot.spi.data.Schema;
import org.apache.pinot.spi.data.readers.GenericRow;
import org.apache.pinot.spi.recordtransformer.RecordTransformer;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
* The {@code SpecialValueTransformer} class will transform special values according to the following rules:
* <ul>
- * <li>Negative zero (-0.0) should be converted to 0.0</li>
- * <li>NaN should be converted to default null</li>
+ * <li>
+ * For FLOAT and DOUBLE:
+ * <ul>
+ * <li>Negative zero (-0.0) should be converted to 0.0</li>
+ * <li>NaN should be converted to default null</li>
+ * </ul>
+ * </li>
+ * <li>
+ * For BIG_DECIMAL:
+ * <ul>
+ * <li>Remove trailing zeros</li>
+ * </ul>
+ * </li>
* </ul>
* <p>NOTE: should put this after the {@link DataTypeTransformer} so that we already have the values complying
* with the schema before handling special values and before {@link NullValueTransformer} so that it transforms
* all the null values properly.
*/
public class SpecialValueTransformer implements RecordTransformer {
+ private final static int NEGATIVE_ZERO_FLOAT_BITS = Float.floatToRawIntBits(-0.0f);
+ private final static long NEGATIVE_ZERO_DOUBLE_BITS = Double.doubleToLongBits(-0.0d);
- private static final Logger LOGGER = LoggerFactory.getLogger(SpecialValueTransformer.class);
- private final HashSet<String> _specialValuesKeySet = new HashSet<>();
- private int _negativeZeroConversionCount = 0;
- private int _nanConversionCount = 0;
+ private final Set<String> _columnsToCheck = new HashSet<>();
public SpecialValueTransformer(Schema schema) {
for (FieldSpec fieldSpec : schema.getAllFieldSpecs()) {
- if (!fieldSpec.isVirtualColumn() && (fieldSpec.getDataType() == DataType.FLOAT
- || fieldSpec.getDataType() == DataType.DOUBLE)) {
- _specialValuesKeySet.add(fieldSpec.getName());
+ if (!fieldSpec.isVirtualColumn()) {
+ FieldSpec.DataType dataType = fieldSpec.getDataType();
+ if (dataType == FieldSpec.DataType.FLOAT || dataType == FieldSpec.DataType.DOUBLE
+ || dataType == FieldSpec.DataType.BIG_DECIMAL) {
+ _columnsToCheck.add(fieldSpec.getName());
+ }
}
}
}
- private Object transformNegativeZero(Object value) {
- if ((value instanceof Float) && (Float.floatToRawIntBits((float) value) == Float.floatToRawIntBits(-0.0f))) {
- value = 0.0f;
- _negativeZeroConversionCount++;
- } else if ((value instanceof Double) && (Double.doubleToLongBits((double) value) == Double.doubleToLongBits(
- -0.0d))) {
- value = 0.0d;
- _negativeZeroConversionCount++;
- }
- return value;
- }
-
- private Object transformNaN(Object value) {
- if ((value instanceof Float) && ((Float) value).isNaN()) {
- value = null;
- _nanConversionCount++;
- } else if ((value instanceof Double) && ((Double) value).isNaN()) {
- _nanConversionCount++;
- value = null;
- }
- return value;
- }
-
@Override
public boolean isNoOp() {
- return _specialValuesKeySet.isEmpty();
+ return _columnsToCheck.isEmpty();
}
@Override
public GenericRow transform(GenericRow record) {
- for (String element : _specialValuesKeySet) {
- Object value = record.getValue(element);
+ for (String column : _columnsToCheck) {
+ Object value = record.getValue(column);
if (value instanceof Object[]) {
// Multi-valued column.
Object[] values = (Object[]) value;
- int numValues = values.length;
- List<Object> negativeZeroNanSanitizedValues = new ArrayList<>(numValues);
- for (Object o : values) {
- Object zeroTransformedValue = transformNegativeZero(o);
- Object nanTransformedValue = transformNaN(zeroTransformedValue);
- if (nanTransformedValue != null) {
- negativeZeroNanSanitizedValues.add(nanTransformedValue);
+ List<Object> transformedValues = new ArrayList<>(values.length);
+ boolean transformed = false;
+ for (Object v : values) {
+ Object transformedValue = transformValue(v);
+ if (transformedValue != v) {
+ transformed = true;
+ }
+ if (transformedValue != null) {
+ transformedValues.add(transformedValue);
+ }
+ if (transformed) {
+ record.putValue(column, !transformedValues.isEmpty() ? transformedValues.toArray() : null);
}
}
- record.putValue(element, negativeZeroNanSanitizedValues.toArray());
- } else {
+ } else if (value != null) {
// Single-valued column.
- Object zeroTransformedValue = transformNegativeZero(value);
- Object nanTransformedValue = transformNaN(zeroTransformedValue);
- if (nanTransformedValue != value) {
- record.putValue(element, nanTransformedValue);
+ Object transformedValue = transformValue(value);
+ if (transformedValue != value) {
+ record.putValue(column, transformedValue);
}
}
}
- if (_negativeZeroConversionCount > 0 || _nanConversionCount > 0) {
- LOGGER.debug("Converted {} -0.0s to 0.0 and {} NaNs to null", _negativeZeroConversionCount, _nanConversionCount);
- }
return record;
}
- @VisibleForTesting
- int getNegativeZeroConversionCount() {
- return _negativeZeroConversionCount;
- }
-
- @VisibleForTesting
- int getNanConversionCount() {
- return _nanConversionCount;
+ @Nullable
+ private Object transformValue(Object value) {
+ if (value instanceof Float) {
+ Float floatValue = (Float) value;
+ if (floatValue.isNaN()) {
+ return null;
+ }
+ if (Float.floatToRawIntBits(floatValue) == NEGATIVE_ZERO_FLOAT_BITS) {
+ return 0.0f;
+ }
+ } else if (value instanceof Double) {
+ Double doubleValue = (Double) value;
+ if (doubleValue.isNaN()) {
+ return null;
+ }
+ if (Double.doubleToRawLongBits(doubleValue) == NEGATIVE_ZERO_DOUBLE_BITS) {
+ return 0.0d;
+ }
+ } else if (value instanceof BigDecimal) {
+ BigDecimal bigDecimalValue = (BigDecimal) value;
+ BigDecimal stripped = bigDecimalValue.stripTrailingZeros();
+ if (!stripped.equals(bigDecimalValue)) {
+ return stripped;
+ }
Review Comment:
doesn't this fundamentally change the data, though. If you have `10.100`, this will return `10.1`. But there's many scientific applications where those 2 trailing zeroes indicate actual significant figures and can't just be removed.
--
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] Canonicalize BigDecimal values during ingestion [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #14958:
URL: https://github.com/apache/pinot/pull/14958#discussion_r1936746829
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/SpecialValueTransformer.java:
##########
@@ -18,112 +18,119 @@
*/
package org.apache.pinot.segment.local.recordtransformer;
-import com.google.common.annotations.VisibleForTesting;
+import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
+import java.util.Set;
+import javax.annotation.Nullable;
import org.apache.pinot.spi.data.FieldSpec;
-import org.apache.pinot.spi.data.FieldSpec.DataType;
import org.apache.pinot.spi.data.Schema;
import org.apache.pinot.spi.data.readers.GenericRow;
import org.apache.pinot.spi.recordtransformer.RecordTransformer;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
/**
* The {@code SpecialValueTransformer} class will transform special values according to the following rules:
* <ul>
- * <li>Negative zero (-0.0) should be converted to 0.0</li>
- * <li>NaN should be converted to default null</li>
+ * <li>
+ * For FLOAT and DOUBLE:
+ * <ul>
+ * <li>Negative zero (-0.0) should be converted to 0.0</li>
+ * <li>NaN should be converted to default null</li>
+ * </ul>
+ * </li>
+ * <li>
+ * For BIG_DECIMAL:
+ * <ul>
+ * <li>Remove trailing zeros</li>
+ * </ul>
+ * </li>
* </ul>
* <p>NOTE: should put this after the {@link DataTypeTransformer} so that we already have the values complying
* with the schema before handling special values and before {@link NullValueTransformer} so that it transforms
* all the null values properly.
*/
public class SpecialValueTransformer implements RecordTransformer {
+ private final static int NEGATIVE_ZERO_FLOAT_BITS = Float.floatToRawIntBits(-0.0f);
+ private final static long NEGATIVE_ZERO_DOUBLE_BITS = Double.doubleToLongBits(-0.0d);
- private static final Logger LOGGER = LoggerFactory.getLogger(SpecialValueTransformer.class);
- private final HashSet<String> _specialValuesKeySet = new HashSet<>();
- private int _negativeZeroConversionCount = 0;
- private int _nanConversionCount = 0;
+ private final Set<String> _columnsToCheck = new HashSet<>();
public SpecialValueTransformer(Schema schema) {
for (FieldSpec fieldSpec : schema.getAllFieldSpecs()) {
- if (!fieldSpec.isVirtualColumn() && (fieldSpec.getDataType() == DataType.FLOAT
- || fieldSpec.getDataType() == DataType.DOUBLE)) {
- _specialValuesKeySet.add(fieldSpec.getName());
+ if (!fieldSpec.isVirtualColumn()) {
+ FieldSpec.DataType dataType = fieldSpec.getDataType();
+ if (dataType == FieldSpec.DataType.FLOAT || dataType == FieldSpec.DataType.DOUBLE
+ || dataType == FieldSpec.DataType.BIG_DECIMAL) {
+ _columnsToCheck.add(fieldSpec.getName());
+ }
}
}
}
- private Object transformNegativeZero(Object value) {
- if ((value instanceof Float) && (Float.floatToRawIntBits((float) value) == Float.floatToRawIntBits(-0.0f))) {
- value = 0.0f;
- _negativeZeroConversionCount++;
- } else if ((value instanceof Double) && (Double.doubleToLongBits((double) value) == Double.doubleToLongBits(
- -0.0d))) {
- value = 0.0d;
- _negativeZeroConversionCount++;
- }
- return value;
- }
-
- private Object transformNaN(Object value) {
- if ((value instanceof Float) && ((Float) value).isNaN()) {
- value = null;
- _nanConversionCount++;
- } else if ((value instanceof Double) && ((Double) value).isNaN()) {
- _nanConversionCount++;
- value = null;
- }
- return value;
- }
-
@Override
public boolean isNoOp() {
- return _specialValuesKeySet.isEmpty();
+ return _columnsToCheck.isEmpty();
}
@Override
public GenericRow transform(GenericRow record) {
- for (String element : _specialValuesKeySet) {
- Object value = record.getValue(element);
+ for (String column : _columnsToCheck) {
+ Object value = record.getValue(column);
if (value instanceof Object[]) {
// Multi-valued column.
Object[] values = (Object[]) value;
- int numValues = values.length;
- List<Object> negativeZeroNanSanitizedValues = new ArrayList<>(numValues);
- for (Object o : values) {
- Object zeroTransformedValue = transformNegativeZero(o);
- Object nanTransformedValue = transformNaN(zeroTransformedValue);
- if (nanTransformedValue != null) {
- negativeZeroNanSanitizedValues.add(nanTransformedValue);
+ List<Object> transformedValues = new ArrayList<>(values.length);
+ boolean transformed = false;
+ for (Object v : values) {
+ Object transformedValue = transformValue(v);
+ if (transformedValue != v) {
+ transformed = true;
+ }
+ if (transformedValue != null) {
+ transformedValues.add(transformedValue);
+ }
+ if (transformed) {
+ record.putValue(column, !transformedValues.isEmpty() ? transformedValues.toArray() : null);
}
}
- record.putValue(element, negativeZeroNanSanitizedValues.toArray());
- } else {
+ } else if (value != null) {
// Single-valued column.
- Object zeroTransformedValue = transformNegativeZero(value);
- Object nanTransformedValue = transformNaN(zeroTransformedValue);
- if (nanTransformedValue != value) {
- record.putValue(element, nanTransformedValue);
+ Object transformedValue = transformValue(value);
+ if (transformedValue != value) {
+ record.putValue(column, transformedValue);
}
}
}
- if (_negativeZeroConversionCount > 0 || _nanConversionCount > 0) {
- LOGGER.debug("Converted {} -0.0s to 0.0 and {} NaNs to null", _negativeZeroConversionCount, _nanConversionCount);
- }
return record;
}
- @VisibleForTesting
- int getNegativeZeroConversionCount() {
- return _negativeZeroConversionCount;
- }
-
- @VisibleForTesting
- int getNanConversionCount() {
- return _nanConversionCount;
+ @Nullable
+ private Object transformValue(Object value) {
+ if (value instanceof Float) {
+ Float floatValue = (Float) value;
+ if (floatValue.isNaN()) {
+ return null;
+ }
+ if (Float.floatToRawIntBits(floatValue) == NEGATIVE_ZERO_FLOAT_BITS) {
+ return 0.0f;
+ }
+ } else if (value instanceof Double) {
+ Double doubleValue = (Double) value;
+ if (doubleValue.isNaN()) {
+ return null;
+ }
+ if (Double.doubleToRawLongBits(doubleValue) == NEGATIVE_ZERO_DOUBLE_BITS) {
+ return 0.0d;
+ }
+ } else if (value instanceof BigDecimal) {
+ BigDecimal bigDecimalValue = (BigDecimal) value;
+ BigDecimal stripped = bigDecimalValue.stripTrailingZeros();
+ if (!stripped.equals(bigDecimalValue)) {
+ return stripped;
+ }
Review Comment:
I don't fully follow, can you elaborate? We need to strip zeros because `10.100` and `10.1` can also mess up the index.
--
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] Canonicalize BigDecimal values during ingestion [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #14958:
URL: https://github.com/apache/pinot/pull/14958#issuecomment-2626528318
> Maybe the fix should be where we set `isSorted`? Meaning when we're iterating values checking if it's sorted, we should check that both `.equals` and `.compareTo` return (`true` and `0`) or (`false` and `1/-1`). Otherwise, the index just can't properly handle it.
Sorted index is just one structure that can be broken because the ordering is not consistent with equals. Other data structure such as `TreeMap` could also break, and you may read more about that in the javadoc of `Comparable` interface.
This part is interesting, which literally explains this problem:
```
* It is strongly recommended (though not required) that natural orderings be
* consistent with equals. This is so because sorted sets (and sorted maps)
* without explicit comparators behave "strangely" when they are used with
* elements (or keys) whose natural ordering is inconsistent with equals. In
* particular, such a sorted set (or sorted map) violates the general contract
* for set (or map), which is defined in terms of the {@code equals}
* method.<p>
*
* For example, if one adds two keys {@code a} and {@code b} such that
* {@code (!a.equals(b) && a.compareTo(b) == 0)} to a sorted
* set that does not use an explicit comparator, the second {@code add}
* operation returns false (and the size of the sorted set does not increase)
* because {@code a} and {@code b} are equivalent from the sorted set's
* perspective.<p>
*
* Virtually all Java core classes that implement {@code Comparable} have natural
* orderings that are consistent with equals. One exception is
* {@code java.math.BigDecimal}, whose natural ordering equates
* {@code BigDecimal} objects with equal values and different precisions
* (such as 4.0 and 4.00).<p>
```
I don't think we can handle this in all 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