Skip to content

Conversation

@Jay-ju
Copy link
Contributor

@Jay-ju Jay-ju commented Jan 5, 2026

  • Add Filtering with Custom SQL Expressions section to docs/connectors/lance.md.
  • Add test cases for LanceDB read with custom SQL filters and projections.

Changes Made

Related Issues

@github-actions github-actions bot added the docs label Jan 5, 2026
@Jay-ju Jay-ju force-pushed the docs/lance-custom-filter-clean branch from 37d7747 to d6758b6 Compare January 5, 2026 14:09
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 5, 2026

Greptile Summary

Added documentation and tests for Lance custom SQL filter support, enabling users to pass raw SQL filters and projections to Lance scanner via default_scan_options.

  • Documentation added for filtering with custom SQL expressions and geo-spatial functions
  • Two new test cases validate SQL filter passthrough and geo projection functionality
  • Removed redundant test case from test_lancedb_filter_then_limit_behavior

Critical Issues:

  • Documentation contains duplicate "Filtering with Custom SQL Expressions" section (lines 104-130 and 254-273)
  • Duplicate "Compaction" heading at line 275 that needs removal
  • Redundant inline lance imports in new test functions (already imported at top level)

Confidence Score: 2/5

  • Not safe to merge - contains duplicate documentation sections that break document structure
  • The PR has critical structural issues with duplicate documentation sections that would confuse readers and break the document flow. While the test implementations are sound, the documentation errors are blocking issues.
  • Pay close attention to docs/connectors/lance.md - duplicate sections must be removed before merge

Important Files Changed

Filename Overview
docs/connectors/lance.md Added documentation for custom SQL filters, but introduced duplicate sections that need removal
tests/io/lancedb/test_lancedb_reads.py Added comprehensive tests for SQL filter passthrough and geo projections with redundant inline imports

Sequence Diagram

sequenceDiagram
    participant User
    participant Daft
    participant LanceScanner
    participant LanceDataset
    
    User->>Daft: read_lance(uri, default_scan_options)
    Note over User,Daft: default_scan_options contains:<br/>- filter: SQL filter string<br/>- columns: SQL projections<br/>- with_row_id: boolean
    
    Daft->>LanceScanner: Initialize scanner with options
    LanceScanner->>LanceDataset: Open Lance dataset
    LanceDataset-->>LanceScanner: Dataset metadata
    
    LanceScanner->>LanceScanner: Apply SQL filter string
    Note over LanceScanner: Executes Lance-native SQL<br/>(e.g., st_distance, st_intersects)
    
    LanceScanner->>LanceScanner: Apply SQL projections
    Note over LanceScanner: Calculate derived columns<br/>(e.g., distance calculations)
    
    LanceScanner-->>Daft: Filtered & projected data
    Daft-->>User: DataFrame with results
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional Comments (4)

  1. docs/connectors/lance.md, line 254-273 (link)

    logic: duplicate section - "Filtering with Custom SQL Expressions" already exists at lines 104-130

  2. docs/connectors/lance.md, line 275 (link)

    logic: duplicate heading - "### Compaction" already exists at line 184

  3. tests/io/lancedb/test_lancedb_reads.py, line 152 (link)

    style: lance is already imported at the top of the file (line 3), remove this redundant inline import

    Context Used: Rule from dashboard - Import statements should be placed at the top of the file rather than inline within functions or met... (source)

  4. tests/io/lancedb/test_lancedb_reads.py, line 184 (link)

    style: lance is already imported at the top of the file (line 3), remove this redundant inline import

    Context Used: Rule from dashboard - Import statements should be placed at the top of the file rather than inline within functions or met... (source)

2 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

@Jay-ju Jay-ju force-pushed the docs/lance-custom-filter-clean branch from d6758b6 to ec4fd8e Compare January 6, 2026 03:01
- Add `Filtering with Custom SQL Expressions` section to `docs/connectors/lance.md`.
- Add test cases for LanceDB read with custom SQL filters and projections.
@Jay-ju Jay-ju force-pushed the docs/lance-custom-filter-clean branch from ec4fd8e to 935b970 Compare January 6, 2026 03:08
@codecov
Copy link

codecov bot commented Jan 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.62%. Comparing base (535941d) to head (4eaf12c).
⚠️ Report is 39 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5916      +/-   ##
==========================================
+ Coverage   72.48%   72.62%   +0.14%     
==========================================
  Files         966      970       +4     
  Lines      125838   126553     +715     
==========================================
+ Hits        91211    91908     +697     
- Misses      34627    34645      +18     

see 88 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kevinzwang kevinzwang requested review from everettVT and ykdojo January 6, 2026 22:02
Copy link
Contributor

@everettVT everettVT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Jay-ju for adding this! LGTM.

@Jay-ju
Copy link
Contributor Author

Jay-ju commented Jan 8, 2026

@ykdojo Could you please take a look when you have time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants