CodiumAI-Agent commented on PR #9088: URL: https://github.com/apache/incubator-gluten/pull/9088#issuecomment-2750779120
## PR Reviewer Guide ๐ Here are some key observations to aid the review process: <table> <tr><td> **๐ซ Ticket compliance analysis ๐ถ** **[9087](https://github.com/apache/incubator-gluten/issues/9087) - Partially compliant** Compliant requirements: - Spark dependency removals by replacing Spark-specific imports and methods. - Use of new conversion utilities (e.g., SparkToJavaConverter) for type translations. - Refactoring of window function nodes using WindowFunctionNodeUtil. - Updating literal builders to use native Java types (e.g., BigDecimal) and new SchemaField. Non-compliant requirements: - No outstanding non-compliances detected. Requires further human verification: - Verify that all edge cases for window function boundary handling (foldable flags, offsets, reference nodes) are correctly implemented. - Confirm integration with non-Spark backends like Flink for full functional parity. </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/9088/files#diff-305962c4761679ea182cb5230877479e8ab399bee70ff3456dbf30995a2ce536R81-R119'><strong>Window Function Boundaries</strong></a> The new implementation introduces several additional parameters (foldable flags, offsets, reference nodes) for window boundaries. Further validation is needed to ensure these changes correctly handle all edge cases across different backends. </summary> ```java this.columnName = columnName; this.outputTypeNode = outputTypeNode; this.upperBound = upperBound; this.lowerBound = lowerBound; this.frameType = frameType; this.ignoreNulls = ignoreNulls; this.upperBoundFoldable = upperBoundFoldable; this.lowerBoundFoldable = lowerBoundFoldable; this.upperBoundOffset = upperBoundOffset; this.lowerBoundOffset = lowerBoundOffset; this.upperBoundRefNode = upperBoundRefNode; this.lowerBoundRefNode = lowerBoundRefNode; this.isPreComputeRangeFrameUpperBound = isPreComputeRangeFrameUpperBound; this.isPreComputeRangeFrameLowerBound = isPreComputeRangeFrameLowerBound; } private Expression.WindowFunction.Bound.Builder setBound( Expression.WindowFunction.Bound.Builder builder, String boundType, boolean foldable, long offset, ExpressionNode refNode, boolean isPreComputeRangeFrameBound) { switch (boundType) { case ("CURRENT ROW"): Expression.WindowFunction.Bound.CurrentRow.Builder currentRowBuilder = Expression.WindowFunction.Bound.CurrentRow.newBuilder(); builder.setCurrentRow(currentRowBuilder.build()); break; case ("UNBOUNDED PRECEDING"): Expression.WindowFunction.Bound.Unbounded_Preceding.Builder precedingBuilder = Expression.WindowFunction.Bound.Unbounded_Preceding.newBuilder(); builder.setUnboundedPreceding(precedingBuilder.build()); break; case ("UNBOUNDED FOLLOWING"): Expression.WindowFunction.Bound.Unbounded_Following.Builder followingBuilder = Expression.WindowFunction.Bound.Unbounded_Following.newBuilder(); builder.setUnboundedFollowing(followingBuilder.build()); break; ``` </details> <details><summary><a href='https://github.com/apache/incubator-gluten/pull/9088/files#diff-61f5a3236df11322e6a3ab40b6f686b811fc89c37808408468bfc388a108309aR51-R100'><strong>Type Conversion Consistency</strong></a> The upgraded conversion logic for decimals, lists, maps, and structs relies on the new SparkToJavaConverter. Ensure that the conversion handles nulls and complex nested structures reliably across various data types. </summary> ```java private static Object toJava(Object obj, TypeNode nodeType) { return toJava(obj, nodeType, null); } public static Object toJava(Object obj, TypeNode nodeType, DataType dataType) { if (obj == null) { return null; } if ((nodeType instanceof BooleanTypeNode) || (nodeType instanceof I8TypeNode) || (nodeType instanceof I16TypeNode) || (nodeType instanceof I32TypeNode) || (nodeType instanceof I64TypeNode) || (nodeType instanceof FP32TypeNode) || (nodeType instanceof FP64TypeNode) || (nodeType instanceof DateTypeNode) || (nodeType instanceof TimestampTypeNode) || (nodeType instanceof StringTypeNode) || (nodeType instanceof BinaryTypeNode)) { return obj; } else if (nodeType instanceof DecimalTypeNode) { return ((Decimal) obj).toJavaBigDecimal(); } else if (nodeType instanceof ListNode) { ListNode listType = (ListNode) nodeType; if (obj instanceof UnsafeArrayData) { UnsafeArrayData unsafeArray = (UnsafeArrayData) obj; List<Object> javaList = new ArrayList<>(unsafeArray.numElements()); DataType elementType = ((ArrayType) dataType).elementType(); for (int i = 0; i < unsafeArray.numElements(); i++) { javaList.add(toJava(unsafeArray.get(i, elementType), listType.getNestedType())); } return javaList; } else { ArrayData array = (ArrayData) obj; List<Object> javaList = new ArrayList<>(array.numElements()); for (Object elem : array.array()) { javaList.add(toJava(elem, listType.getNestedType())); } return javaList; } } else if (nodeType instanceof MapNode) { MapNode mapType = (MapNode) nodeType; MapData map = (MapData) obj; Map<Object, Object> javaMap = new HashMap<>(map.numElements()); for (int i = 0; i < map.numElements(); i++) { javaMap.put( toJava(map.keyArray().array()[i], mapType.getKeyType()), toJava(map.valueArray().array()[i], mapType.getValueType())); } return javaMap; ``` </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]
