Reviewing files that changed from the base of the PR and between 6fd3bba and d0fc25b.

• docs/grpc/docs.md (1 hunks)
• docs/redoc/master/openapi.json (2 hunks)
• lib/api/src/grpc/conversions.rs (4 hunks)
• lib/api/src/grpc/proto/points.proto (1 hunks)
• lib/api/src/grpc/qdrant.rs (1 hunks)
• lib/api/src/grpc/validate.rs (2 hunks)
• lib/segment/src/index/field_index/field_index_base.rs (18 hunks)
• lib/segment/src/index/field_index/index_selector.rs (7 hunks)
• lib/segment/src/index/field_index/mod.rs (2 hunks)
• lib/segment/src/index/field_index/null_index/mmap_null_index.rs (1 hunks)
• lib/segment/src/index/field_index/null_index/mod.rs (1 hunks)
• lib/segment/src/index/query_estimator.rs (2 hunks)
• lib/segment/src/index/query_optimization/condition_converter.rs (9 hunks)
• lib/segment/src/index/query_optimization/condition_converter/match_converter.rs (6 hunks)
• lib/segment/src/index/query_optimization/rescore_formula/value_retriever.rs (1 hunks)
• lib/segment/src/index/struct_payload_index.rs (4 hunks)
• lib/segment/src/payload_storage/condition_checker.rs (2 hunks)
• lib/segment/src/problems/unindexed_field.rs (2 hunks)
• lib/segment/src/segment_constructor/segment_builder.rs (1 hunks)
• lib/segment/src/types.rs (10 hunks)
• lib/segment/src/vector_storage/dense/dynamic_mmap_flags.rs (3 hunks)
• lib/segment/tests/integration/payload_index_test.rs (1 hunks)

• lib/segment/src/index/field_index/null_index/mod.rs

• GitHub Check: test-snapshot-operations-s3-minio
• GitHub Check: test-shard-snapshot-api-s3-minio
• GitHub Check: test-low-resources
• GitHub Check: test-consistency
• GitHub Check: Basic TLS/HTTPS tests
• GitHub Check: test (macos-latest)
• GitHub Check: test-consensus-compose
• GitHub Check: test
• GitHub Check: test (windows-latest)
• GitHub Check: test-consensus
• GitHub Check: test
• GitHub Check: test (ubuntu-latest)
• GitHub Check: test

lib/api/src/grpc/qdrant.rs (1)

6281-6286: LGTM - Properly implemented new field condition checks.

The addition of is_empty and is_null optional boolean fields to the FieldCondition message is implemented correctly with appropriate protobuf tags, types, and documentation. This enhancement aligns well with the PR objective of improving empty/null field handling in field queries.

From the PR objectives and AI summary, it appears these fields are consistently added across the codebase to support checking for empty or null field values, which is a useful enhancement to the filtering capabilities.

lib/segment/src/segment_constructor/segment_builder.rs (1)

244-244: LGTM! Correctly handles the new NullIndex case.

The match arm FieldIndex::NullIndex(_) => {} properly follows the existing pattern for index types that don't contribute to ordering values. This addition is necessary to support the new null indexing capability and is implemented consistently with other similar index types.

lib/api/src/grpc/proto/points.proto (1)

1048-1049: Good addition of empty/null field checks to enhance filtering capabilities.

These new optional boolean fields enable explicit filtering on empty and null field states, which is a valuable feature for queries. The implementation follows the protobuf best practice of using optional fields with descriptive comments.

lib/segment/src/index/query_optimization/rescore_formula/value_retriever.rs (1)

178-178: LGTM! Properly handles the new NullIndex case.

The handler correctly returns None for NullIndex, which is the expected behavior since null values don't have retrievable content. The comment also provides valuable context that this index type is expected to be used alongside other indices for the same field.

lib/segment/src/problems/unindexed_field.rs (2)

220-222: LGTM! Successfully extracted the new fields from the FieldCondition struct.

The addition of is_empty and is_null to the destructuring pattern correctly extracts these new fields from the FieldCondition struct for further processing.

---

259-262: LGTM! Enhanced condition to support new field checks.

The updated condition now correctly considers the new is_empty and is_null fields when determining if indexes should be inferred. This ensures that queries using these new field checks will have the opportunity to benefit from appropriate indexing.

lib/api/src/grpc/conversions.rs (4)

1376-1377: Proper integration of new field conditions.

