feat: Add field-level descriptions to FieldSchema #1087
Conversation
- Add description field to Rust FieldSchema struct (Arc<str>) - Update Python FieldSchema to include description field - Extract Pydantic field descriptions during schema creation - Update JSON schema builder to include field descriptions - Add comprehensive tests for field description functionality Resolves cocoindex-io#1074
| } | ||
| // Set field description if available | ||
| if let Some(description) = &f.description { | ||
| self.set_description(&mut schema, description, field_path.prepend(&f.name)); |
There was a problem hiding this comment.
We have a problem of description overwrite here.
The schema returned by for_enriched_value_type() above may already have a description. e.g. self.description() is already called in many places based on the type (example).
I think we may concatenate both descriptions (field level + type level, separated by a newline).
Here's one way to do this:
-
Change
set_description()to "append" instead of "overwrite" if we already have a description. -
Instead of calling
set_description()here (which is after the type is already processed, so field description are placed after type description), we can pass the description as a argument down tofor_enriched_value_type()/for_value_type()/for_basic_value_type()/ etc. Then we can callset_description()with the passed-in description right after we createSchemaObject(which is beforeset_description()called based on type) - this will make sure the type description is added after field descriptions.
Please let me know if there's questions on this. Thanks!
There was a problem hiding this comment.
@georgeh0 ,I have added the changes concatenate field and type descriptions instead of overwriting, kindly review it
- Modified set_description method to append descriptions with newline separator - Fixed description overwrite issue where field-level descriptions would replace type-level descriptions - Added description: None to all FieldSchema initializations to satisfy new field requirement - Added test to verify description concatenation works correctly - All existing tests pass (129 passed, 0 failed) Resolves reviewer feedback about description overwrite in PR cocoindex-io#1087
- Modified set_description method to append descriptions with newline separator - Fixed description overwrite issue where field-level descriptions would replace type-level descriptions - Added description: None to all FieldSchema initializations to satisfy new field requirement - Added comprehensive test to verify description concatenation works correctly - All existing tests pass (129 passed, 0 failed) with no regressions detected - Implementation handles both extract_descriptions: true and false scenarios - Field-level descriptions now properly concatenate with type-level descriptions using newline separator - Resolves reviewer feedback about description overwrite in PR cocoindex-io#1087 - Changes maintain backward compatibility while fixing the core issue - Ready for review and testing by the maintainer team
belloibrahv
force-pushed
the
feature/field-descriptions
branch
from
25c9b5e to
d7db89c
Compare
|
Thanks! There're two checks failed. You may install pre-commit install
pre-commit run --all-files |
|
ops sorry closed by accident, this is a great PR, just a final touch on the format. Thanks @belloibrahv ! |
No problem, thanks for opening the PR back |
I have updated the PR with the required changes as requested, and I ensured that all failing checks were resolved locally. Please help me review when you have time. |
|
Thanks for the PR! Merging. |
3 participants
Arc<str>)Resolves #1074