fix(index): align scalar index field type support#7966
Conversation
📝 WalkthroughWalkthroughScalar-index validation now accepts binary, large binary, decimal, and duration-related types. Decimal extrema calculation and DataFusion scalar coercion support were added, with expanded Python and Rust test coverage. ChangesScalar index type support
Estimated code review effort: 3 (Moderate) | ~20 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: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
python/python/lance/dataset.py-3248-3252 (1)
3248-3252: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the rejection error match the expanded validation.
Unsupported columns still receive a split, stale
TypeErrorthat omits binary/decimal support and the actualfield_type. Raise one message that includescolumnandfield_type.Proposed fix
- raise TypeError( - f"BTREE/BITMAP/ZONEMAP index column {column} must be int", - ", float, bool, str, large_str, fixed-size-binary, or temporal", - ) + raise TypeError( + "BTREE/BITMAP/ZONEMAP index column " + f"{column!r} must be boolean, integer, floating, string, " + "binary, fixed-size-binary, decimal, or temporal; " + f"got field_type={field_type}" + )As per coding guidelines, “Include full error context, including variable names, values, sizes, and types.”
🤖 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 `@python/python/lance/dataset.py` around lines 3248 - 3252, Update the rejection TypeError in the validation surrounding the field-type checks to use one message covering all unsupported types, including the column name and actual field_type value and type. Remove the stale split error wording while preserving the expanded validation conditions.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.
Inline comments:
In `@rust/lance-datafusion/src/expr.rs`:
- Around line 451-469: The public safe_coerce_scalar function needs Rustdoc
documenting its LargeBinary, decimal, and duration coercion behavior. Add
examples using its actual signature, with links to ScalarValue, DataType, and
the relevant casting method, and keep the examples synchronized with supported
inputs and return behavior.
---
Other comments:
In `@python/python/lance/dataset.py`:
- Around line 3248-3252: Update the rejection TypeError in the validation
surrounding the field-type checks to use one message covering all unsupported
types, including the column name and actual field_type value and type. Remove
the stale split error wording while preserving the expanded validation
conditions.
🪄 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: QUIET
Plan: Pro Plus
Run ID: a4b45219-5565-4eb2-a8a3-896ca03dcb95
📒 Files selected for processing (4)
python/python/lance/dataset.pypython/python/tests/test_scalar_index.pyrust/arrow-stats/src/lib.rsrust/lance-datafusion/src/expr.rs
8dd1a15 to
7f0575d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/arrow-stats/src/lib.rs (1)
753-755: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse direct
Vecconstruction forDecimal256Array.
Decimal128Array::from(Vec)is already used here, and Arrow Rust 58 supportsDecimal256Array::from(Vec<i256>), so this can match the existing primitive-array style:Decimal256Array::from(vec![300_i128, 100, 200].into_iter().map(Into::into))🤖 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/arrow-stats/src/lib.rs` around lines 753 - 755, Update the Decimal256Array construction in the affected test or example to use Decimal256Array::from with a Vec<i128> iterator, matching the existing Decimal128Array construction style, and remove the from_iter_values call while preserving the values and conversion to i256.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.
Nitpick comments:
In `@rust/arrow-stats/src/lib.rs`:
- Around line 753-755: Update the Decimal256Array construction in the affected
test or example to use Decimal256Array::from with a Vec<i128> iterator, matching
the existing Decimal128Array construction style, and remove the from_iter_values
call while preserving the values and conversion to i256.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: c05718f6-d085-4bc6-8f41-34fbb46f3c65
📒 Files selected for processing (4)
python/python/lance/dataset.pypython/python/tests/test_scalar_index.pyrust/arrow-stats/src/lib.rsrust/lance-datafusion/src/expr.rs
|
Could please @Xuanwo help review this? thanks! |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
7f0575d to
daa8c63
Compare
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
python/python/tests/test_scalar_index.py-758-758 (1)
758-758: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd scalar-column coverage for RTREE via the uncommitted index route.
create_scalar_index(..., index_type="RTREE")is accepted, but RTREE is routed throughcreate_index_uncommitted(...), andtest_scalar_index.pyonly covers BTREE/BITMAP/ZONEMAP here. Add a valid scalar-column RTREE test calling the segmented uncommitted path, like the BTREE/BITMAP/ZONEMAP cases.🤖 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 `@python/python/tests/test_scalar_index.py` at line 758, Extend the index_type coverage loop in test_scalar_index.py to include RTREE and add the corresponding valid scalar-column case through create_index_uncommitted, matching the existing BTREE, BITMAP, and ZONEMAP test flow. Ensure the RTREE case invokes the segmented uncommitted path and preserves the current assertions.Source: Coding guidelines
🧹 Nitpick comments (1)
rust/arrow-stats/src/lib.rs (1)
753-755: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConstruct the primitive array with
From<Vec<i256>>.Use
Decimal256Array::from(vec![300_i64.into(), 100_i64.into(), 200_i64.into()])to avoid the iteratorfrom_iter_valuespath.🤖 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/arrow-stats/src/lib.rs` around lines 753 - 755, Replace the Decimal256Array construction using from_iter_values in the affected test with the From<Vec<i256>> form, explicitly building a vector of 300_i64, 100_i64, and 200_i64 converted into i256 values. Preserve the existing array contents and ordering.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.
Other comments:
In `@python/python/tests/test_scalar_index.py`:
- Line 758: Extend the index_type coverage loop in test_scalar_index.py to
include RTREE and add the corresponding valid scalar-column case through
create_index_uncommitted, matching the existing BTREE, BITMAP, and ZONEMAP test
flow. Ensure the RTREE case invokes the segmented uncommitted path and preserves
the current assertions.
---
Nitpick comments:
In `@rust/arrow-stats/src/lib.rs`:
- Around line 753-755: Replace the Decimal256Array construction using
from_iter_values in the affected test with the From<Vec<i256>> form, explicitly
building a vector of 300_i64, 100_i64, and 200_i64 converted into i256 values.
Preserve the existing array contents and ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: febe9cc3-9682-43b8-86e2-366ddaf6d8fc
📒 Files selected for processing (4)
python/python/lance/dataset.pypython/python/tests/test_scalar_index.pyrust/arrow-stats/src/lib.rsrust/lance-datafusion/src/expr.rs
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
Legacy Decimal ZoneMap indexes can return false negatives after this change.
Before this PR, Rust and Python IndexConfig could create a Decimal ZoneMap even though arrow-stats returned no extrema for Decimal, so non-null zones were persisted with typed-null min/max. This PR makes typed Decimal predicates reach ScalarIndexQuery, while evaluate_zone_against_query treats a missing finite minimum as no match and ZONEMAP_INDEX_VERSION remains 0.
I reproduced this on the current head with a pre-PR index containing [1.00, 2.00, 3.00]: value = arrow_cast(2.00, 'Decimal128(10, 2)') returns 0 rows through ScalarIndexQuery, while the same filter with scalar indexes disabled returns 1 row. The old writer behavior is also present in released v8.0.0.
Could we conservatively include zones whose bounds are null but whose null_count shows non-null values, or otherwise reject/rebuild legacy Decimal v0 indexes? A historical fixture comparing indexed and flat reads for equality/range would cover this compatibility path.
2 participants
Summary
LargeBinary,Decimal128,Decimal256, and duration fields.Root cause
Python validation was narrower than the Core index capabilities. In addition, some accepted types could create an index but their typed query literals were not coerced by the planner, causing filters to fall back to
LanceRead.Non-goals
Decimal32andDecimal64are not included because Lance does not yet support them as dataset types. Their support is tracked separately in #5174.Testing
cargo test -p lance-datafusion expr::testscargo test -p lance-arrow-stats test_rstest_primitivesuv run --python 3.11 pytest python/tests/test_scalar_index.py::test_scalar_index_typesuv run make lintcargo fmt --all --checkcargo clippy --all --tests --benches -- -D warnings