The addition of is_empty and is_null fields to the FieldCondition struct is a good enhancement, allowing for more expressive query conditions to check for empty or null values.

---

1400-1401: Consistent field handling in type conversion.

The proper handling of is_empty and is_null fields in the try_from implementation ensures field conditions are correctly transferred from gRPC representations to internal types.

---

1416-1417: Appropriate field inclusion in struct destructuring.

The inclusion of the new fields in struct destructuring ensures they're properly captured during conversion.

---

1435-1436: Bidirectional conversion support for new fields.

The proper mapping of is_empty and is_null fields in the reverse conversion direction ensures consistency in data transformation between internal and API representations.

lib/segment/src/index/field_index/mod.rs (2)

5-5: Simplified type imports matching refactored condition types.

The import statement has been updated to remove IsEmptyCondition and IsNullCondition types, which aligns with the refactoring to integrate these condition types into the FieldCondition struct.

---

27-32:

✅ Verification successful

Simplified PrimaryCondition enum with integrated condition handling.

The PrimaryCondition enum has been simplified by removing dedicated variants for IsEmpty and IsNull conditions. This is a good refactoring that reduces duplication and simplifies the condition handling by integrating these checks into the FieldCondition struct.

---

🏁 Script executed:

#!/bin/bash
# Find usages of PrimaryCondition to confirm the refactoring is applied consistently
rg "PrimaryCondition" --type rust

Length of output: 7045

---

PrimaryCondition Refactor Verified and Approved

The updated PrimaryCondition enum shows a consistent application of refactoring by removing the dedicated IsEmpty and IsNull variants in favor of using FieldCondition for condition handling. The grep output confirms that all usages across tests and index implementations now rely on the Condition variant (as well as the Ids and HasVector variants), ensuring that the refactoring is applied uniformly without impacting the overall functionality.

• The simplified enum in lib/segment/src/index/field_index/mod.rs (lines 27-32) is correctly reflected.
• All references in integration tests and various index modules consistently use the PrimaryCondition::Condition variant.
• The integration of condition handling into FieldCondition reduces duplication and streamlines the logic.

lib/api/src/grpc/validate.rs (2)

209-211: Thorough field extraction in validation.

The destructuring of the FieldCondition struct has been updated to include the new is_empty and is_null fields, ensuring they are properly accessible for validation.

---

219-221: Complete validation of all field condition types.

The all_fields_none check has been extended to include the new fields, ensuring that validation correctly identifies when no field conditions are specified. This maintains the requirement that at least one field condition must be specified.

lib/segment/src/vector_storage/dense/dynamic_mmap_flags.rs (1)

148-150: Enhanced documentation for set_len behavior.

The addition of documentation clarifying the capacity behavior is helpful. It clearly communicates that capacity can be up to twice the current length, which is important for understanding performance implications.

lib/segment/src/index/field_index/field_index_base.rs (12)

22-22: Appropriate import for the new null index types.

This import correctly brings in the necessary types for supporting null indexing functionality.

---

124-124: Good addition of NullIndex variant.

Adding the NullIndex variant to the FieldIndex enum is a clean way to extend the system's capability to handle null values explicitly.

---

140-140: Debug implementation for NullIndex properly added.

The Debug implementation correctly follows the established pattern for other index types.

---

181-182: Consistent implementation for special_check_condition.

The implementation returns None for NullIndex, following the same pattern as most other index types.

---

197-198: Correct delegation to payload_field_index in get_payload_field_index.

The implementation correctly returns the payload_field_index for the NullIndex variant.

---

213-214: Comprehensive implementation across all FieldIndex methods.

All necessary methods for handling the NullIndex variant are properly implemented, including load, cleanup, add_point, remove_point, get_telemetry_data, values_count, and values_is_empty. Each follows the established pattern for other index types.

Also applies to: 229-230, 297-299, 315-316, 331-332, 347-348, 363-364

---

378-380: Correct exclusion of NullIndex from numeric representation.

The NullIndex variant is correctly excluded from having a numeric representation, similar to FullTextIndex and other non-numeric indices.

---

394-396: Appropriate exclusion of NullIndex from facet index representation.

The NullIndex is correctly excluded from having a facet index representation, following the pattern for other index types that don't support this functionality.

---

444-444: Builder pattern correctly extended with NullIndex variant.

The FieldIndexBuilder enum is properly extended with the NullIndex variant to support the creation of null indices.

---

470-471: Complete implementation of builder methods for NullIndex.

