Repository navigation
[fix](compile) Fix MacOS compile failed - #44901
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
b5d6193 to
66d8bfc
Compare
|
run buildall |
column_decimal.h:262
TPC-H: Total hot run time: 40653 ms |
TPC-DS: Total hot run time: 190896 ms |
ClickBench: Total hot run time: 32.82 s |
|
TeamCity be ut coverage result: |
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
### What problem does this PR solve? Issue Number: None Related PR: apache#44901 Problem Summary: With the per-row seed fix, every row's seed goes through `cast_set<uint32_t>`, which throws INTERNAL_ERROR for a negative seed or one above 4294967295: SELECT array_shuffle([1, 2, 3, 4, 5], -1); -- [INTERNAL_ERROR]value -1 cast to type j out of range [0,4294967295] Default NULL handling runs the function on every physical row, so this also fails rows whose result is NULL: a NULL array next to seed -1 in the same block, or `NULLIF(s, -1)`, which keeps -1 under the NULL. The range check came from apache#44901 (a macOS build fix in 4.0); 2.1/3.0/3.1 accepted these seeds. Rows with 0 or 1 element also re-seeded the generator, which fills the 624-word mt19937 state, although shuffling them draws no random numbers. Fix: - Like ClickHouse's arrayShuffle, use the whole BIGINT as a 64-bit seed with std::mt19937_64 (the engine uniform() already seeds per row). Any BIGINT is a valid seed, seeds with the same low 32 bits (such as -1 and 4294967295) no longer give the same result, and no row can fail on its seed. The query above now returns [3, 1, 4, 2, 5]. - Skip arrays with fewer than 2 elements before seeding the generator. - The no-seed path uses the same engine; it is still one time-based stream per block. - The UT helper now stops when execute fails, instead of reading the missing result column and aborting the whole test binary. ### Release note array_shuffle(arr, seed) accepts any BIGINT seed again, including negative values. The permutation for a given seed differs from earlier versions. ### Check List (For Author) - Test: Unit Test / Regression test - BE UT function_array_shuffle_test: 4/4 pass; the new any_bigint_seed case fails on the previous code with the INTERNAL_ERROR above - Regression test_array_shuffle_seed (new cases: negative seeds, the BIGINT limits, a NULL array with seed -1, NULLIF(s, -1)), test_array_functions, test_size_function, test_array_function_doc (.out regenerated, only seeded shuffle results changed), test_array_functions_by_literal, test_array_zip_array_enumerate_uniq - Behavior changed: Yes. Negative seeds and seeds above 4294967295 work instead of failing, and the permutation for a given seed differs from earlier versions. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
### What problem does this PR solve? Issue Number: None Related PR: apache#44901 Problem Summary: array_shuffle(arr[, seed]) builds one random generator per block, and every row of the block draws from it. The earlier commits on this branch re-seeded the generator for every row, which gave each row its own seed but costs about 1 us per row, because seeding std::mt19937_64 fills 312 state words. Other engines with a seeded array shuffle (ClickHouse arrayShuffle, Spark shuffle, Velox, the DataFusion Spark-compatible shuffle) do not take a per-row seed: the seed must be a constant, and each block draws from one random sequence. This commit switches to that model and fixes the related problems: 1. A non-constant seed was accepted but silently ignored, because the generator was seeded from the seed of the first row of the block. The FE now rejects it in ArrayShuffle.checkLegalityBeforeTypeCoercion ("The seed of array_shuffle must be a constant"), and the BE rejects it in open(), so a BE that gets such a plan from an older FE during a rolling upgrade fails instead of ignoring the seeds. 2. Since apache#44901, a negative seed or a seed larger than 4294967295 failed with an out-of-range INTERNAL_ERROR from cast_set<uint32_t>. The seed now keeps its low 32 bits with static_cast<uint32_t>, which is the 3.x behavior. Results for seeds in [0, 4294967295] do not change. 3. A constant array got the same order on every row, because the BE computed the call once for all-constant arguments, and with enable_fold_constant_by_be = true the FE folded the call into one literal. The BE now returns false from use_default_implementation_for_constants(), and ArrayShuffle extends UniqueFunction like random() and uuid(), so it is never folded into a constant or merged with another call. Before: 100 rows of a constant array gave 1 order; after: 100 orders. 4. Without a seed, each block was seeded with time(nullptr), so blocks processed in the same second repeated the same orders: 100000 rows of a 20-element array gave only 26560 different orders. Each block is now seeded from std::random_device, like rand(), and the same query gives 100000 orders. A single call with a constant seed, such as array_shuffle([1, 2, 3, 4], 0), still gives the same result as before. ### Release note array_shuffle / shuffle: a non-constant seed now reports an error instead of being silently ignored; a negative seed or a seed larger than 4294967295 is accepted again, and only its low 32 bits are used; a constant array is shuffled on each row instead of getting the same order on every row; without a seed, rows processed in the same second no longer repeat the same orders. ### Check List (For Author) - Test: Unit Test / Regression test - BE unit test: function_array_shuffle_test, 6 cases passed - FE unit test: UniqueFunctionTest, 18 cases passed - Regression test: test_array_shuffle_seed passed; test_array_functions, test_size_function and test_array_function_doc passed and their .out files are the same as on master - Behavior changed: Yes. A non-constant seed reports an error, a negative or large seed no longer reports an error, and a constant array gets a different order on each row. - Does this need documentation: Yes, an apache/doris-website PR will follow Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
### What problem does this PR solve? Issue Number: None Related PR: apache#44901 Problem Summary: array_shuffle kept only the low 32 bits of its BIGINT seed for std::mt19937, so seeds with the same low 32 bits, such as -1 and 4294967295, gave the same order. ClickHouse arrayShuffle gives the full 64-bit seed to pcg64_fast. This commit switches the generator to std::mt19937_64 and uses all 64 bits of the seed, so -1 and 4294967295 now give different orders. Without a seed, each block is seeded with 64 bits from std::random_device. Because the generator changed, the order for a given seed is different from before, so the seeded results of test_size_function, test_array_function_doc and test_array_shuffle_seed are regenerated. Unit tests that expect two orders to differ now use a 20-element array, because a 5-element array has only 120 orders and two seeds can give the same one by chance. This commit also fixes the comments that said all rows share one random sequence. Each block starts its own sequence from the seed, so a per-row seed would be ignored, which is why a non-constant seed is rejected. ### Release note array_shuffle / shuffle uses all 64 bits of the seed, so the order for a given seed differs from earlier versions, and seeds with the same low 32 bits no longer give the same order. ### Check List (For Author) - Test: Unit Test / Regression test - BE unit test: function_array_shuffle_test, 6 cases passed - Regression test: test_array_shuffle_seed, test_array_functions, test_size_function and test_array_function_doc passed; test_array_functions keeps its .out file - Behavior changed: Yes. The order for a given seed changes, and seeds with the same low 32 bits give different orders. - Does this need documentation: Yes, an apache/doris-website PR will follow Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
### What problem does this PR solve? Issue Number: None Related PR: apache#44901 Problem Summary: An earlier commit on this branch read the constant seed with unpack_if_const, which takes the one-value column inside the ColumnConst instead of expanding it to a full column. Both give the same seed, and expanding the constant only copies one BIGINT per row of the block, which is small next to shuffling the arrays. Go back to convert_to_full_column_if_const, as the original code does, so the change to this function stays small, and drop the two includes that were only added for unpack_if_const. ### Release note None ### Check List (For Author) - Test: Unit Test - BE unit test: function_array_shuffle_test, 6 cases passed - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
### What problem does this PR solve? Issue Number: None Related PR: #44901 Problem Summary: 1. `array_shuffle` now rejects a non-constant seed, which was silently ignored for every row but the first one of each block. 2. The seed now uses all 64 bits (`std::mt19937_64`), so any BIGINT seed works, including a negative one, which failed with an out-of-range error since #44901. 3. Every row is now shuffled on its own, so a constant array no longer gets the same order on every row, and blocks without a seed no longer repeat each other. doc: apache/doris-website#4176 ### Release note `array_shuffle` / `shuffle` rejects a non-constant seed, uses all 64 bits of the seed (so the order for a given seed changes), and shuffles every row on its own. ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [x] Regression test - [x] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [x] Yes. See the three points above. - Does this need documentation? - [ ] No. - [x] Yes. <!-- apache/doris-website PR link to be added --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into --> --------- Co-authored-by: Claude Opus 5.5 (1M context) <[email protected]>
### What problem does this PR solve? Issue Number: None Related PR: #44901 Problem Summary: 1. `array_shuffle` now rejects a non-constant seed, which was silently ignored for every row but the first one of each block. 2. The seed now uses all 64 bits (`std::mt19937_64`), so any BIGINT seed works, including a negative one, which failed with an out-of-range error since #44901. 3. Every row is now shuffled on its own, so a constant array no longer gets the same order on every row, and blocks without a seed no longer repeat each other. doc: apache/doris-website#4176 ### Release note `array_shuffle` / `shuffle` rejects a non-constant seed, uses all 64 bits of the seed (so the order for a given seed changes), and shuffles every row on its own. ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [x] Regression test - [x] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [x] Yes. See the three points above. - Does this need documentation? - [ ] No. - [x] Yes. <!-- apache/doris-website PR link to be added --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into --> --------- Co-authored-by: Claude Opus 5.5 (1M context) <[email protected]>
What problem does this PR solve?
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)