[I] Compound RLI does not escape the char that used as separator [hudi]

["hudi-bot (via GitHub)" <gi...@apache.org>]

hudi-bot opened a new issue, #17168:
URL: https://github.com/apache/hudi/issues/17168

   Compound RLI we use "," as separator. But for actual record key value, we didn't escape the separator
   {code:java}
   select key from hudi_metadata('customers') where type=7 order by key;
   bob2@example.com$customer_id:3,customer_name:Bob, Johnson
   jane@example.com$customer_id:2,customer_name:Jane, Doe
   john.smith@example.com$customer_id:1,customer_name:John Smith
   john@example.com$customer_id:1,customer_name:John, Smith {code}
   I didn't find correctness issue so far based on my very limited understanding of compound RLI. I'm scared by the fact that this behavior is not compliant with the basic design principles of handling strings which makes the logic very shaky IMO.
   
    
   
   Extending this to all other indices. We should think about this as well.
   
    
   
   if an index could come with some compound fashion + no separator escaping in the value content, we must make sure we
    * never try to split The key based on the separator
    * never do prefix lookup (compound keys has 2 keys, we prefix lookup on the first key)
   
   ## JIRA info
   
   - Link: https://issues.apache.org/jira/browse/HUDI-9738
   - Type: Bug
   - Fix version(s):
     - 1.1.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@hudi.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Compound RLI does not escape the char that used as separator [hudi]

["rangareddy (via GitHub)" <gi...@apache.org>]

rangareddy commented on issue #17168:
URL: https://github.com/apache/hudi/issues/17168#issuecomment-5366103879

   This issue was reviewed as part of the JIRA-migrated backlog triage (HUDI-9738).
   
   **Findings: confirmed, and the parsing code is itself the strongest evidence.**
   
   **Write side does not escape.** `hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/keygen/BuiltinKeyGenerator.java:224` and `:275` append `DEFAULT_RECORD_KEY_PARTS_SEPARATOR` directly around raw field values, so a value containing the separator goes in verbatim - which is how you get `customer_name:Bob, Johnson`.
   
   **Read side compensates with a heuristic.** `KeyGenUtils.extractRecordKeysByFields` (`hudi-client/hudi-client-common/src/main/java/org/apache/hudi/keygen/KeyGenUtils.java:139`) starts with:
   
   ```java
   // if there is no ',' and ':', then it's a key value
   if (!recordKey.contains(DEFAULT_RECORD_KEY_PARTS_SEPARATOR) || !recordKey.contains(DEFAULT_COLUMN_VALUE_SEPARATOR)) {
     return new String[] {recordKey};
   }
   ```
   
   and then does index arithmetic with repeated `lastIndexOf(DEFAULT_RECORD_KEY_PARTS_SEPARATOR, keyValueSep2)` calls at `:166`, `:181`, `:204` and `:208` - scanning backwards to guess where one field ends and the next begins.
   
   That backwards-scanning logic exists *only* because the separator is unescaped in the value. It is a heuristic, which means whether a key round-trips correctly depends on the data rather than on the format. Your caution is well placed: absence of an observed correctness bug is not the same as correctness here, and a value containing `:` as well as `,` is the case most likely to break it.
   
   Worth noting that changing the encoding is an on-disk compatibility change for existing record-index entries, so it likely needs to ride an index-version bump rather than being fixed in place.
   
   Keeping this open.
   


-- 
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@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org