All necessary methods (init, add_point, finalize) for the NullIndex builder are properly implemented, maintaining consistency with other index types.

Also applies to: 496-497, 520-521

---

22-521:

✅ Verification successful

Verify integration with the rest of the system.

The implementation looks solid, but it's important to ensure that the system correctly uses this new index type for null value queries. Make sure that:

1. The null index is created when needed
2. Queries properly utilize the null index when filtering for null values
3. The indexing mechanism correctly identifies and indexes null values

---

🏁 Script executed:

#!/bin/bash
# Description: Verify how the NullIndex is used in the codebase

# Search for where NullIndex is created and built
echo "Checking for NullIndex creation:"
rg -A 5 "NullIndex|null_index" --type rust

# Check if there's code that decides when to create a NullIndex
echo -e "\nChecking index selection logic:"
rg -A 10 "index.*selector|select.*index" --type rust

# Check if null fields are properly handled in queries
echo -e "\nChecking query handling of null fields:"
rg -A 5 "is_null|null.*field|field.*null" --type rust

Length of output: 73216

---

Review Approved: NullIndex Integration Verified

The integration of the new NullIndex has been thoroughly verified across the codebase. The verification output confirms that:

• The NullIndex is created when needed (e.g. in the index selector using MmapNullIndex::builder).
• Query logic correctly handles null fields by explicitly checking for FieldIndex::NullIndex in condition converters and payload estimations.
• The indexing mechanism properly identifies and indexes null values (via values_is_null and related functions).

No further action is required.

---

22-22:

❓ Verification inconclusive

Verify correctness of the null index implementation.

Let's make sure the imported MmapNullIndex and MmapNullIndexBuilder are properly implemented.

---

🏁 Script executed:

#!/bin/bash
# Description: Examine the implementation of null index

# Check the MmapNullIndex implementation
echo "Examining MmapNullIndex implementation:"
fd -e rs "null_index.*mmap_null_index\.rs" | xargs cat

# Check if there are tests for the null index functionality
echo -e "\nChecking for null index tests:"
rg -A 5 "test.*null_index|null_index.*test" --type rust

Length of output: 280

---

Action: Manually verify the null index implementation.

Our automated scans did not return any results for the definitions of MmapNullIndex and MmapNullIndexBuilder. Please ensure that these components are implemented correctly in the codebase and that the import path in lib/segment/src/index/field_index/field_index_base.rs is accurate. If the files exist under a different name or location, update the import accordingly.

---

---

🏁 Script executed:

#!/bin/bash
# Search for definitions of MmapNullIndex and MmapNullIndexBuilder in the repository

echo "Searching for 'struct MmapNullIndex' definition:"
rg -n "struct\s\+MmapNullIndex" --type rust

echo -e "\nSearching for 'struct MmapNullIndexBuilder' definition:"
rg -n "struct\s\+MmapNullIndexBuilder" --type rust

echo -e "\nSearching for file names containing 'mmap_null_index':"
fd -i 'mmap_null_index'

Length of output: 563

---

Action: Please manually verify the implementation.

Our automated checks confirm that the file exists at
lib/segment/src/index/field_index/null_index/mmap_null_index.rs,
but we couldn’t locate explicit definitions for MmapNullIndex or MmapNullIndexBuilder using automated searches. It’s recommended to manually inspect the file to ensure that these types (or their equivalents) are properly defined and that their behavior matches the intended functionality. Also, verify that corresponding tests exist to validate their correctness.

• File to check: lib/segment/src/index/field_index/null_index/mmap_null_index.rs
• Focus Areas:
  • Ensure that MmapNullIndex and MmapNullIndexBuilder are either explicitly defined or correctly re-exported.
  • Confirm that public interfaces and related tests cover the null index functionality.

docs/redoc/master/openapi.json (3)

8358-8358: Documentation improvement for geo_bounding_box description

This change correctly updates the terminology from "geo location" to "geolocation" for better consistency in the documentation.

---

8401-8405: Addition of is_empty field checking capability

This new field provides an alternative syntax for checking if a specified field is empty, enhancing the filtering capabilities. The description is clear and the field is correctly marked as nullable since it's optional.

As per a previous comment by agourlay, integration tests should be added for this alternative syntax to ensure it works as expected. Please verify that integration tests have been added for this feature.

---

8406-8410: Addition of is_null field checking capability

