xuchenhao commented on PR #59065: URL: https://github.com/apache/doris/pull/59065#issuecomment-4022421168
Hi @github-actions[bot], thank you for the thorough review. I have addressed all the issues mentioned in your summary. Here's a breakdown of the changes: ### Critical Issues 1. **[Bug] Missing `__isset` check for optional Thrift field**: Added `__isset` checks in `CsvReader`, `NewJsonReader`, `ParquetReader`, and `OrcReader` before assigning `file_cache_admission` to ensure backward compatibility. 2. **[Bug] Lock not in try-finally blocks**: Wrapped all `readLock.unlock()` and `writeLock.unlock()` calls in `finally` blocks in `FileCacheAdmissionManager.java` to prevent potential deadlocks. 3. **[Bug] Potential NPE from `File.listFiles()`**: Added a null check for the return value of `ruleDir.listFiles()` in `FileCacheAdmissionManager.java`. ### Moderate Issues 4. **[Bug] `ConcurrentRuleManager.getIndex()` out of bounds**: Restricted `getIndex()` to handle only ASCII alphabetic characters (`A-Z`, `a-z`) and added defensive checks for other characters. 5. **[Style] Unnecessary `Boolean` boxing**: Replaced `Boolean` with primitive `boolean` in `SplitAssignment`, `FileQueryScanNode`, and `RuleCollection`. 6. **[Robustness] `loadOnStartup()` validation**: Added validation for `file_cache_admission_control_json_dir` before starting the `ConfigWatcher`. ### Minor Observations - Replaced boxed `Boolean` with `boolean` in `RuleCollection`. - Removed dead code and simplified `logDefaultAdmission`. - **Updated tests**: Refactored `FileCacheAdmissionRuleRefresherTest.java` to use `Files.createTempDirectory` instead of writing to the current working directory. All changes have been verified and unit tests have been updated accordingly. Please let me know if there's anything else! -- 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]
