Copilot commented on code in PR #19595:
URL: https://github.com/apache/datafusion/pull/19595#discussion_r2656645780
##########
datafusion/datasource-parquet/src/metadata.rs:
##########
@@ -613,6 +616,40 @@ impl FileMetadata for CachedParquetMetaData {
}
}
+/// Convert a [`PhysicalSortExpr`] to a Parquet [`SortingColumn`].
+///
+/// Returns `Err` if the expression is not a simple column reference.
+pub(crate) fn sort_expr_to_sorting_column(
+ sort_expr: &PhysicalSortExpr,
+) -> Result<SortingColumn> {
+ let column = sort_expr
+ .expr
+ .as_any()
+ .downcast_ref::<Column>()
+ .ok_or_else(|| {
+ DataFusionError::Plan(format!(
+ "Parquet sorting_columns only supports simple column
references, \
+ but got expression: {}",
+ sort_expr.expr
+ ))
+ })?;
+
+ Ok(SortingColumn {
+ column_idx: column.index() as i32,
Review Comment:
The conversion from `usize` to `i32` could potentially overflow for schemas
with a very large number of columns. While unlikely in practice, consider using
`try_into()` with proper error handling to make this conversion safe and
explicit.
```suggestion
let column_idx: i32 = column.index().try_into().map_err(|_| {
DataFusionError::Plan(format!(
"Column index {} is too large to be represented as i32",
column.index()
))
})?;
Ok(SortingColumn {
column_idx,
```
--
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]
