Jackie-Jiang commented on code in PR #13103:
URL: https://github.com/apache/pinot/pull/13103#discussion_r1610619081
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -175,6 +203,15 @@ public void setMaxLength(int maxLength) {
_maxLength = maxLength;
}
+ public MaxLengthExceedStrategy getMaxLengthExceedStrategy() {
+ return _maxLengthExceedStrategy;
+ }
+
+ // Required by JSON de-serializer. DO NOT REMOVE.
+ public void setMaxLengthExceedStrategy(MaxLengthExceedStrategy
maxLengthExceedStrategy) {
Review Comment:
```suggestion
public void setMaxLengthExceedStrategy(@Nullable MaxLengthExceedStrategy
maxLengthExceedStrategy) {
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -115,6 +121,9 @@ public abstract class FieldSpec implements
Comparable<FieldSpec>, Serializable {
protected String _virtualColumnProvider;
+ // NOTE: This only applies to STRING column during {@link
SanitizationTransformer}
+ protected MaxLengthExceedStrategy _maxLengthExceedStrategy;
Review Comment:
(minor) Let's put this next to `_maxLength`
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -175,6 +203,15 @@ public void setMaxLength(int maxLength) {
_maxLength = maxLength;
}
+ public MaxLengthExceedStrategy getMaxLengthExceedStrategy() {
Review Comment:
```suggestion
@Nullable
public MaxLengthExceedStrategy getMaxLengthExceedStrategy() {
```
##########
pinot-common/src/test/java/org/apache/pinot/common/data/FieldSpecTest.java:
##########
@@ -312,9 +316,10 @@ public void testOrderOfFields()
// Multi-value dimension field with default null value.
dimensionFields = new String[]{
"\"name\":\"dimension\"", "\"dataType\":\"STRING\"",
"\"singleValueField\":false",
- "\"defaultNullValue\":\"default\""
+ "\"defaultNullValue\":\"default\", \"max length exceed strategy\":
\"TRIM_LENGTH\""
Review Comment:
Is this correct? Should it be `maxLengthExceedStrategy`?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -129,11 +138,30 @@ public FieldSpec(String name, DataType dataType, boolean
isSingleValueField, @Nu
public FieldSpec(String name, DataType dataType, boolean isSingleValueField,
int maxLength,
@Nullable Object defaultNullValue) {
+ this(name, dataType, isSingleValueField, maxLength, defaultNullValue,
null);
+ }
+
+ public FieldSpec(String name, DataType dataType, boolean isSingleValueField,
int maxLength,
+ @Nullable Object defaultNullValue, @Nullable MaxLengthExceedStrategy
maxLengthExceedStrategy) {
_name = name;
_dataType = dataType;
_isSingleValueField = isSingleValueField;
_maxLength = maxLength;
setDefaultNullValue(defaultNullValue);
+ switch (_dataType) {
Review Comment:
We don't need to set it here. We can simply assign `_maxLengthExceedStrategy
= maxLengthExceedStrategy`. Basically `FieldSpec.getMaxLengthExceedStrategy()`
can return `null`, and `SanitizationTransformer` knows what default value to
use when it is not explicitly configured
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
