You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains 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
I noticed something weird while reading the code: I believe there are missing break; statement in handling Uuid's; also note mismatch between Uuid handling and string-alike handling, this is yet another bug (mostly easily fixed by unifying string-alike and Uuid handling).
I added very crude test (just copied another and replaced strings with Uuid). It fails when applied alone, still fails with only break added, and passes with complete series applied.
Compile-tested, limited runtime-tested (my machine is too weak to handle even single round of compilation).
Hi! Thank you for contributing!
The tests on this PR will run after a maintainer adds an ok-to-test label to this PR manually. Thank you for your patience!
Also, why was not this bug (well, first half of this bug) caught in by static analysis (aka warning): it seems missing -Wimplicit-fallthrough (implied by -Wextra in gcc, but not in clang).
⚪ 2024-05-06 21:17:04 UTC Pre-commit check for 347e262 has started.
⚪ 2024-05-06 21:17:05 UTC Build linux-x86_64-release-clang14 is running...
🟢 2024-05-06 22:32:03 UTC Build successful.
⚪ 2024-05-06 21:17:12 UTC Pre-commit check for 347e262 has started.
⚪ 2024-05-06 21:17:14 UTC Build linux-x86_64-relwithdebinfo is running...
🟢 2024-05-06 21:20:29 UTC Build successful.
⚪ 2024-05-06 21:22:11 UTC Tests are running...
🔴 2024-05-06 23:16:48 UTC Some tests failed, follow the links below.
⚪ 2024-05-06 21:17:18 UTC Pre-commit check for 347e262 has started.
⚪ 2024-05-06 21:17:21 UTC Build linux-x86_64-release-asan is running...
🟢 2024-05-06 21:20:16 UTC Build successful.
⚪ 2024-05-06 21:21:59 UTC Tests are running...
🔴 2024-05-06 23:11:18 UTC Some tests failed, follow the links below.
... by unifying with other string-alikes.
Notice
auto str = TuplePtrs[i]->AsStringRef();
vs.
auto str = TuplePtrs[pi.ColumnIdx]->AsStringRef();
Former seems a bug (and fails tests).
⚪ 2024-05-22 13:03:36 UTC Pre-commit check for aae6390 has started.
⚪ 2024-05-22 13:03:38 UTC Build linux-x86_64-relwithdebinfo is running...
🟢 2024-05-22 13:06:59 UTC Build successful.
⚪ 2024-05-22 13:08:44 UTC Tests are running...
🔴 2024-05-22 15:01:37 UTC Some tests failed, follow the links below.
⚪ 2024-05-22 13:04:19 UTC Pre-commit check for aae6390 has started.
⚪ 2024-05-22 13:04:21 UTC Build linux-x86_64-release-asan is running...
🟢 2024-05-22 13:08:23 UTC Build successful.
⚪ 2024-05-22 13:10:12 UTC Tests are running...
🔴 2024-05-22 15:11:19 UTC Some tests failed, follow the links below.
⚪ 2024-05-22 13:05:36 UTC Pre-commit check for aae6390 has started.
⚪ 2024-05-22 13:05:38 UTC Build linux-x86_64-release-clang14 is running...
🟢 2024-05-22 13:36:42 UTC Build successful.
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.
yumkam
force-pushed
the
fix-grace-join-uuid-encoding
branch
from
Changelog entry
Fix Uuid handling in mkql_grace_join
Changelog category
Additional information
I noticed something weird while reading the code: I believe there are missing
break;statement in handling Uuid's; also note mismatch between Uuid handling and string-alike handling, this is yet another bug (mostly easily fixed by unifying string-alike and Uuid handling).I added very crude test (just copied another and replaced strings with Uuid). It fails when applied alone, still fails with only
breakadded, and passes with complete series applied.Compile-tested, limited runtime-tested (my machine is too weak to handle even single round of compilation).