fix: convert Arrow JSON when updating columns via merge/update path#7472
Conversation
xloya
force-pushed
the
upstream-pr/json-fragment-update
branch
from
7de1961 to
9939817
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
wjones127
left a comment
wjones127
left a comment
There was a problem hiding this comment.
Thanks for making a PR. I noticed one potential blocking issue when reviewing, but otherwise this looks like a nice fix.
| @@ -1134,8 +1135,25 @@ impl MergeInsertJob { | |||
| // will be updates. | |||
| let mut source_batches = Vec::with_capacity(batches.len() + 1); | |||
| source_batches.push(batches[0].clone()); // placeholder for source data | |||
There was a problem hiding this comment.
issue(blocking): You convert all the batches in this vector except the first one added here, which means the schema might be inconsistent between the batches. I think you need to convert this one as well.
There was a problem hiding this comment.
Good catch. I removed the needs_json_conversion flag entirely and now call convert_json_columns on every batch. Since convert_json_columns already no-ops (returns a clone) when there's nothing to convert, the separate schema scan was redundant — this fixes the schema-consistency bug and also handles nested Arrow JSON that the old top-level predicate missed. So the schema_has_json_fields helper is no longer needed.
| .fields() | ||
| .iter() | ||
| .any(|f| is_arrow_json_field(f) || has_json_fields(f)); |
There was a problem hiding this comment.
suggestion: should we just make a helper schema_has_json_fields(schema: &Schema) -> bool?
xloya
force-pushed
the
upstream-pr/json-fragment-update
branch
from
afea1ac to
281ba83
Compare
Convert every source batch (including the placeholder) with convert_json_columns, which is a no-op clone when there is nothing to convert. This fixes the placeholder batch skipping conversion and drops the now-unused flag/predicate helpers. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
xloya
force-pushed
the
upstream-pr/json-fragment-update
branch
from
281ba83 to
0ed54f4
Compare
| /// A wrapper around a `RecordBatchReader` that converts Arrow JSON columns | ||
| /// (Utf8/LargeUtf8 with `arrow.json` extension) to Lance JSON columns | ||
| /// (LargeBinary with `lance.json` extension / JSONB format). | ||
| /// | ||
| /// This is needed when user-provided data contains Arrow JSON fields but the | ||
| /// dataset stores them in Lance's JSONB binary format. | ||
| struct JsonConvertingReader { |
There was a problem hiding this comment.
suggestion: should we later consolidate this with SchemaAdapter?
lance/rust/lance/src/dataset/utils.rs
Lines 194 to 198 in 98efff9
Can do that as a follow up.
2 participants
Problem
Updating an Arrow JSON column through
fragment.update_columns/ the update merge path fails with a type-mismatch error.Root cause
The HashJoiner right-side stream did not convert Arrow JSON (Utf8) to Lance JSON (LargeBinary/JSONB) before joining.
Fix
Wrap the right-side stream in a
JsonConvertingReaderthat converts Arrow JSON fields to Lance JSON before the join.Test
Python
test_fragment_update_columns_with_json_columnplus dataset update tests.