[lumina] Resolve search options through one merge - #10064
Closed
jackylee-ch wants to merge 1 commit into
Closed
jackylee-ch wants to merge 1 commit into
jackylee-ch wants to merge 1 commit into
Conversation
buildSearchOptions and ensureLoaded each inlined the same three-step merge, and mergeOptions -- which is that merge -- had no production caller, so the only test of the precedence rule asserted against a copy of it.
Contributor
|
Closing after review. The production diff routes two already-identical option merges through the existing helper, so it does not change the options used by an index load or search. The new tests call the helper directly and do not demonstrate a failing user-visible search on the baseline. I cannot establish end-to-end value for this standalone PR. A reproducer that fails through the reader, with a regression test at that boundary, would justify reopening. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
buildSearchOptionsandensureLoadedeach inlined the same three-step merge — table options, then index metadata, then per-query options — whilemergeOptions, which is that merge, had no production caller. The only test of the precedence rule therefore asserted against a copy of it rather than against the path a search takes.Both call sites now go through
mergeOptions. No behaviour change: all three copies already had the same order.The order matters because the metadata records what the index was built with, while the table value may have been changed since. A build-time option read from the table would search an index with parameters it was not built for.
Tests
LuminaSearchOptionPrecedenceTest: a stalelumina.encoding.pq.m=0and an unset dimension on the table cannot reach the searcher, because the metadata's8and256override them.paimon-lumina: 43 run, 0 failures, 25 skipped — the native library ships linux/amd64 only.Written with Claude Code; verification is mine.