CodiumAI-Agent commented on PR #9029: URL: https://github.com/apache/incubator-gluten/pull/9029#issuecomment-2728938490
## PR Reviewer Guide ๐ Here are some key observations to aid the review process: <table> <tr><td> **๐ซ Ticket compliance analysis ๐ถ** **[8872](https://github.com/apache/incubator-gluten/issues/8872) - Partially compliant** Compliant requirements: - Fix reading bug for update operations. - Adding tests for delete, update, and upsert operations. Non-compliant requirements: - Verification of behavior when no deletion vector exists. Requires further human verification: - Validation of functional correctness for CH backend behavior. </td></tr> <tr><td>โฑ๏ธ <strong>Estimated effort to review</strong>: 4 ๐ต๐ต๐ต๐ตโช</td></tr> <tr><td>๐งช <strong>PR contains tests</strong></td></tr> <tr><td>๐ <strong>No security concerns identified</strong></td></tr> <tr><td>โก <strong>Recommended focus areas for review</strong><br><br> <details><summary><a href='https://github.com/apache/incubator-gluten/pull/9029/files#diff-36f06b08ace618c32827d19432436597006f12fadf5db2e7cdc689903f405e71R48-R53'><strong>Exception Safety</strong></a> The new condition checking the row index filter type throws an exception if the provided filter type is not supported. It is important to verify that the exception message and error code properly communicate the issue and that all possible invalid inputs are handled as expected. </summary> ```c++ { if (row_index_filter_type != DeltaDVBitmapConfig::DELTA_ROW_INDEX_FILTER_TYPE) throw DB::Exception(DB::ErrorCodes::BAD_ARGUMENTS, "Row index filter type does not support : {}", row_index_filter_type); bitmap_config_ = std::make_shared<DeltaDVBitmapConfig>(); rapidjson::Document doc; ``` </details> <details><summary><a href='https://github.com/apache/incubator-gluten/pull/9029/files#diff-36f06b08ace618c32827d19432436597006f12fadf5db2e7cdc689903f405e71R111-R125'><strong>Nullable Bitmap Array</strong></a> The updated implementation in deleteRowsByDV checks for a null bitmap array. Ensure that in cases where bitmap_array is not set, the fallback behavior (setting all deletion flags to false) is the desired outcome. </summary> ```c++ if (bitmap_array) { size_t tmp_row_id_pos_output = readHeader.getPositionByName(ParquetVirtualMeta::TMP_ROWINDEX); size_t tmp_row_id_pos = chunk.getNumColumns() - 1; if (tmp_row_id_pos_output < tmp_row_id_pos) tmp_row_id_pos = tmp_row_id_pos_output; for (int i = 0; i < num_rows; i++) vec[i] = bitmap_array->rb_contains(chunk.getColumns()[tmp_row_id_pos]->get64(i)); } else { // the bitmap array is null, set the values of the '__delta_internal_is_row_deleted' column to the false for (int i = 0; i < num_rows; i++) vec[i] = false; } ``` </details> </td></tr> </table> -- 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]
