fix: Resolve null literal in array_split_into_chunks via generic registration#16923
Open
allenshen13 wants to merge 2 commits intofacebookincubator:mainfrom
Open
fix: Resolve null literal in array_split_into_chunks via generic registration#16923allenshen13 wants to merge 2 commits intofacebookincubator:mainfrom
allenshen13 wants to merge 2 commits intofacebookincubator:mainfrom
Conversation
meta-cla
Bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
allenshen13
changed the title
allenshen13
changed the title
allenshen13
force-pushed
the
fix-array-split-into-chunks-null-handling
branch
from
2f6ccf0 to
2c4f8d5
Compare
Collaborator
Author
|
Currently there are 3 CI failures:
2 + 3) Biased Expression Fuzzer + Presto Bias Fuzzer: Both fail for the same reason: the fuzzer generates nested array_split_into_chunks calls that produce intermediate empty arrays. Presto Java's implementation uses sequence(1, 0, sz) for empty arrays, which throws because stop < start with a positive step. Velox correctly returns an empty array. This is a Presto Java bug. |
Collaborator
|
I see this closes prestodb/presto#27429 and there is a TODO in the code linked with that issue as well https://github.com/prestodb/presto/blob/d488205f1fdedddee6a537e25b704799d3b5e5b8/presto-native-tests/src/test/java/com/facebook/presto/nativetests/TestSqlInvokedFunctions.java#L116. Lets complete the TODO and add e2e tests in Presto as well. |
3 tasks
allenshen13
force-pushed
the
fix-array-split-into-chunks-null-handling
branch
from
2c4f8d5 to
a1e14d4
Compare
CI Failure Analysis
❌ Signature Changes — SIGNATURE Failure View logsSignature errors: The PR replaces 12 type-specific registrations with a single ❌ Expression Fuzzer with Presto SOT — FUZZER Failure View logsFuzzer: Presto Expression Fuzzer with Presto as source of truth The results differ in a single map key value: Velox returned ❌ Biased Expression Fuzzer — FUZZER Failure View logsFuzzer: Biased Expression Fuzzer (biased toward The fuzzer, biased specifically toward Correlation with PR changes:
Known issues:
Reproduce locally: For the Signature Changes failure, update the signature file to reflect the new generic signature after removing concrete type registrations. For the Biased Expression Fuzzer: ./velox_expression_fuzzer_test \
--seed 2488 \
--only=array_split_into_chunks \
--lazy_vector_generation_ratio 0.2 \
--common_dictionary_wraps_generation_ratio=0.3 \
--duration_sec 300 \
--enable_variadic_signatures \
--velox_fuzzer_enable_complex_types \
--velox_fuzzer_enable_column_reuse \
--velox_fuzzer_enable_expression_reuse \
--max_expression_trees_per_step 2 \
--retry_with_try \
--batch_size=6 \
--presto_url=http://127.0.0.1:8080For the Expression Fuzzer with Presto SOT (instance 3): ./velox_expression_fuzzer_test \
--seed 711308906 \
--enable_variadic_signatures \
--velox_fuzzer_enable_complex_types \
--lazy_vector_generation_ratio 0.2 \
--common_dictionary_wraps_generation_ratio=0.3 \
--velox_fuzzer_enable_column_reuse \
--velox_fuzzer_enable_expression_reuse \
--enable_dereference \
--duration_sec 300 \
--special_forms="cast,coalesce,if" \
--velox_fuzzer_max_level_of_nesting=1 \
--presto_url=http://127.0.0.1:8080Recommended fix:
|
Build Impact AnalysisSelective Build Targets (building these covers all 299 affected)Total affected: 299/571 targets Affected targets (299)Directly changed (2)
Transitively affected (297)
Fast path • Graph from main@5335309b03218136f89df30080e9389a42f6f2c7 |
CI Failure Analysis❌ Signature Changes — SIGNATURE Failure View logsSignature errors: The PR removes 12 type-specific registrations of Correlation with PR changes:
Known issues:
Recommended fix:
🤖 Generated with Claude Code |
allenshen13
changed the title
allenshen13
added a commit
to prestodb/presto
that referenced
this pull request
Apr 27, 2026
Narrow PR scope to enabling the native e2e test for array_split_into_chunks null literal handling (now possible thanks to facebookincubator/velox#16923). Revert: - The IF(cardinality(input) = 0, array[], ...) guard in ArraySqlFunctions. - The empty-array assertions in TestArraySqlFunctions. - The empty-array assertions in AbstractTestSqlInvokedFunctions. Keep the TestSqlInvokedFunctions TODO override removal so the parent test's "array_split_into_chunks(null, 2)" assertion runs against the native engine. Both Velox and Presto Java still throw on empty input today; aligning them on array[] is a separate semantic decision out of scope for #27429.
Narrow PR scope to the null-literal fix (generic registration). Restore the VELOX_USER_CHECK_GT on empty input and its throw-based test. Empty arrays still throw on both Velox and Presto Java today, so the fuzzer stays green under case "both paths threw." Whether to change this to return array[] is a separate semantic decision, tracked independently.
allenshen13
force-pushed
the
fix-array-split-into-chunks-null-handling
branch
from
6a33522 to
cbf0f5a
Compare
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.
Fixes: prestodb/presto#27429
Summary
Sidecar overload resolution fails on
array_split_into_chunks(null, 2):registerArraySplitIntoChunksFunctions<Generic<T1>>(prefix). A genericregistration lets the null literal resolve unambiguously and covers
all element types.
Expected CI failures (not blockers)
This is the intended change —
Generic<T1>replaces them. Pleaseacknowledge on the PR.