This new field adds the ability to check if a specified field is null using an alternative syntax. Similar to is_empty, this enhances the filtering capabilities and is well-documented.

As with the is_empty field, please ensure that integration tests are added for this alternative syntax as recommended by agourlay's previous comment to verify correct behavior in various scenarios.

lib/segment/src/index/query_estimator.rs (3)

286-287: Nice addition of the new fields to support empty and null checks.

These fields will allow for more flexible filtering capabilities in accordance with the PR objective of implementing IsEmpty/IsNull indexing.

---

331-333: Good refactoring of the IsEmpty condition implementation.

The change properly encapsulates the condition within a generic PrimaryCondition::Condition containing a boxed FieldCondition::new_is_empty(). This approach aligns with the PR's objective of creating a more consistent structure for field conditions.

---

339-341: Good refactoring of the IsNull condition implementation.

Similar to the IsEmpty change, this properly encapsulates the condition using PrimaryCondition::Condition with a boxed FieldCondition::new_is_null(), which maintains consistency in how conditions are represented.

lib/segment/src/payload_storage/condition_checker.rs (3)

46-47: Cleanly integrated the new fields in the struct destructuring.

The new is_empty and is_null fields are properly added to the destructuring pattern, ensuring they're available for use in the function body.

---

71-78: Well-implemented matching logic for the is_empty condition.

The implementation correctly handles different value types:

• For Value::Null, it returns the is_empty value directly
• For scalar types (Bool, Number, String, Object), it returns the negation of is_empty
• For arrays, it checks if the array's emptiness matches the requested is_empty value

This aligns with the documented behavior where "this condition will match all records where the field reports either does not exist, or has null or [] value."

---

79-86: Well-implemented matching logic for the is_null condition.

The implementation correctly handles different value types:

• For Value::Null, it returns the is_null value directly
• For scalar types (Bool, Number, String, Object), it returns the negation of is_null
• For arrays, it checks if any element is null and compares that to the is_null value

This provides a logical extension to the null checking capabilities.

lib/segment/src/index/query_optimization/condition_converter/match_converter.rs (4)

56-56: Complete coverage of NullIndex in match_value_checker.

The additions properly integrate the new NullIndex type into the match patterns, ensuring it's handled appropriately for all value variant types (Bool, Integer, String). This ensures that value matching will work correctly with null indexes.

Also applies to: 66-66, 74-75

---

133-133: Complete coverage of NullIndex in match_any_checker.

The additions properly integrate the new NullIndex type into the match patterns for both AnyVariants::Integers and AnyVariants::Strings, ensuring that "any" matching will work correctly with null indexes.

Also applies to: 141-142

---

196-196: Complete coverage of NullIndex in match_except_checker.

The additions properly integrate the new NullIndex type into the match patterns for both AnyVariants::Strings and AnyVariants::Integers, ensuring that "except" matching will work correctly with null indexes.

Also applies to: 205-206

---

235-236: Complete coverage of NullIndex in match_text_checker.

The additions properly integrate both UuidMapIndex and NullIndex types into the match patterns, ensuring that text matching will work correctly with these index types.

lib/segment/src/index/field_index/index_selector.rs (2)

23-23: Use of MmapNullIndex import is valid
This import is necessary for the newly introduced null index functionality.

---

358-360: null_dir extension is consistent
This function correctly follows the pattern used in other directory helpers such as map_dir and numeric_dir.

lib/segment/src/index/struct_payload_index.rs (3)

40-41: Expanded imports for payload schema
The inclusion of PayloadContainer and similar items at these lines aligns with new usage in the cardinality and condition code paths.

---

253-257: IsEmpty condition bridging to FieldCondition
This snippet transforms an IsEmptyCondition into a full FieldCondition for estimation. The pattern is consistent with how other conditions are handled.

---

260-264: IsNull condition bridging to FieldCondition
Similarly, IsNullCondition is converted into a FieldCondition and carries through the same evaluation flow. This fosters uniformity in condition handling.

lib/segment/src/index/query_optimization/condition_converter.rs (6)

9-9: Added MmapNullIndex import
Required for indexing null/empty values. Import is correct.

---

85-107: Custom logic for Condition::IsNull
This logic cleanly attempts a null-index-based check first, then gracefully defaults to payload inspection. The approach is consistent and reduces code duplication in repeated checks.

---

269-270: Added NullIndex to the match arms returning None
For geo or numeric checks, NullIndex is correctly bypassed to the fallback because it doesn’t handle these operations. This explicit coverage prevents silent mismatches.

