comphead commented on code in PR #98:
URL: https://github.com/apache/datafusion-spark/pull/98#discussion_r4566785311
##########
crates/spark/src/function/map/str_to_map.rs:
##########
@@ -275,7 +266,7 @@ fn str_to_map_impl<'a, V: StringArrayType<'a> + Copy>(
"[DUPLICATED_MAP_KEY] Duplicate map key '{key}' was
found, \
please check the input data. To allow duplicate keys
with \
last-value-wins semantics, set \
- `datafusion.spark.map_key_dedup_policy` to
`LAST_WIN`."
+ `spark.map_key_dedup_policy` to `LAST_WIN`."
Review Comment:
This message now names the new key, but the same `[DUPLICATED_MAP_KEY]`
message in `utils.rs` (line 223, used by `map_from_arrays` and
`map_from_entries`) still tells users to set
`datafusion.spark.map_key_dedup_policy`. Those functions no longer read that
key, and it still exists in core on `branch-56`, so following the hint succeeds
silently and changes nothing. Could `utils.rs` say `spark.map_key_dedup_policy`
too? #80 converts those `DUPLICATED_MAP_KEY` checks to full-message assertions
that include the old key name, so whichever lands second needs to update the
expected text.
##########
crates/spark/src/session_state.rs:
##########
@@ -42,6 +43,17 @@ pub trait SessionStateBuilderSpark {
impl SessionStateBuilderSpark for SessionStateBuilder {
fn with_spark_features(mut self) -> Self {
+ // Check if SparkOptions already exists; if not, insert default.
+ // The immutable borrow from .get() is released before .insert().
+ let mut extensions = &mut self
Review Comment:
`extensions` is already a `&mut` and is never reassigned, so `let mut`
should trigger `unused_mut`, which fails `cargo clippy ... -D warnings`. The
comment above it describes the borrow rather than why. Something like:
```rust
let extensions = &mut
self.config().get_or_insert_with(Default::default).options_mut().extensions;
// Keep a `SparkOptions` the caller already registered.
if extensions.get::<SparkOptions>().is_none() {
extensions.insert(SparkOptions::default());
}
```
##########
crates/sqllogictest/test_files/map/map_key_dedup_policy.slt:
##########
@@ -0,0 +1,16 @@
+# Spark map construction uses EXCEPTION by default.
+statement ok
+set spark.map_key_dedup_policy = 'LAST_WIN';
+
+query ?
+SELECT map_from_arrays(array(1, 2, 1), array('a', 'b', 'c'));
+----
+{1: c, 2: b}
Review Comment:
This repeats `map_from_arrays.slt:141-146`, which sets the same key and runs
the same query with the same result. The new coverage in this file is the
invalid value case. Could it keep only that? The two unit tests in `config.rs`
cover the same ground as the `.slt` (the `spark.` prefix and the invalid
value). CI runs `cargo test -p datafusion-spark-sqllogictest` but not `cargo
test -p datafusion-spark`, so Rust unit tests do not run in CI. So they could
go too.
--
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]
