CodiumAI-Agent commented on PR #8947: URL: https://github.com/apache/incubator-gluten/pull/8947#issuecomment-2712250834
## PR Reviewer Guide ๐ Here are some key observations to aid the review process: <table> <tr><td>โฑ๏ธ <strong>Estimated effort to review</strong>: 5 ๐ต๐ต๐ต๐ต๐ต</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/8947/files#diff-f1625a6ac2a39d4b79842f262935a102a793681c99382f3439566aa10c12f776R60-R68'><strong>Null Check</strong></a> In the rb_read function, the new dynamic_cast result (ReadBufferBuilderPtr) is used without verifying if the pointer is non-null. Please add a proper null check to prevent potential null-pointer dereferences. </summary> ```c++ file_info.set_length(data_size); const Poco::URI file_uri(file_path); ReadBufferBuilderPtr read_buffer_builder = ReadBufferBuilderFactory::instance().createBuilder(file_uri.getScheme(), context); auto * in = dynamic_cast<DB::SeekableReadBuffer *>(read_buffer_builder->build(file_info).release()); in->seek(offset, SEEK_SET); int size; readBinaryBigEndian(size, *in); ``` </details> <details><summary><a href='https://github.com/apache/incubator-gluten/pull/8947/files#diff-a7726893f4e744d78d76bb606f5d0c56ae0138d8e10e532ea50b60c3a7e7e392R42-R66'><strong>JSON Parsing</strong></a> The new convertRowIndexFilterIdEncoded function parses JSON without explicit error handling. Consider validating the parsed output and handling possible parse errors or unexpected formats. </summary> ```scala override def convertRowIndexFilterIdEncoded( file: PartitionedFile, otherConstantMetadataColumnValues: JMap[String, Object]): JMap[String, Object] = { val newOtherConstantMetadataColumnValues: JMap[String, Object] = new JHashMap[String, Object] for ((k, v) <- otherConstantMetadataColumnValues.asScala) { if (k.equalsIgnoreCase(DeltaParquetFileFormat.FILE_ROW_INDEX_FILTER_ID_ENCODED)) { val decoded = JsonUtils.fromJson[DeletionVectorDescriptor](v.toString) val decodedPath = decoded.absolutePath(new Path(file.filePath.toString()).getParent) val newDeletionVectorDescriptor = decoded.copy( decoded.storageType, decodedPath.toUri.toASCIIString, decoded.offset, decoded.sizeInBytes, decoded.cardinality, decoded.maxRowIndex ) newOtherConstantMetadataColumnValues.put(k, JsonUtils.toJson(newDeletionVectorDescriptor)) } else { newOtherConstantMetadataColumnValues.put(k, v) } } newOtherConstantMetadataColumnValues } ``` </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]
