martin-g commented on code in PR #21577:
URL: https://github.com/apache/datafusion/pull/21577#discussion_r3072543663
##########
datafusion/sqllogictest/src/engines/datafusion_engine/normalize.rs:
##########
@@ -233,18 +238,15 @@ pub fn cell_to_string(col: &ArrayRef, row: usize,
is_spark_path: bool) -> Result
DataType::Dictionary(_, _) => {
let dict = col.as_any_dictionary();
let key = dict.normalized_keys()[row];
- Ok(cell_to_string(dict.values(), key, is_spark_path)?)
+ Ok(cell_to_string(
+ dict.values(),
+ key,
+ is_spark_path,
+ format_options,
+ )?)
}
_ => {
- let mut datafusion_format_options =
- datafusion::config::FormatOptions::default();
-
- datafusion_format_options.set("null", "NULL").unwrap();
-
- let arrow_format_options: arrow::util::display::FormatOptions =
- (&datafusion_format_options).try_into().unwrap();
-
- let f = ArrayFormatter::try_new(col.as_ref(),
&arrow_format_options)?;
+ let f = ArrayFormatter::try_new(col.as_ref(), format_options)?;
Review Comment:
This creates an ArrayFormatter instance for each cell.
It would be better to create one per column and reuse it here.
##########
datafusion/sqllogictest/src/engines/datafusion_engine/normalize.rs:
##########
Review Comment:
```suggestion
DataType::Null => Ok(format_options.null.to_string()),
```
##########
datafusion/sqllogictest/test_files/set_variable.slt:
##########
Review Comment:
It would be good to add a `SELECT` query here to validate that `Null` is
returned instead of the default `NULL`.
##########
datafusion/sqllogictest/src/util.rs:
##########
@@ -141,6 +143,13 @@ pub fn is_spark_path(relative_path: &Path) -> bool {
relative_path.starts_with("spark/")
}
+// Get passed custom FormatOptions by SessionContext to be used for
sqllogictest
Review Comment:
```suggestion
/// Get passed custom FormatOptions by SessionContext to be used for
sqllogictest
```
##########
datafusion/sqllogictest/src/util.rs:
##########
@@ -141,6 +143,13 @@ pub fn is_spark_path(relative_path: &Path) -> bool {
relative_path.starts_with("spark/")
}
+// Get passed custom FormatOptions by SessionContext to be used for
sqllogictest
+pub fn get_format_options(ctx: &SessionContext) -> Result<FormatOptions> {
+ let mut df_format = ctx.state().config().options().format.clone();
+ df_format.set("null", "NULL")?;
Review Comment:
IMO this should be done only when the old value is `""`, i.e. the default
one in Arrow-rs. If the user has set a custom one then do not overwrite it.
##########
datafusion/sqllogictest/src/engines/datafusion_engine/normalize.rs:
##########
@@ -185,7 +185,12 @@ macro_rules! get_row_value {
/// [NULL Values and empty strings]:
https://duckdb.org/dev/sqllogictest/result_verification#null-values-and-empty-strings
///
/// Floating numbers are rounded to have a consistent representation with the
Postgres runner.
-pub fn cell_to_string(col: &ArrayRef, row: usize, is_spark_path: bool) ->
Result<String> {
+pub fn cell_to_string(
+ col: &ArrayRef,
+ row: usize,
+ is_spark_path: bool,
+ format_options: &FormatOptions<'_>,
+) -> Result<String> {
if col.is_null(row) {
// represent any null value with the string "NULL"
Ok(NULL_STR.to_string())
Review Comment:
```suggestion
Ok(format_options.null.to_string())
```
--
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]
