andygrove commented on PR #4971:
URL:
https://github.com/apache/datafusion-comet/pull/4971#issuecomment-5441692140
> **Note on this review:** this was generated by an LLM (Claude Code) at my
request while I worked through a review backlog. I have not verified the
individual findings myself. Please treat everything below as suggestions to
evaluate rather than as authoritative review feedback, and push back on
anything that is wrong or already handled.
Matching Spark's first-occurrence semantics for duplicate keys is the right
fix, and keeping the full traversal so that malformed content after the match
still rejects the row is a detail that would have been easy to get wrong.
I think there is a case where the code does not do what the description
says, though.
**A first match that resolves to nothing falls through to the second
occurrence**
The description says:
> The lock is keyed on the *key match*, not on a successful subpath
resolution: if the first matching value does not contain the rest of the path,
the result is `null` and the second duplicate key is never consulted, matching
Spark.
But the code is:
```rust
while let Some(matched) = map.next_key_seed(KeySeed(name))? {
if matched && !found.matched {
let candidate = map.next_value_seed(PathSeed { segments:
&self.segments[1..], reject_direct_null: true })?;
if candidate.matched {
found = candidate;
}
} else {
map.next_value::<IgnoredAny>()?;
}
```
`found.matched` is `PathResult::matched`, which means "the path resolved to
a value", not "we saw the key". So when the first occurrence's subpath fails,
`found.matched` stays `false` and the loop tries the second occurrence. That is
the opposite of the description.
Two concrete cases I would expect to differ from Spark:
```sql
SELECT get_json_object('{"a":{"b":1},"a":{"c":2}}', '$.a.b')
```
Spark stops at the first `a`, finds no `b`, returns `NULL`. This code should
skip to the second `a`, find no `b` either, and also return `NULL`, so this one
happens to agree. But:
```sql
SELECT get_json_object('{"a":null,"a":2}', '$.a')
```
`reject_direct_null: true` makes the first occurrence return an unmatched
`PathResult`, so the loop consults the second and returns `2`. Spark stops at
the first `a` and returns `NULL`.
Could you check that second case against Spark? If it does differ, the guard
needs to be on whether the key was seen rather than on `found.matched`, which
is what the description already describes. Either way a test for
`{"a":null,"a":2}` would be worth adding, since it is the shape where the two
readings diverge.
**The change is bigger than the description says**
The description talks about `visit_map` locking in the first match. The diff
also introduces a `PathResult` type replacing `Option<Value>` throughout, adds
a `reject_direct_null` flag with its own semantics for
null-below-a-named-field, and rewrites `visit_seq` including the wildcard
branch. Those are separate behavioral changes and each deserves a line in the
description and its own test, especially `reject_direct_null`, which changes
what `$.a` returns for `{"a":null}`.
**A performance note on `visit_seq`**
The `Index` branch now calls `IgnoredAny.visit_seq(seq)` after finding its
element, so `$[0]` on a large array scans the whole array instead of stopping
at the first element. The comment explains why (a malformed element after the
match must reject), and that is correct Spark behavior. Worth measuring on a
wide array though, since `$[0]` over a 10k-element array goes from O(1) to O(n)
and that could be a visible regression for someone.
--
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]
