fix: don't quote column names in list_indices - #7503
Conversation
github-actions
Bot
added
A-python
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.
This looks thoughtfully done. If you get the remaining tests passing, I'd be open to merging this.
db34411 to
9ce957e
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughSchema field-path generation now supports minimal, round-trippable quoting. Python index descriptions and directory namespace index listings use the new format for indexed field names. ChangesField-path formatting and index integration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance-core/src/datatypes/schema.rs`:
- Around line 779-790: Add compilable doctest examples to the public APIs
Schema::field_path_minimal and format_field_path_minimal. Cover hyphenated
names, dotted segments, escaped backticks, and round-tripping the formatted
result through parse_field_path, using the existing public structs and methods
in the examples.
- Around line 2858-2915: Add coverage in test_field_path_minimal for a nested
field named child`x: assert field_path_minimal formats it with doubled-backtick
escaping as `child``x`, then assert parse_field_path round-trips the formatted
path to the original field components.
- Around line 1672-1680: Update the field-segment formatting logic in
format_field_path_minimal to treat empty strings like other segments requiring
quoting, producing a parseable quoted empty segment; alternatively, reject empty
field names during the existing schema validation path if empty segments are not
supported. Ensure formatted paths remain accepted by parse_field_path, including
["parent", ""].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 541788a3-9038-4b15-9579-5d0693ec410d
📒 Files selected for processing (3)
python/src/indices.rsrust/lance-core/src/datatypes/schema.rsrust/lance-namespace-impls/src/dir.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance-core/src/datatypes/schema.rs (1)
1669-1730: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared quoting/escaping helper for
format_field_pathandformat_field_path_minimal.The two functions are identical except for the
needs_quotingpredicate. Duplicated escape-and-join logic risks silently diverging (e.g., if backtick-escaping is fixed in one but not the other).As per coding guidelines, "Extract logic repeated in two or more places into a shared helper, but avoid helpers that only rename or forward existing calls."♻️ Proposed refactor
+fn format_field_path_with(fields: &[&str], needs_quoting: impl Fn(&str) -> bool) -> String { + fields + .iter() + .map(|field| { + if needs_quoting(field) { + let escaped = field.replace('`', "``"); + format!("`{}`", escaped) + } else { + field.to_string() + } + }) + .collect::<Vec<_>>() + .join(".") +} + pub fn format_field_path(fields: &[&str]) -> String { - fields - .iter() - .map(|field| { - // Quote if the field contains any non-identifier character - // (i.e., anything other than alphanumeric or underscore) - let needs_quoting = field.chars().any(|c| !c.is_alphanumeric() && c != '_'); - if needs_quoting { - // Escape backticks by doubling them (PostgreSQL style) - let escaped = field.replace('`', "``"); - format!("`{}`", escaped) - } else { - field.to_string() - } - }) - .collect::<Vec<_>>() - .join(".") + format_field_path_with(fields, |field| { + field.chars().any(|c| !c.is_alphanumeric() && c != '_') + }) } ... pub fn format_field_path_minimal(fields: &[&str]) -> String { - fields - .iter() - .map(|field| { - let needs_quoting = field.contains('.') || field.contains('`'); - if needs_quoting { - let escaped = field.replace('`', "``"); - format!("`{}`", escaped) - } else { - field.to_string() - } - }) - .collect::<Vec<_>>() - .join(".") + format_field_path_with(fields, |field| field.contains('.') || field.contains('`')) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-core/src/datatypes/schema.rs` around lines 1669 - 1730, Extract the shared segment escaping, quoting, and joining logic from format_field_path and format_field_path_minimal into a helper that accepts the needs_quoting predicate. Update both public functions to provide only their respective quoting conditions and reuse the helper, preserving their existing output and backtick escaping behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@rust/lance-core/src/datatypes/schema.rs`:
- Around line 1669-1730: Extract the shared segment escaping, quoting, and
joining logic from format_field_path and format_field_path_minimal into a helper
that accepts the needs_quoting predicate. Update both public functions to
provide only their respective quoting conditions and reuse the helper,
preserving their existing output and backtick escaping behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 54ce8c0a-2b3b-41ba-b4e8-adc5ca9c9a0b
📒 Files selected for processing (2)
rust/lance-core/src/datatypes.rsrust/lance-core/src/datatypes/schema.rs
|
@wjones127 ok, thanks! yeah I go back and forth on if it's even worth it but I guess so. so I added some coverage and ran the tests locally, seemed to pass; I guess you have to approve the rest of the tests? |
0f79dae to
ff1a242
Compare
|
@wjones127 ok, I think I fixed the test that was breaking; ready for another test run (do you already get notified for this? I'll stop @-ing you if so) |
2 participants
The bug: have a column with a hyphen, like "col-3". Build an index on it. Call list_indices, get a value that's double-quoted, like:
The fix: use
field_path_minimalto only quote the value if it has a dot or a backtick in it (and therefore needs it - otherwise it's impossible to tell ifmy_loc.coords.latis{my_loc: {coords: {lat: ...}}}or{my_loc: {"coords.lat": }}I think this change is purely cosmetic, because calls like
drop_columnandupdate_field_metadataseem to work even when passedSo I am fine with either merging this or not (and prettifying it on the UI side e.g. if I display these results)