refactor CompactionState since core and processing have been merged by clintropolis · Pull Request #14932 · apache/druid

clintropolis · 2023-08-31T05:54:15Z

Since #13698 was merged, it is now possible for CompactionState to refer to real types instead of using Map and TypeReference to convert stuff for TransformSpec, IndexSpec, and AggregatorFactory.

…o now can refer to real types

clintropolis · 2023-08-31T05:55:39Z

GranularitySpec is still in druid-server and so is still defined using a Map. Should we move it into druid-processing? It doesn't seem to depend on anything else in server to require it be there...

kfaraz · 2023-08-31T06:03:51Z

Yeah, it makes sense to move GranularitySpec to druid-processing to help simplify CompactionState further.

kfaraz · 2023-08-31T06:05:08Z

                                      : new DimensionsSpec(ingestionSpec.getDataSchema().getDimensionsSpec().getDimensions());
      // We only need to store filter since that is the only field auto compaction support
-      Map<String, Object> transformSpec = ingestionSpec.getDataSchema().getTransformSpec() == null || TransformSpec.NONE.equals(ingestionSpec.getDataSchema().getTransformSpec())
+      TransformSpec transformSpec = ingestionSpec.getDataSchema().getTransformSpec() == null || TransformSpec.NONE.equals(ingestionSpec.getDataSchema().getTransformSpec())


Nit: Pull ingestionSpec.getDataSchema().getTransformSpec() into a variable. It is used thrice and seems too verbose.

kfaraz · 2023-08-31T18:09:58Z

-                                 ? null
-                                 : toolbox.getJsonMapper().convertValue(ingestionSpec.getDataSchema().getAggregators(), new TypeReference<List<Object>>() {});
+      TransformSpec transformSpec = ingestionSpec.getDataSchema().getTransformSpec();
+      if (TransformSpec.NONE.equals(transformSpec)) {


Should this handle transformSpec == null too?

ah yeah, though i put a not null check in the else instead since it seemed a little strange to check for null and assign null again

kfaraz · 2023-08-31T18:12:13Z

 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.google.common.base.Preconditions;
+import org.apache.druid.indexer.granularity.GranularitySpec;


Nit: This might get flagged as an unnecessary import as it is only used in javadoc.

kfaraz · 2023-08-31T18:15:09Z

-  // org.apache.druid.query.aggregation.AggregatorFactory cannot be used here because it's in the 'processing' module which
-  // has a dependency on the 'core' module where this class is.
-  private final List<Object> metricsSpec;
+  // 'server' module which has a dependency on the 'processing' module where this class is.


Can remove this comment too.

oops, fixed

github-actions · 2024-03-02T00:15:52Z

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

kfaraz · 2024-03-04T05:53:06Z

@clintropolis , there are some conflicts that would need to be resolved before we can merge this.

github-actions · 2024-05-04T00:16:20Z

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

github-actions · 2024-07-07T00:19:37Z

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

kfaraz · 2024-09-02T06:32:15Z

@clintropolis , should we resolve the conflicts here so that we can include this patch in Druid 31?

github-actions · 2024-11-02T00:20:32Z

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

github-actions · 2025-01-05T00:23:13Z

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

github-actions · 2025-02-03T00:21:28Z

This pull request/issue has been closed due to lack of activity. If you think that
is incorrect, or the pull request requires review, you can revive the PR at any time.

refactor CompactionState since core and processing have been merged s…

3b0e7be

…o now can refer to real types

clintropolis added the Refactoring label Aug 31, 2023

kfaraz approved these changes Aug 31, 2023

View reviewed changes

clintropolis added 3 commits August 31, 2023 02:58

move GranularitySpec to druid-processing

d37e846

fix

e6f7c52

style

57ca94b

kfaraz reviewed Aug 31, 2023

View reviewed changes

adjust

23964be

kfaraz approved these changes Aug 31, 2023

View reviewed changes

github-actions Bot added the stale label Mar 2, 2024

kfaraz removed the stale label Mar 4, 2024

github-actions Bot added the stale label May 4, 2024

kfaraz removed the stale label May 7, 2024

github-actions Bot added the stale label Jul 7, 2024

clintropolis removed the stale label Jul 22, 2024

github-actions Bot added the stale label Nov 2, 2024

clintropolis removed the stale label Nov 5, 2024

github-actions Bot added the stale label Jan 5, 2025

github-actions Bot closed this Feb 3, 2025

clintropolis mentioned this pull request Feb 20, 2025

refactor CompactionState since 'core' and 'processing' modules have been merged #17741

Merged

Conversation

clintropolis commented Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clintropolis commented Aug 31, 2023

Uh oh!

kfaraz commented Aug 31, 2023

Uh oh!

kfaraz Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

kfaraz Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

clintropolis Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

kfaraz Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

kfaraz Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

clintropolis Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Mar 2, 2024

Uh oh!

kfaraz commented Mar 4, 2024

Uh oh!

github-actions Bot commented May 4, 2024

Uh oh!

github-actions Bot commented Jul 7, 2024

Uh oh!

kfaraz commented Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Nov 2, 2024

Uh oh!

github-actions Bot commented Jan 5, 2025

Uh oh!

github-actions Bot commented Feb 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clintropolis commented Aug 31, 2023 •

edited

Loading

kfaraz commented Sep 2, 2024 •

edited

Loading