Also applies to: 291-292, 312-313, 345-346, 369-370

---

373-389: get_is_empty_indexes provides a primary/fallback strategy
Splitting out null-index-based checks from any other available index is clear. The function nicely returns a two-part tuple to guide the next step.

---

391-413: get_fallback_is_empty_checker merges index checks with payload
Combining index results with a final payload reference is pragmatic to ensure consistency. This approach is particularly valuable if payload data can differ from partially built indexes.

---

437-453: get_is_null_checker parallels get_is_empty_checker
The logic is nearly identical yet adapted for null checks. Implementation is consistent. Similar expansions could apply if other indexes begin tracking null states differently.

lib/segment/src/index/field_index/null_index/mmap_null_index.rs (4)

41-52: Ensure robust directory creation and error handling.
When creating new directories, consider race conditions under concurrent usage. For instance, if another thread/process is creating the same directory at the same time, handle AlreadyExists gracefully rather than treating it as a terminal error.

---

95-130: Validate correct handling for multi-valued payloads.
Within add_point, if payload contains multiple arrays or nested data, ensure has_values is set accurately (e.g., for non-empty arrays). Otherwise, legitimate values might be ignored.

---

112-116: Revisit array emptiness logic and consider adding a dedicated test.
A previous comment suggested marking has_values as true for non-empty arrays. If the intention is to treat non-empty arrays as 'has values,' make sure to update has_values. Also, adding a targeted unit test would help validate various array payload cases.

---

307-316: Check concurrency safety for builder usage.
While the builder approach is straightforward, if multiple threads use the same builder, there may be data races on the internal slices. Document or enforce single-thread usage if concurrency is not intended.

lib/segment/src/types.rs (2)

2122-2125: Clarify semantics of is_empty vs. is_null.
Having both options can lead to overlapping semantics (e.g., an array could be empty but not null). Provide guidance or checks to avoid confusion, especially if user expects “empty” to include or exclude null.

---

2319-2332: Streamline usage of From<JsonPath> for null/empty checks.
These conversions are handy, but verify that they work as intended with nested JSON paths. For instance, “key.subkey” for null vs. empty might behave differently if key or subkey is missing.

Reviewing files that changed from the base of the PR and between d0fc25b and f22e1a2.

• lib/segment/src/index/field_index/null_index/mmap_null_index.rs (1 hunks)
• lib/segment/src/payload_storage/condition_checker.rs (4 hunks)
• tests/openapi/test_basic_retrieve_api.py (5 hunks)

• GitHub Check: Basic TLS/HTTPS tests
• GitHub Check: test-snapshot-operations-s3-minio
• GitHub Check: test-shard-snapshot-api-s3-minio
• GitHub Check: test-low-resources
• GitHub Check: test-consistency
• GitHub Check: test-consensus-compose
• GitHub Check: test (macos-latest)
• GitHub Check: test
• GitHub Check: test (windows-latest)
• GitHub Check: test (ubuntu-latest)
• GitHub Check: test-consensus
• GitHub Check: test
• GitHub Check: test

lib/segment/src/payload_storage/condition_checker.rs (4)

46-48: Well-structured implementation of these new fields

The addition of is_empty and is_null fields to the FieldCondition struct is clean and follows the existing pattern of optional condition fields.

---

71-78: Comprehensive implementation of the is_empty condition checking

The implementation correctly handles different types of JSON values according to your documentation. For arrays, it checks if any non-null elements exist, which aligns with the expected behavior where an array is considered empty if it contains only null values or is an empty array.

According to the past discussion, this matches your documentation: "This condition will match all records where the field reports either does not exist, or has null or [] value."

---

79-86: Logical implementation of the is_null condition checking

The implementation for is_null properly handles all JSON value types. For arrays, it checks if any element in the array is null, which aligns with the expected behavior where an array is considered to have null if at least one element is null.

---

323-419: Comprehensive test coverage for the new condition checks

The test suite thoroughly validates the behavior of the is_empty and is_null conditions against various JSON value types, including edge cases like empty arrays, arrays with null values, and arrays with a mix of null and non-null values.

tests/openapi/test_basic_retrieve_api.py (3)

183-205: Additional alternative is_empty syntax test improves coverage

This test complements the existing is_empty condition test by verifying that the newer syntax (using "key": "city", "is_empty": true) produces the same results as the older syntax (using "is_empty": {"key": "city"}). This ensures backward compatibility while introducing the more intuitive syntax.

