[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #11218: Fix for escape/unescape issue for segment min/max values.

["Jackie-Jiang (via GitHub)" <gi...@apache.org>]

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: 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