Jackie-Jiang commented on code in PR #11218:
URL: https://github.com/apache/pinot/pull/11218#discussion_r1278227361
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -605,30 +615,30 @@ static boolean isValidPropertyValue(String value) {
@VisibleForTesting
static String getValidPropertyValue(String value, boolean isMax, DataType
dataType) {
String valueWithinLengthLimit = getValueWithinLengthLimit(value, isMax,
dataType);
- int length = valueWithinLengthLimit.length();
+ String escapedValue = StringEscapeUtils.escapeJava(valueWithinLengthLimit);
+ int length = escapedValue.length();
if (length > 0) {
// check and replace first character if it's a white space
- if (Character.isWhitespace(valueWithinLengthLimit.charAt(0))) {
- String unicodeValue = "\\u" + String.format("%04x", (int)
valueWithinLengthLimit.charAt(0));
- valueWithinLengthLimit = unicodeValue +
valueWithinLengthLimit.substring(1);
+ if (Character.isWhitespace(escapedValue.charAt(0))) {
Review Comment:
Will escape handle whitespace? If so, we don't need to explicitly handle
whitespace
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -571,8 +572,17 @@ public static void
addColumnMetadataInfo(PropertiesConfiguration properties, Str
public static void addColumnMinMaxValueInfo(PropertiesConfiguration
properties, String column, String minValue,
String maxValue, DataType dataType) {
- properties.setProperty(getKeyFor(column, MIN_VALUE),
getValidPropertyValue(minValue, false, dataType));
- properties.setProperty(getKeyFor(column, MAX_VALUE),
getValidPropertyValue(maxValue, true, dataType));
+ String validMinValue = getValidPropertyValue(minValue, false, dataType);
+ properties.setProperty(getKeyFor(column, MIN_VALUE), validMinValue);
+
+ String validMaxValue = getValidPropertyValue(maxValue, true, dataType);
+ properties.setProperty(getKeyFor(column, MAX_VALUE), validMaxValue);
+
+ // setting the property for signifying that the mon or max value is
escaped.
+ if (validMinValue.compareTo(minValue) != 0
Review Comment:
We want to compare the result before and after the escape. With current
code, we will count it as escaped even when it is just modified but not escaped.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -571,8 +572,17 @@ public static void
addColumnMetadataInfo(PropertiesConfiguration properties, Str
public static void addColumnMinMaxValueInfo(PropertiesConfiguration
properties, String column, String minValue,
String maxValue, DataType dataType) {
- properties.setProperty(getKeyFor(column, MIN_VALUE),
getValidPropertyValue(minValue, false, dataType));
- properties.setProperty(getKeyFor(column, MAX_VALUE),
getValidPropertyValue(maxValue, true, dataType));
+ String validMinValue = getValidPropertyValue(minValue, false, dataType);
+ properties.setProperty(getKeyFor(column, MIN_VALUE), validMinValue);
+
+ String validMaxValue = getValidPropertyValue(maxValue, true, dataType);
+ properties.setProperty(getKeyFor(column, MAX_VALUE), validMaxValue);
+
+ // setting the property for signifying that the mon or max value is
escaped.
+ if (validMinValue.compareTo(minValue) != 0
+ || validMaxValue.compareTo(maxValue) != 0) {
+ properties.setProperty(getKeyFor(column, IS_MIN_MAX_VALUE_ESCAPED),
true);
Review Comment:
After a second thought, keep `iS_MIN_VALUE_ESCAPED` and
`IS_MAX_VALUE_ESCAPED` separately might be better
--
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]
