fix: coerce uuid.UUID and os.PathLike to str for VARCHAR/JSON fields - #3757
fix: coerce uuid.UUID and os.PathLike to str for VARCHAR/JSON fields#3757pangwangshu wants to merge 3 commits into
Conversation
Insert of a UUID or Path value into a VARCHAR field, or nested inside a JSON/dynamic field, previously failed instead of auto-converting to a string as requested in milvus-io#2917. A prior attempt (milvus-io#3408) only patched dtype inference, which left the actual VARCHAR serialization boundary (convert_to_str_array/entity_to_str_arr) rejecting these values with ParamError; this covers dtype inference, the VARCHAR/TEXT/GEOMETRY string boundary (both scalar row-based and list column-based insert paths), and JSON/dynamic-field serialization, with end-to-end regression tests for each. Signed-off-by: Wangshu Pang <pangwangshu@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pangwangshu The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @pangwangshu! It looks like this is your first PR to milvus-io/pymilvus 🎉 |
|
Tick the box to add this pull request to the merge queue (same as
|
…cion Addresses review from @yhmo on milvus-io#3757: - preprocess_numpy_types (convert_to_json) only descended into dict/list, so a PathLike nested in a tuple was left unconverted even though orjson accepts tuples as JSON arrays. Tuples are now traversed the same way, staged as a list since JSON has no tuple/list distinction. - os.PathLike.__fspath__() may return bytes, not just str. Both coercion points (convert_to_json's PathLike branch and entity_helper's _coerce_str_like) used os.fspath(), which left bytes untouched and still unusable; switched to os.fsdecode() to normalize to text. Signed-off-by: Wangshu Pang <pangwangshu@gmail.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
shashvat-singham
left a comment
There was a problem hiding this comment.
Ran the branch. The VARCHAR side does what it says, and the tuple fix in the walker is a nice catch — (PurePosixPath("/a/b"),) really did stay unconverted before.
I went looking for a uuid.UUID branch in preprocess_numpy_types to match the os.PathLike one and couldn't find it, assumed that was an oversight, then tested it and found it works anyway — orjson serialises UUID natively, so the walker doesn't need to touch it. Worth noting in case a future reader has the same reaction I did.
That does leave one real gap, though: it works because orjson handles it, and orjson isn't always the serialiser. convert_to_json falls back to stdlib json past orjson's ~500-level recursion limit, and stdlib json has no native UUID support:
def nest(leaf, depth):
o = leaf
for _ in range(depth):
o = {"n": o}
return o
convert_to_json(nest(uuid.UUID(int=0), 10)) # b'{"n":...{"n":"00000000-..."}}'
convert_to_json(nest(uuid.UUID(int=0), 600)) # TypeError: Object of type UUID is not JSON serializable
convert_to_json(nest(PurePosixPath("/a/b"), 600)) # finePathLike survives the deep case precisely because this PR normalises it to str in the walker, before either serialiser sees it. UUID doesn't, so a JSON field nested past ~500 levels still raises the raw TypeError that GH-2917 is about.
To be clear this fails the same way on master, so it's not a regression from this PR — but since the PR's stated goal is making UUID and PathLike work in VARCHAR and JSON fields, and it fixes the deep case for one and not the other, it seems worth closing here. Adding
elif isinstance(current, uuid.UUID):
assign_to_parent(str(current))next to the PathLike branch would make both paths serialiser-independent and match the reasoning already in that comment block.
Admittedly 500-deep JSON is an unusual payload, so if you'd rather keep this PR tight I'm happy to send that as a follow-up — just flagging it while the context is fresh.
|
@shashvat-singham Good catch, thanks for digging into the fallback path. Added an explicit Verified with your exact repro — |
…enarios Signed-off-by: Wangshu Pang <pangwangshu@gmail.com>
07cfff7 to
259ee1a
Compare
Summary
Fixes #2917. Inserting a
uuid.UUIDorpathlib.Pathvalue into a VARCHAR field — or nesting one inside a JSON/dynamic field — failed instead of auto-converting to a string, as the issue requested.#3408 attempted this by patching dtype inference only. @XuanYang-cn correctly flagged that as incomplete (comment):
convert_to_str_array/entity_to_str_arrreject non-strvalues independently of dtype inference. This PR fixes all three points on that path:orm/types.py—infer_dtype_by_scalar_data/infer_dtype_bydatarecognizeUUID/PathLikeasVARCHAR(previouslyUNKNOWN;PathLikecould also crashinfer_dtype_bydatavia an uncaughtTypeError).client/entity_helper.py—convert_to_str_arraycoercesUUID→str/PathLike→os.fspath(), for both the row-insert (scalar) and column-insert (list) call paths.client/entity_helper.py—convert_to_jsonnow handlesPathLikenested in JSON/dynamic fields (previously a raworjsonTypeError).Test plan
convert_to_str_array(both call paths), andconvert_to_jsonpack_field_value_to_field_data/entity_to_field_data— the real insert path, not just the helpers in isolation (the gap that made fix: infer uuid.UUID and os.PathLike as VARCHAR in dtype inference #3408 look sufficient)make coverage(full unit suite) — passingmake integration-lite— passingmake lint— clean