---

236-259: Comprehensive test for is_null condition with consistent result verification

Good test implementation that verifies both syntaxes for the is_null condition produce identical results. The assertion of identical IDs between the two approaches ensures consistency in your API.

---

294-317: Well-structured test for is_null negation

The test validates that the must_not/is_null combination is equivalent to using is_null: false directly. This is important as it demonstrates both approaches produce consistent results and helps users understand the relationship between these query patterns.

lib/segment/src/index/field_index/null_index/mmap_null_index.rs (3)

18-28: Well-designed index structure for empty/null conditions

The MmapNullIndex structure is well-designed with a clear separation of concerns between tracking values and nulls. The documentation clearly explains the purpose and expected usage of this specialized index.

---

207-250: Clear and efficient filter implementation

The filter implementation correctly handles the different scenarios for is_empty and is_null conditions. The implementation is efficient by leveraging the precomputed flags and the comments clearly explain the reasoning behind each branch.

---

334-428: Comprehensive test coverage for null index

The test for the null index is thorough and covers all important cases:

1. Testing various combinations of values (null, array with null, empty, non-null)
2. Validating both positive and negative conditions
3. Verifying cardinality estimation

The test structure makes it easy to understand what's being tested and the expected outcomes.

Reviewing files that changed from the base of the PR and between f22e1a2 and 82fb933.

• lib/segment/src/common/utils.rs (1 hunks)
• lib/segment/src/payload_storage/condition_checker.rs (5 hunks)
• lib/segment/src/payload_storage/query_checker.rs (1 hunks)
• lib/segment/src/types.rs (11 hunks)

• lib/segment/src/types.rs

• GitHub Check: test-snapshot-operations-s3-minio
• GitHub Check: test-shard-snapshot-api-s3-minio
• GitHub Check: test-low-resources
• GitHub Check: test-consistency
• GitHub Check: Basic TLS/HTTPS tests
• GitHub Check: test (macos-latest)
• GitHub Check: test-consensus
• GitHub Check: test (windows-latest)
• GitHub Check: test-consensus-compose
• GitHub Check: test
• GitHub Check: test (ubuntu-latest)
• GitHub Check: test
• GitHub Check: test

lib/segment/src/payload_storage/query_checker.rs (1)

193-195: Verify the early return behavior for empty fields.

When field_values is empty, the code returns immediately without applying potential field-index logic. If this behavior is intentional, no changes are required; otherwise, consider integrating a fallback check for indexes to ensure consistent results.

lib/segment/src/common/utils.rs (1)

21-21: Definition of empty arrays now includes all-null arrays.

Returning true for arrays whose elements are all null aligns with the documented requirement that null values are treated as non-data. This is consistent with the new semantics of “empty” in the codebase.

lib/segment/src/payload_storage/condition_checker.rs (6)

33-38: Default check_empty implementation may mask specialized behaviors.

By always returning false, custom types that implement ValueChecker won't detect emptiness unless overridden. If this is the intended fallback, it’s correct. Otherwise, you might consider documenting that implementers must override check_empty where appropriate.

---

51-59: Partial null checks in arrays.

array.iter().any(|x| x.is_null()) == is_null means an array with at least one null satisfies is_null = true. This matches the stated requirement that “null is not a value” and is consistent with your documentation.

---

73-75: Adding is_empty and is_null checks to FieldCondition.

Including these fields in the destructured logic and OR condition ensures that empty/null checks seamlessly integrate with existing match, range, and geo checks. The approach is sound.

Also applies to: 98-99

---

103-127: Order of condition checks can skip multi-condition scenarios.

When both values_count and is_empty/is_null are present, the function handles values_count first. If you intend to prioritize values_count over empty/null checks, this ordering is correct. Otherwise, consider making the logic more explicit about combining conditions.

---

129-148: check_empty favoring is_empty over is_null.

The code returns is_empty first if present; if not, it returns the inverse of is_null. This is logical but means specifying both fields simultaneously might lead to unexpected outcomes for users unless well-documented.

---

377-473: Extensive test coverage for null and empty conditions.

Good job adding thorough tests, including edge cases like [null] and partial-null arrays, ensuring robust verification of the new logic.

Reviewing files that changed from the base of the PR and between 82fb933 and 87e1c30.

• lib/segment/src/index/field_index/null_index/mmap_null_index.rs (1 hunks)

