test: Enable native e2e test for array_split_into_chunks null literal#27559
Draft
allenshen13 wants to merge 1 commit intomasterfrom
Draft
test: Enable native e2e test for array_split_into_chunks null literal#27559allenshen13 wants to merge 1 commit intomasterfrom
allenshen13 wants to merge 1 commit intomasterfrom
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the SQL-implemented array_split_into_chunks function to return an empty array of arrays when given an empty input array, and adds corresponding unit and e2e tests while re-enabling the native engine test override. File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
4db6aa3 to
2df5f81
Compare
2df5f81 to
e7e282c
Compare
65773cf to
280ba5b
Compare
The SQL expression used sequence(1, cardinality(input), sz) which threw "sequence stop value should be greater than or equal to start value if step is greater than zero" for empty input arrays. Add an explicit empty-array guard in the SQL function that throws "Cannot split an empty array.", matching the message Velox already throws. This lets cross-engine tests assert the same error regex against both the Java and native engines, and unblocks enabling the parent test in the native test suite. Changes: - ArraySqlFunctions: add IF(cardinality(input) = 0, fail(...), ...) guard. - TestArraySqlFunctions: add unit test for the empty-array error. - AbstractTestSqlInvokedFunctions: add e2e coverage for empty-array, exact-fit boundary, boolean elements, and the cardinality-limit error. - TestSqlInvokedFunctions (native): remove the TODO override so the parent test runs against the native engine. Fixes: #27429
280ba5b to
e29701d
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
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.
Summary
Match Velox's behavior for
array_split_into_chunks(array[], n)so the inheritednative e2e test (
TestSqlInvokedFunctions#testArraySplitIntoChunks) can rununmodified:
"Cannot split an empty array."— byte-identical to Velox'sVELOX_USER_CHECK_GT(inputSize, 0, "Cannot split an empty array.")invelox/functions/prestosql/ArrayFunctions.h.// TODOoverride inpresto-native-tests/.../TestSqlInvokedFunctions.javathat was suppressing the entire native run of
testArraySplitIntoChunks.AbstractTestSqlInvokedFunctions.testArraySplitIntoChunkswith empty-array,exact-fit, and boolean-element cases. These run against H2 (Java) directly and
against the native engine via the inherited
TestSqlInvokedFunctions.TestArraySqlFunctions.testArraySplitIntoChunksEmptyArraycovering emptyarrays of bigint / varchar / integer / double.
Tracks #27429.
Behavior change
array_split_into_chunks(array[], n)withn > 0failed withthe
sequencebuiltin's error:"sequence stop value should be greater thanor equal to start value if step is greater than zero...". After this PR itthrows
"Cannot split an empty array.", identical to what Velox throws,so the same call fails the same way on both engines and the inherited native
e2e test can assert one regex against both. The original sequence failure throw is
ambiguous and should be changed.
Test plan
TestArraySqlFunctions#testArraySplitIntoChunksEmptyArraypassesAbstractTestSqlInvokedFunctions#testArraySplitIntoChunkspasses (Java/H2)TestSqlInvokedFunctions#testArraySplitIntoChunkspasses against the native engineMerge order
This PR must wait for facebookincubator/velox#16923 to land and for the
Velox SHA pinned by Presto to advance past it. That PR fixes the null-literal
overload-resolution half of #27429; until it's in, the inherited test's
array_split_into_chunks(null, 2)assertion will still fail on native.