Skip to content

chore(sql): fix potential NPE in parallel group by#6303

Merged
ideoma merged 2 commits intomasterfrom
puzpuzpuz_parallel_groupby_smoke_test_npe
Oct 24, 2025
Merged

chore(sql): fix potential NPE in parallel group by#6303
ideoma merged 2 commits intomasterfrom
puzpuzpuz_parallel_groupby_smoke_test_npe

Conversation

@puzpuzpuz
Copy link
Copy Markdown
Contributor

@puzpuzpuz puzpuzpuz commented Oct 24, 2025

Since #6268, getLong(Record) call on count_distinct group by functions may use the GroupByAllocator when merging partial hash sets. Yet, the allocator was set on group by functions only when getFunctionUpdater() was called. So, in rare cases when the query owner thread never participated in aggregation and, thus, never called the getFunctionUpdater() method, an NPE would occur when returning the group by function values to the client.

@puzpuzpuz puzpuzpuz self-assigned this Oct 24, 2025
@puzpuzpuz puzpuzpuz added the DO NOT MERGE These changes should not be merged to main branch label Oct 24, 2025
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Oct 24, 2025

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch puzpuzpuz_parallel_groupby_smoke_test_npe

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@puzpuzpuz puzpuzpuz force-pushed the puzpuzpuz_parallel_groupby_smoke_test_npe branch from acd9874 to 336561d Compare October 24, 2025 08:53
@puzpuzpuz puzpuzpuz changed the title test(sql): investigate NPE in parallel group by smoke tests chore(sql): fix potential NPE in parallel group by Oct 24, 2025
@puzpuzpuz puzpuzpuz removed the DO NOT MERGE These changes should not be merged to main branch label Oct 24, 2025
@glasstiger
Copy link
Copy Markdown
Contributor

[PR Coverage check]

😍 pass : 19 / 20 (95.00%)

file detail

path covered line new line coverage
🔵 io/questdb/cutlass/pgwire/PGPipelineEntry.java 0 1 00.00%
🔵 io/questdb/griffin/engine/groupby/GroupByArraySink.java 2 2 100.00%
🔵 io/questdb/griffin/engine/table/AsyncGroupByAtom.java 9 9 100.00%
🔵 io/questdb/griffin/engine/table/AsyncGroupByNotKeyedAtom.java 8 8 100.00%

@ideoma ideoma merged commit 3525d8a into master Oct 24, 2025
35 checks passed
@ideoma ideoma deleted the puzpuzpuz_parallel_groupby_smoke_test_npe branch October 24, 2025 09:54
@puzpuzpuz
Copy link
Copy Markdown
Contributor Author

@ideoma thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants