Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix handling an empty list of versions #16198

Merged
merged 3 commits into from
Mar 26, 2024
Merged

Fix handling an empty list of versions #16198

merged 3 commits into from
Mar 26, 2024

Conversation

abhishekrb19
Copy link
Contributor

This patch addresses this review comment by treating null and an empty list of versions differently. An empty collection passed should return no results, rather than ignoring it and returning all results.

Changes

  • Update the condition to handle the null and empty versions differently
  • Clarify javadocs on the behavior of empty versions
  • Add tests for the empty versions in different classes, which would fail without this patch

This PR has:

  • been self-reviewed.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • been tested in a test Druid cluster.

@abhishekrb19
Differentiate null and empty lists of segment IDs and versions.
cbf9849 
Treat them differently so the. Segment IDs and versions can be An empty list,
in which case, the queries should just not return anything. Versions are optional, so
they can be null, which just indicates nothing, so the queries should return segments with
all possible versions. Segment IDs cannot be null as indicated by the absence of @nullable
annotation.
@abhishekrb19
Update javadocs and add empty versions test to kill task.
ff69ede
kfaraz
kfaraz approved these changes Mar 25, 2024
View reviewed changes
Copy link
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing this!

server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java Show resolved Hide resolved
@abhishekrb19 abhishekrb19 merged commit 95595ba into apache:master Mar 26, 2024
85 checks passed
@abhishekrb19 abhishekrb19 deleted the fix_null_empty_checks branch March 26, 2024 00:51
@abhishekrb19
Copy link
Contributor Author

Thanks for the quick review, @kfaraz!

@adarshsanjeev adarshsanjeev added this to the 30.0.0 milestone May 6, 2024
@adarshsanjeev adarshsanjeev mentioned this pull request May 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kfaraz kfaraz kfaraz approved these changes

Assignees
No one assigned
Labels
Area - Ingestion
Projects
None yet
Milestone
30.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@abhishekrb19 @kfaraz @adarshsanjeev