fix: preserve OR grouping when combining pushed-down filters

[wombatu-kun]

Summary

This PR started as an explicit BuiltInFunctionDefinitions.IN branch. That branch is unreachable: Calcite expands IN over a literal list into OR(=, =, ...) while converting SQL to RelNode, so the predicate is already an OR in the abstract syntax tree and applyFilters never sees an IN call. SQL IN was already fully pushed down by the existing OR and EQUALS handling, so the branch and the getFilters() accessor added for it are dropped. Writing an end-to-end test to prove that surfaced a real defect, which is what this PR now fixes.

The bug

buildFilterExpression() joined accepted filters with String.join(" AND ", filters) without parenthesising them. The planner rewrites predicates into CNF and hands the conjunctive terms over separately, and a term can itself contain a top-level OR; since AND binds tighter, joining them bare regroups the predicate.

WHERE (status = 'active' AND id = 1) OR status = 'archived' arrives as two OR terms and was combined into:

(status = 'active') OR (status = 'archived') AND (id = 1) OR (status = 'archived')

which Lance reads as status = 'active' OR status = 'archived'. The id = 1 constraint is silently dropped and the scan returns rows that do not match the query. Pre-existing on main, not introduced by this PR. Each term is now wrapped before joining.

Coverage

LanceFilterPushDownSqlTest builds a Lance dataset through SQL and compares returned rows rather than rendered strings: string IN, numeric IN, NOT IN (covering the non-standard != the source emits in place of <>), a disjunctive predicate that regression-tests the fix above, and a literal containing an apostrophe. Each case also asserts the plan, since a row assertion on its own stays green if push-down regresses and Flink evaluates the predicate itself.

It is a *Test class rather than an addition to LanceSqlITCase because *ITCase classes never execute: surefire has no <includes> and no failsafe plugin is configured.

Build change

Surefire gains --add-opens=java.base/java.nio=ALL-UNNAMED. Arrow's MemoryUtil reflects into java.nio.Buffer.address, which JDK 16+ denies, and these are the first tests in the repository to allocate off-heap Arrow memory. The @{argLine} placeholder preserves the JaCoCo agent.

Relationship to #62

#62 re-enables testInPredicatePushDown by asserting the OR chain the planner produces. This PR leaves that file untouched, so the two do not conflict.

[wombatu-kun]

This PR doubles as a fix for a pre-existing failure on main: LanceReadOptimizationsTest.testInPredicatePushDown already lives on main and asserts IN-predicate pushdown, but the implementation (buildInFilter) is only added here. So main's test suite is currently red on that test, and any test-running CI (e.g. #25) stays red until this lands — merging #21 first turns it green.

[wombatu-kun]

This PR doubles as a fix for a pre-existing failure on main: LanceReadOptimizationsTest.testInPredicatePushDown already lives on main and asserts IN-predicate pushdown, but the implementation (buildInFilter) is only added here. So main's test suite is currently red on that test, and any test-running CI (e.g. #25) stays red until this lands — merging #21 first turns it green.

hi @fightBoxing review and merge it please

[LuciferYang]

Measured with EXPLAIN ... WHERE status IN ('a','b','c'): the planner pushes OR(OR(=,=),=) down to the source, so BuiltInFunctionDefinitions.IN never reaches applyFilters and the new branch isn't hit on the SQL or Table API push-down path.

• Impact: IN was already fully pushed down by the existing OR+EQUALS path before this PR (no leftover Calc above the scan), so the new code adds close to nothing.
• Verified: numeric columns, a 60-value list (no SEARCH/Sarg), Table API .in(), and both Flink 1.18 and 1.20 behave the same; NOT IN expands to and(<>,<>) and is likewise not hit. Not checked on 2.x.
• Ask: add a real SQL case proving the branch is hit. If it can't be, neither the branch nor the getFilters() accessor added for it is needed.

[wombatu-kun]

Done bf2f0ad - the branch and getFilters() are dropped. The new LanceFilterPushDownSqlTest asserts the OR chain reaches the scan with nothing left in a Calc.

[LuciferYang]

#55 on main added @Disabled to testInPredicatePushDown (IN push-down didn't exist then), so after merging, the two assertions you added get skipped with it. Measured on the merged tree: Tests run: 24, Skipped: 1.

• Impact: main gains code with no assertion exercising it, and getFilters() has no live caller.
• Ask: drop the @Disabled and its import when rebasing. Its stated reason cites the IN (not supported yet) comment, which this PR removes.
• Note: moot if the IN branch is dropped per the reachability finding.

[wombatu-kun]

Left to #62. This PR no longer touches LanceReadOptimizationsTest, so the two do not conflict.

[LuciferYang]

The new assertion checks the rendered string, but nothing exercises where that string goes (buildFilterExpression -> readFilter -> Lance scan). Pre-existing in this repo, same for = and LIKE.

• Impact: if Lance rejects some syntax, unit tests stay green and it only fails at runtime on real SQL.
• Ask: add a WHERE status IN (...) case against a real dataset in LanceSqlITCase, asserting the row count, to show Lance accepts the string.
• Note: that class currently holds only factory/options assertions, so this would be its first executeSql execution test.

[wombatu-kun]

Done bf2f0ad - LanceFilterPushDownSqlTest creates a dataset through SQL and compares returned rows for string IN, numeric IN, NOT IN and an escaped apostrophe, asserting the plan alongside each so the case still fails if push-down regresses.

Two deviations. It is a *Test class rather than LanceSqlITCase, because surefire has no <includes> and no failsafe plugin is configured, so *ITCase classes never execute. And it needed --add-opens=java.base/java.nio=ALL-UNNAMED in surefire: these are the first tests here to allocate off-heap Arrow memory, and JDK 17/21 otherwise fail in MemoryUtil.

The disjunctive case found a real defect on main: buildFilterExpression() joined accepted terms without parenthesising them, so under SQL precedence AND captured OR operands and silently dropped a constraint. Fixed in the same commit.