• GitHub Check: test-snapshot-operations-s3-minio
• GitHub Check: test-shard-snapshot-api-s3-minio
• GitHub Check: test-low-resources
• GitHub Check: test-consensus-compose
• GitHub Check: test-consensus
• GitHub Check: Basic TLS/HTTPS tests
• GitHub Check: test (macos-latest)
• GitHub Check: test-consistency
• GitHub Check: test (windows-latest)
• GitHub Check: test
• GitHub Check: test
• GitHub Check: test
• GitHub Check: test (ubuntu-latest)

lib/segment/src/index/field_index/null_index/mmap_null_index.rs (4)

18-28: Well-designed specialized index structure

This implementation of a specialized index for null and empty values is a good approach to optimize query performance for these specific conditions. The design choice to keep separate flags for tracking values and nulls enables efficient filtering.

---

223-250: Well-implemented filtering logic

The filter implementation correctly handles the different filtering conditions based on is_empty and is_null. The approach to iterate only over known values when possible and return None when external iteration is required is a good optimization.

---

304-325: Builder pattern implementation

The builder pattern is properly implemented with clear initialization and finalization steps. The delegation to the underlying MmapNullIndex for add_point ensures consistency.

---

335-428:

❓ Verification inconclusive

Comprehensive test coverage

The test case thoroughly checks various scenarios including null values, null values in arrays, empty payloads, and non-null values. It validates the behavior of both filtering and cardinality estimation.

Consider adding a test for edge cases like large point IDs near the capacity of DynamicMmapFlags to ensure proper resizing behavior.

How does DynamicMmapFlags handle resizing for large point IDs? Is there a risk of performance degradation or memory issues?

---

🏁 Script executed:

#!/bin/bash
# Check how DynamicMmapFlags handles resizing
rg -A 10 "set_with_resize" lib/segment/src/vector_storage/dense/dynamic_mmap_flags.rs

Length of output: 511

---

Edge Case Test Recommendation

The current test case robustly checks filtering and cardinality logic for various payloads like null values, arrays with nulls, empty payloads, and non-null values. However, we should consider adding tests for edge cases that use very large point IDs.

Our verification shows that in lib/segment/src/vector_storage/dense/dynamic_mmap_flags.rs, the set_with_resize function dynamically resizes the index by setting the new length to key + 1 when a true value is provided, while it skips resizing for false values (given the default is false). Although this resizing strategy works as intended, it could lead to performance or memory issues if many high point IDs trigger frequent resizes.

Please add tests to:

• Verify the resizing behavior for large point IDs.
• Assess potential performance or memory overhead when resizing is triggered at scale.

Reviewing files that changed from the base of the PR and between 87e1c30 and 952d23f.

• lib/segment/src/index/field_index/null_index/mmap_null_index.rs (1 hunks)
• lib/segment/src/payload_storage/condition_checker.rs (5 hunks)

• GitHub Check: test-snapshot-operations-s3-minio
• GitHub Check: test-shard-snapshot-api-s3-minio
• GitHub Check: test-low-resources
• GitHub Check: test-consistency
• GitHub Check: Basic TLS/HTTPS tests
• GitHub Check: test (macos-latest)
• GitHub Check: test
• GitHub Check: test (windows-latest)
• GitHub Check: test-consensus-compose
• GitHub Check: test (ubuntu-latest)
• GitHub Check: test
• GitHub Check: test
• GitHub Check: test-consensus

lib/segment/src/index/field_index/null_index/mmap_null_index.rs (3)

18-28: Well-designed index struct for optimizing null and empty value queries.

The MmapNullIndex struct is well-designed for its specialized purpose of optimizing IsNull and IsEmpty conditions. The two separate flags for tracking whether values exist (has_values_slice) and whether null values exist (is_null_slice) allow for efficient querying of both conditions independently.

---

112-119: The array handling logic correctly implements the design decision.

The logic for handling arrays correctly implements the design decision that null values don't count as values (based on previous review comments). The code correctly checks for nulls and non-empty arrays separately.

---

334-428: Comprehensive test with good coverage of different cases.

The test thoroughly verifies the behavior of the null index with various combinations of values (null, null in array, empty, non-null). It validates both the filtering functionality and cardinality estimation.

lib/segment/src/payload_storage/condition_checker.rs (8)

33-37: Strong addition of empty value handling to the ValueChecker trait

