fix: accept any numeric literal for OPTIMIZE options - #833
Open
jackylee-ch wants to merge 3 commits into
Open
jackylee-ch wants to merge 3 commits into
jackylee-ch wants to merge 3 commits into
Conversation
The extension grammar boxes a numeric literal as Long, Float or Double depending on how it was written (`1`, `0.5`, `0.5d`), but buildOptions cast each option to one fixed box. So `OPTIMIZE t WITH (materialize_deletions_threshold = 1)` — the natural way to ask for "always materialize" — threw java.lang.ClassCastException: class java.lang.Long cannot be cast to class java.lang.Float and `= 0.5d` threw the same for Double. Only `0.5` and `0.5f` parsed as Float and worked; the one existing test happened to use `0.2f`. Route every numeric option through a Number accessor that takes all three boxes, and give the non-numeric case an error naming the option, matching the wording AddIndexExec.extractTrain already uses for `train`. The boolean options get the same treatment so a wrong literal no longer surfaces a raw cast error.
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
K-approved
The shared Number accessor widened the Long options too, so `num_threads = 2.5` would have run compaction with a silently truncated 2 where it previously failed. Narrow the widening to the one option documented as a float, and reject a non-integer literal for the Long options by name instead.
lance-gatekeeper
Bot
removed
K-approved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
the
K-approved
geruh
reviewed
Sep 18, 2026
|
|
||
| // The grammar boxes 1 as Long, 0.5 as Float and 0.5d as Double, so every spelling has to | ||
| // reach withMaterializeDeletionsThreshold instead of failing the cast. Only the first call | ||
| // has fragments left to compact, so assert acceptance rather than the compaction counts. |
Collaborator
There was a problem hiding this comment.
nit: test is pretty selfdocumenting we can drop the comment
Comment on lines
+54
to
+59
| /** | ||
| * The SQL extension grammar boxes a numeric literal as Long, Float or Double depending on how it | ||
| * was written (`1`, `0.5`, `0.5d`), so an option documented as a float cannot assume one of them. | ||
| * Options documented as Long stay Long-only on purpose: widening them would let `2.5` through as | ||
| * a silently truncated `2` instead of being rejected. | ||
| */ |
Collaborator
There was a problem hiding this comment.
nit: we can drop the javadoc. floatArg is any Number. the Long-only rule belongs on longArg w/Number.longValue would turn 2.5 into 2.
Collaborator
|
Thanks for working on this @jackylee-ch!! |
lance-gatekeeper
Bot
removed
the
K-approved
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The current merge retains the byte-for-byte patch reviewed at e614328. The Long-option conversion remains corrected: integer-valued options reject fractional literals by name, while materialize_deletions_threshold continues to accept the supported numeric spellings. The original threshold-boxing failure remains covered, and no significant residual risk remains.
lance-gatekeeper
Bot
added
the
K-approved
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
2 participants
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.
The extension grammar boxes a numeric literal as
Long,FloatorDoubledepending on how it was written (1,0.5,0.5d), butbuildOptionscast each option to one fixed box. SoOPTIMIZE t WITH (materialize_deletions_threshold = 1)— the natural way to ask for "always materialize" — threwjava.lang.ClassCastException: class java.lang.Long cannot be cast to class java.lang.Float, and= 0.5dthrew the same forDouble. Only0.5and0.5fparse asFloatand worked; the one existing test happens to use0.2f.Routes every numeric option through a
Numberaccessor that takes all three boxes, and names the option when the value is not numeric — the wordingAddIndexExec.extractTrainalready uses fortrain. Booleans get the same treatment, so a wrong literal no longer surfaces a raw cast error either.Reverting just the threshold accessor reproduces the
Long cannot be cast to Floatfailure and turns the non-numeric message back intoclass java.lang.String cannot be cast to class java.lang.Float. With the fix: 7 tests, 0 failures on both 4.1/2.13 and 3.5/2.12.