Skip to content

test: assert IN predicate falls back to Flink instead of disabling the case - #60

Closed
LuciferYang wants to merge 1 commit into
lance-format:mainfrom
LuciferYang:fix/in-pushdown-test-assertion
Closed

test: assert IN predicate falls back to Flink instead of disabling the case#60
LuciferYang wants to merge 1 commit into
lance-format:mainfrom
LuciferYang:fix/in-pushdown-test-assertion

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

What

Re-enables LanceReadOptimizationsTest.testInPredicatePushDown, which #55 disabled, by asserting what the connector actually does with an IN predicate today.

Why

#55 needed a green pipeline, and this test was the one red case: it asserted that IN gets pushed down, but the source never implemented that. convertCallExpression() has no branch for BuiltInFunctionDefinitions.IN, so it returns null and applyFilters() routes the predicate into remainingFilters. The quickest way to get CI green was @Disabled, with re-working the test called out as a follow-up. This is that follow-up.

Disabling had a cost: testInPredicatePushDown was the only test touching BuiltInFunctionDefinitions.IN, so the IN path ended up with no coverage at all. Nothing would catch it if a later refactor made IN throw, or accepted it for push-down when the filter translation still can't express it.

How

Assert the fallback rather than the unimplemented feature:

assertEquals(0, result.getAcceptedFilters().size(), "IN should not be pushed down yet");
assertEquals(1, result.getRemainingFilters().size(), "IN should be left for Flink to evaluate");

CI stays green and the path stays under test. When IN push-down does land, these assertions turn red, which is the reminder to flip them back to acceptedFilters == 1. The reasoning is in a comment on the test so the next reader doesn't have to re-derive it.

Also corrected the class Javadoc, which listed IN and BETWEEN as covered. BETWEEN has no test and no implementation either (the source just has a // BETWEEN (not supported yet) comment).

Test plan

  • mvn -pl lance-flink-1.18 test -Dtest=LanceReadOptimizationsTest -> 24 run, 0 failures, 0 skipped (was 24 run / 1 skipped)
  • mvn -B -ntp -am -pl lance-flink-1.18 verify -> BUILD SUCCESS (178 run, 0 failures, 17 skipped)
  • mvn -B -ntp -am -pl lance-flink-1.19 verify -> BUILD SUCCESS
  • mvn -B -ntp -am -pl lance-flink-1.20 verify -> BUILD SUCCESS

The remaining 17 skips are LanceCatalogS3Test$MinioIntegrationTests, gated on MinIO env vars.

Still open from the #55 discussion

Wiring *ITCase into CI needs work before the pipeline part is worth doing, since the ITs don't pass today:

  1. Fix the shade relocation. With maven-failsafe-plugin added locally, the ITs run post-package against the shaded jar and hit NoSuchMethodError on LanceTypeConverter.toArrowSchema(RowType) and LanceNamespace.connect(String, Map, BufferAllocator), which looks like relocated and non-relocated Arrow signatures mixed on one classpath.
  2. Fix LanceNamespaceCatalog.createTable, which doesn't send the Arrow IPC schema stream, so DirectoryNamespace rejects it with InvalidInputException code=13.
  3. Then add failsafe and split the workflow into a fast unit-test lane and a separate IT job.

…e case

lance-format#55 disabled testInPredicatePushDown so the new CI pipeline could go
green: it asserted that an IN predicate gets pushed down, but
convertCallExpression() has no branch for BuiltInFunctionDefinitions.IN
and returns null, so applyFilters() routes the predicate to
remainingFilters.

Disabling left IN as the only push-down path with no coverage at all.
Assert the current fallback instead (acceptedFilters 0, remainingFilters
1), which keeps CI green and keeps the path under test; the assertions
turn red once IN push-down is implemented, which is the reminder to
update them.

Also fix the class Javadoc, which listed IN and BETWEEN as covered.
BETWEEN has no test and is not implemented in the source either.
@github-actions github-actions Bot added the chore Maintenance and chores label Aug 6, 2026
@LuciferYang
LuciferYang marked this pull request as draft August 6, 2026 11:27
@LuciferYang

Copy link
Copy Markdown
Contributor Author

Closing this in favour of #21, which fixes the source instead of the test.

I opened this without checking for existing work, and #21 (feat: push IN predicate down to Lance filter engine, open since April) already implements IN push-down in LanceDynamicTableSource and strengthens this same test case. Adapting the test to the unimplemented state was the wrong call when the implementation is already written: with #21 merged, testInPredicatePushDown passes as originally written and the @Disabled that #55 added can just be dropped.

This PR would also have put #21 into conflict, since both touch the same assertions in testInPredicatePushDown.

The Javadoc correction here (the class listed IN and BETWEEN as covered) is unrelated to that conflict, so I'll fold it into whatever lands alongside #21 rather than keep this PR open for one comment line.

@LuciferYang LuciferYang closed this Aug 6, 2026
@LuciferYang
LuciferYang deleted the fix/in-pushdown-test-assertion branch August 6, 2026 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance and chores

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant