Skip to content

add missing vector object selector for multi-value string columns, refactor some stuff#13379

Merged
clintropolis merged 4 commits into
apache:masterfrom
clintropolis:multi-value-string-vector-object-selector
Nov 18, 2022
Merged

add missing vector object selector for multi-value string columns, refactor some stuff#13379
clintropolis merged 4 commits into
apache:masterfrom
clintropolis:multi-value-string-vector-object-selector

Conversation

@clintropolis
Copy link
Copy Markdown
Member

Description

Adds missing VectorObjectSelector for multi-value STRING typed columns. Several complex aggregators use VectorObjectSelector when they don't care for dealing with or can't do anything useful with dictionary lookup stuff directly, so this fills in the missing selector implementation to ensure if it is called that it doesn't explode.

While I was here, this selector creating code is heavily copied between StringDictionaryEncodedColumn, StringFrontCodedDictionaryEncodedColumn, and even NestedFieldLiteralDictionaryEncodedColumn, so I have refactored the base machinery into abstract types and then each column implementation can bring its own dictionary to fill out how to do lookup, which was the primary difference between them.


This PR has:

  • been self-reviewed.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • 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.

@clintropolis clintropolis merged commit 7f4e386 into apache:master Nov 18, 2022
@clintropolis clintropolis deleted the multi-value-string-vector-object-selector branch November 18, 2022 05:08
@kfaraz kfaraz added this to the 25.0 milestone Nov 21, 2022
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