The addition of the check_empty method to the ValueChecker trait provides a clear way to determine if a condition should match when a field is empty or missing from a payload. The default implementation returning false is appropriate for most condition types.

---

51-59: Well-structured null checking logic

The implementation of check_is_null is clear and properly handles different value types. The approach for arrays (checking if any array element is null) is particularly useful for JSON data processing.

---

63-99: Effective integration of is_empty and is_null conditions into field checking

The updated check_match method properly integrates the new conditions into the existing logical structure, maintaining the clarity of the code while expanding its functionality.

---

129-148: Clarify relationship between empty and null

In the check_empty method, when is_empty is not specified but is_null is, the method returns !*is_null. This implies that "empty" is the opposite of "null", which might not be immediately intuitive.

Is this relationship (empty being the opposite of null) aligned with the intended semantics of these conditions? This is especially important since the documentation seems to treat null and empty arrays as similar concepts.

---

296-298: Elegant implementation of check_empty for ValuesCount

Using check_count(0) to determine emptiness for ValuesCount is a clean and elegant approach that leverages existing functionality.

---

377-473: Comprehensive test coverage for new functionality

The test suite thoroughly covers all combinations of value types and conditions, providing excellent verification of the new functionality. The tests clearly demonstrate the expected behavior for each combination of payload value and condition.

The test cases confirm that:

1. Empty arrays ([]) match when is_empty=true
2. Arrays with null values ([null]) match when is_null=true, not when is_empty=true
3. Arrays with both values and nulls match is_null=true if they contain at least one null
4. Non-array values behave consistently with their expected emptiness/nullness

---

456-464:

❓ Verification inconclusive

Verify array null behavior matches requirements

The test shows that an array containing both a value and null ([true, null]) matches is_null: true. This means any array containing at least one null element is treated as "null" for filtering purposes.

Is this behavior (matching arrays with any null as opposed to only nulls) aligned with user expectations? If a user filters with is_null: true, they might expect to get only fields that are exclusively null, not fields that contain some non-null values.

---

🏁 Script executed:

#!/bin/bash
# Check for any documentation or comments about how is_null should behave with arrays
rg -A 5 -B 5 "is_null.*?array" --type rust
rg -A 5 -B 5 "array.*?is_null" --type rust

Length of output: 3398

---

Action Required: Confirm the Intended “is_null” Behavior for Arrays

The current implementation determines an array as null if it contains any null element (via array.iter().any(|x| x.is_null()) == is_null). The tests in lib/segment/src/payload_storage/condition_checker.rs and similar logic in the null index module confirm this behavior. However, note that a user filtering with is_null: true might expect to retrieve only arrays made up exclusively of null values. Please verify that this “any null” matching is indeed the intended design for filtering.

• File Checked: lib/segment/src/payload_storage/condition_checker.rs
• Key Logic: Value::Array(array) => array.iter().any(|x| x.is_null()) == is_null
• Implication: Arrays containing both null and non-null values (e.g. [true, null]) are considered null.

---

40-49:

❓ Verification inconclusive

Verify if array with null values is considered empty

The check_is_empty function treats array emptiness based on length, but there's a distinction in the implementation compared to the documentation reference: an array with null values ([null]) is not considered empty here.

According to the past review comment referencing your documentation:

This condition will match all records where the field reports either does not exist, or has null or [] value.

Is this intentional that an array containing only null ([null]) is not considered empty, while an empty array ([]) is?

---

🏁 Script executed:

#!/bin/bash
# Looking for other usages of is_empty to verify consistency
rg -A 3 -B 3 "is_empty.*?true" --type rust

Length of output: 4521

---

Review: Clarify Handling of Arrays with Null Values

The current implementation in lib/segment/src/payload_storage/condition_checker.rs at lines 40–49 treats arrays by checking their length (array.is_empty() == is_empty). This means an array like [null] is not considered empty, even though the associated documentation states that a field with a null or empty array ([]) should be treated as empty. We should verify whether this behavior is intentional. If the intention is to treat an array containing only null as empty, the function might need to be updated to inspect array elements (or adjust the documentation if the current behavior is desired).

• File: lib/segment/src/payload_storage/condition_checker.rs (Lines 40–49)
• Current behavior: For Value::Array(array), emptiness is determined solely by the length of the array.
• Documentation expectation: Fields with either a null value or an empty array should be considered empty, implying that [null] might also qualify.

Please confirm if this discrepancy is by design or if additional checks (e.g., treating arrays containing only null as empty) should be implemented.