Skip to content

simplify IncrementalIndex since group-by v1 has been removed#15448

Merged
clintropolis merged 2 commits intoapache:masterfrom
clintropolis:simplify-facts-tables
Nov 29, 2023
Merged

simplify IncrementalIndex since group-by v1 has been removed#15448
clintropolis merged 2 commits intoapache:masterfrom
clintropolis:simplify-facts-tables

Conversation

@clintropolis
Copy link
Copy Markdown
Member

@clintropolis clintropolis commented Nov 29, 2023

Description

This PR removes a some stuff that was only used by group-by v1, which was removed in #14866, simplifying IncrementalIndex and OnHeapIncrementalIndex implementations. The removed parameters are:

  • deserializeComplexMetrics which is always true (was false in group-by v1)
  • concurrentEventAdd which is always false (was true in group-by v1)
  • and sortFacts which is always true (could be false in group-by v1)

Removing the parameters allowed simplifying stuff like FactsHolder and some other things. I've also moved the two FactsHolder implementations to OnHeapIncrementalIndex where they are actually used (no logic change here, just a simple move), assuming that any other implementations would bring their own FactsHolder implementation.


This PR has:

  • been self-reviewed.
  • been tested in a test Druid cluster.

Copy link
Copy Markdown
Contributor

@soumyava soumyava left a comment

Choose a reason for hiding this comment

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

LGTM !

@clintropolis clintropolis merged commit 0516d0d into apache:master Nov 29, 2023
@clintropolis clintropolis deleted the simplify-facts-tables branch November 29, 2023 22:46
yashdeep97 pushed a commit to yashdeep97/druid that referenced this pull request Dec 1, 2023
yashdeep97 pushed a commit to yashdeep97/druid that referenced this pull request Dec 1, 2023
@LakshSingla LakshSingla added this to the 29.0.0 milestone Jan 29, 2024
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