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.

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

fix auto type column indexer realtime logical type to report long if all nulls since it is most restrictive type #16828

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

clintropolis
Copy link
Member

Report logical type as LONG instead of STRING since LONG is the more restrictive type.

…all nulls to match serialized segment if all null values
@clintropolis clintropolis changed the title fix auto type column indexer realtime logical type to report long if all nulls to match serialized segment if all null values fix auto type column indexer realtime logical type to report long if all nulls since it is most restrictive type Aug 1, 2024
// we didn't see anything, so we can be anything, so why not a string?
return ColumnType.STRING;
// we didn't see anything, so we can be anything, so be long as it is the most restrictive type
return ColumnType.LONG;
Copy link
Contributor

Choose a reason for hiding this comment

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

If this column is seen previously, should it just return that type?

Copy link
Member Author

Choose a reason for hiding this comment

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

the issue here is that there have been no values other than nulls so far, so we can't know a type yet

Copy link
Contributor

Choose a reason for hiding this comment

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

In the scenario when the ingestion spec doesn't have any custom dimensions, new incremental index is initialized with columns from previous index https://github.com/apache/druid/blob/master/server/src/main/java/org/apache/druid/segment/realtime/sink/Sink.java#L379, in that case it would probably be better to return the previous seen type?

Copy link
Contributor

@findingrish findingrish 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 the fix, LGTM!

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

Successfully merging this pull request may close these issues.

3 participants