You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "clintropolis (via GitHub)" <gi...@apache.org> on 2025/03/14 22:37:47 UTC
[PR] projections + compaction + catalog + msq (druid)
clintropolis opened a new pull request, #17803:
URL: https://github.com/apache/druid/pull/17803
### Description
changes:
* `CompactionTask` now accepts a `projections` property which will cause classic and MSQ auto-compaction to build segments with projections
* `DataSourceCompactionConfig` has been turned into an interface, with the existing implementation renamed to `InlineSchemaDataSourceCompactionConfig`
* Added projections list to `InlineSchemaDataSourceCompactionConfig` to allow explicitly defining projections in an inline schema compaction spec
* if not explicitly defined, compaction tasks will now preserve existing projections when processing segments, combining all named projections across the segments being processed - different projections with the same name are not checked for equivalence, rather one will be chosen dependent on segment processing order.
* Added ability to define projections as a property of a datasource in the catalog
* If projections are defined in a catalog, they will be automatically used by MSQ insert and replace queries
* Added new experimental `CatalogDataSourceCompactionConfig` which allows populating much of a `CompactionTask` using information stored in the catalog. Currently this has some feature gaps compared to `InlineSchemaDataSourceCompactionConfig`, but will be improved in follow-up work to eventually become much more powerful than what can be expressed via a `InlineSchemaDataSourceCompactionConfig`
* Moved `MetadataCatalog` to druid-server from the catalog extension
* Added method to get `MetadataCatalog` from `CatalogResolver`
* Added `CatalogCoreModule` to provide a null binding for `MetadataCatalog`, overridden if the catalog extension is loaded
* Overlord added as a watcher for catalog like the Broker so that it can have `CatalogResolver` and `MetadataCatalog` available
* Added binding for `MetadataCatalog` to Coordinator to have `MetadataCatalg` available
#### Release note
_todo_
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] 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](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] projections + compaction + catalog + msq (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17803:
URL: https://github.com/apache/druid/pull/17803#discussion_r2022407944
##########
server/src/main/java/org/apache/druid/client/indexing/ClientCompactionTaskQuery.java:
##########
@@ -139,6 +145,12 @@ public Map<String, Object> getContext()
return context;
}
+ @JsonProperty("projections")
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] projections + compaction + catalog + msq (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17803:
URL: https://github.com/apache/druid/pull/17803#discussion_r2022395900
##########
extensions-core/multi-stage-query/pom.xml:
##########
@@ -328,6 +328,12 @@
<type>test-jar</type>
<scope>test</scope>
</dependency>
+ <dependency>
Review Comment:
oops, :+1:
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskQueryMaker.java:
##########
@@ -253,12 +259,29 @@ public static MSQSpec makeQuerySpec(
fieldMapping.stream().map(Entry::getValue).collect(Collectors.toSet())
);
+ final List<AggregateProjectionSpec> projectionSpecs;
Review Comment:
done, also split out some other methods because yeah this was a lot
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] projections + compaction + catalog + msq (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17803:
URL: https://github.com/apache/druid/pull/17803#discussion_r2022697278
##########
server/src/main/java/org/apache/druid/server/coordinator/DataSourceCompactionConfig.java:
##########
@@ -19,351 +19,74 @@
package org.apache.druid.server.coordinator;
-import com.fasterxml.jackson.annotation.JsonCreator;
-import com.fasterxml.jackson.annotation.JsonIgnore;
-import com.fasterxml.jackson.annotation.JsonProperty;
-import com.google.common.base.Preconditions;
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import org.apache.druid.data.input.impl.AggregateProjectionSpec;
import org.apache.druid.indexer.CompactionEngine;
import org.apache.druid.java.util.common.granularity.Granularity;
import org.apache.druid.query.aggregation.AggregatorFactory;
import org.apache.druid.segment.transform.CompactionTransformSpec;
import org.joda.time.Period;
import javax.annotation.Nullable;
-import java.util.Arrays;
+import java.util.List;
import java.util.Map;
-import java.util.Objects;
-public class DataSourceCompactionConfig
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl = InlineSchemaDataSourceCompactionConfig.class)
Review Comment:
That would be ideal, although it doesn't necessarily need to be done in this PR. If you do a custom serde it would be nice if it was generic enough to apply to other interfaces. (I would love to strike `defaultImpl` from the codebase.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] projections + compaction + catalog + msq (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17803:
URL: https://github.com/apache/druid/pull/17803#discussion_r2022694677
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java:
##########
@@ -1086,6 +1130,60 @@ private void processMultiValuedDimensions(final QueryableIndex index)
}
}
+
+ private void processProjections(final QueryableIndex index)
Review Comment:
I'm not sure it matters much. I don't think there would be a serious consequence to picking the "wrong" projection when there is a conflict. I think the current logic is ok, I just wanted to see a description of it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] projections + compaction + catalog + msq (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17803:
URL: https://github.com/apache/druid/pull/17803#discussion_r2022407512
##########
server/src/main/java/org/apache/druid/catalog/model/ResolvedTable.java:
##########
@@ -84,6 +84,15 @@ public Map<String, Object> properties()
return spec.properties();
}
+ public <T> T getProperty(String key)
Review Comment:
fair, renamed to `decodeProperty` which i think resolves the problem
##########
server/src/main/java/org/apache/druid/catalog/model/table/DatasourceDefn.java:
##########
@@ -70,13 +73,48 @@ public class DatasourceDefn extends TableDefn
public static final String TABLE_TYPE = "datasource";
+ public DatasourceDefn()
+ {
+ super(
+ "Datasource",
+ TABLE_TYPE,
+ Arrays.asList(
+ new SegmentGranularityFieldDefn(),
+ new ModelProperties.IntPropertyDefn(TARGET_SEGMENT_ROWS_PROPERTY),
+ new ClusterKeysDefn(),
+ new HiddenColumnsDefn(),
+ new ModelProperties.BooleanPropertyDefn(SEALED_PROPERTY),
+ new ProjectionsDefn()
+ ),
+ null
+ );
+ }
+
+ @Override
+ protected void validateColumn(ColumnSpec spec)
+ {
+ super.validateColumn(spec);
+ if (Columns.isTimeColumn(spec.name()) && spec.dataType() != null) {
+ // Validate type in next PR
+ }
+ }
+
+ public static boolean isDatasource(String tableType)
Review Comment:
i just shuffled some stuff around, this isn't a new method, but agree it could use some javadocs. This method only seems to be used by `TableEditor`, and also is always inverted (it is validating that catalog hidden column modifications are only applied to 'datasource' typed specs. I'll just add a note for now that the expected strings arguments are from `TableSpec.type`, but it does seem like this could be reworked to do this differently (same with the instanceOf version right below it)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] projections + compaction + catalog + msq (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17803:
URL: https://github.com/apache/druid/pull/17803#discussion_r2022100109
##########
extensions-core/druid-catalog/src/main/java/org/apache/druid/catalog/guice/CatalogClientModule.java:
##########
@@ -20,55 +20,59 @@
package org.apache.druid.catalog.guice;
import com.google.inject.Binder;
+import org.apache.druid.catalog.MetadataCatalog;
import org.apache.druid.catalog.http.CatalogListenerResource;
import org.apache.druid.catalog.model.SchemaRegistry;
import org.apache.druid.catalog.model.SchemaRegistryImpl;
import org.apache.druid.catalog.sql.LiveCatalogResolver;
import org.apache.druid.catalog.sync.CachedMetadataCatalog;
import org.apache.druid.catalog.sync.CatalogClient;
+import org.apache.druid.catalog.sync.CatalogClientConfig;
+import org.apache.druid.catalog.sync.CatalogSource;
import org.apache.druid.catalog.sync.CatalogUpdateListener;
import org.apache.druid.catalog.sync.CatalogUpdateReceiver;
-import org.apache.druid.catalog.sync.MetadataCatalog;
-import org.apache.druid.catalog.sync.MetadataCatalog.CatalogSource;
import org.apache.druid.discovery.NodeRole;
import org.apache.druid.guice.Jerseys;
+import org.apache.druid.guice.JsonConfigProvider;
import org.apache.druid.guice.LazySingleton;
import org.apache.druid.guice.LifecycleModule;
import org.apache.druid.guice.ManageLifecycle;
+import org.apache.druid.guice.annotations.ExcludeScope;
import org.apache.druid.guice.annotations.LoadScope;
import org.apache.druid.initialization.DruidModule;
import org.apache.druid.sql.calcite.planner.CatalogResolver;
/**
- * Configures the metadata catalog on the Broker to use a cache
+ * Configures the metadata catalog on the Broker and Overlord to use a cache
* and network communications for pull and push updates.
*/
-@LoadScope(roles = NodeRole.BROKER_JSON_NAME)
-public class CatalogBrokerModule implements DruidModule
+@LoadScope(roles = {NodeRole.BROKER_JSON_NAME, NodeRole.OVERLORD_JSON_NAME})
+@ExcludeScope(roles = {NodeRole.COORDINATOR_JSON_NAME})
Review Comment:
I see that we need it because there's a conflict between this module and `CatalogCoordinatorModule`. Please add a javadoc reference to `CatalogCoordinatorModule` pointing out that the exclusion is needed because we can't load both in the same process.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] projections + compaction + catalog + msq (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis merged PR #17803:
URL: https://github.com/apache/druid/pull/17803
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] projections + compaction + catalog + msq (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #17803:
URL: https://github.com/apache/druid/pull/17803#discussion_r1996404820
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQInsertTest.java:
##########
@@ -466,6 +515,105 @@
}
+ @MethodSource("data")
+ @ParameterizedTest(name = "{index}:with context {0}")
+ public void testInsertOnExternalDataSourceWithCatalogProjections(String contextName, Map<String, Object> context) throws IOException
Review Comment:
## Useless parameter
The parameter 'contextName' is never used.
[Show more details](https://github.com/apache/druid/security/code-scanning/8721)
##########
server/src/test/java/org/apache/druid/server/compaction/CompactionStatusTest.java:
##########
@@ -105,41 +106,34 @@
@Test
public void testFindPartitionsSpecFromConfigWithDeprecatedMaxRowsPerSegmentAndMaxTotalRowsReturnGivenValues()
{
- final DataSourceCompactionConfig config = new DataSourceCompactionConfig(
- "datasource",
- null,
- null,
- 100,
- null,
- new UserCompactionTaskQueryTuningConfig(
- null,
- null,
- null,
- 1000L,
- null,
- null,
- null,
- null,
- null,
- null,
- null,
- null,
- null,
- null,
- null,
- null,
- null,
- null,
- null
- ),
- null,
- null,
- null,
- null,
- null,
- null,
- null
- );
+ final DataSourceCompactionConfig config =
+ InlineSchemaDataSourceCompactionConfig.builder()
+ .forDataSource("datasource")
+ .withMaxRowsPerSegment(100)
Review Comment:
## Deprecated method or constructor invocation
Invoking [Builder.withMaxRowsPerSegment](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/8724)
##########
server/src/test/java/org/apache/druid/client/indexing/ClientCompactionRunnerInfoTest.java:
##########
@@ -274,21 +275,18 @@
List<DimensionSchema> dimensions
)
{
- return new DataSourceCompactionConfig(
- "dataSource",
- null,
- 500L,
- 10000,
- new Period(3600),
- createTuningConfig(partitionsSpec),
- granularitySpec,
- new UserCompactionTaskDimensionsConfig(dimensions),
- metricsSpec,
- null,
- null,
- CompactionEngine.MSQ,
- context
- );
+ return InlineSchemaDataSourceCompactionConfig.builder()
+ .forDataSource("dataSource")
+ .withInputSegmentSizeBytes(500L)
+ .withMaxRowsPerSegment(10000)
Review Comment:
## Deprecated method or constructor invocation
Invoking [Builder.withMaxRowsPerSegment](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/8723)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] projections + compaction + catalog + msq (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17803:
URL: https://github.com/apache/druid/pull/17803#discussion_r2019184490
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskQueryMaker.java:
##########
@@ -253,12 +259,29 @@ public static MSQSpec makeQuerySpec(
fieldMapping.stream().map(Entry::getValue).collect(Collectors.toSet())
);
+ final List<AggregateProjectionSpec> projectionSpecs;
Review Comment:
break this out into its own method please; the current function is long enough as it is.
##########
extensions-core/multi-stage-query/pom.xml:
##########
@@ -328,6 +328,12 @@
<type>test-jar</type>
<scope>test</scope>
</dependency>
+ <dependency>
Review Comment:
indentation seems off here
##########
server/src/main/java/org/apache/druid/client/indexing/ClientCompactionTaskQuery.java:
##########
@@ -139,6 +145,12 @@ public Map<String, Object> getContext()
return context;
}
+ @JsonProperty("projections")
Review Comment:
It can be null, so how about adding `@Nullable` and also json-include only if nonnull?
##########
processing/src/main/java/org/apache/druid/timeline/CompactionState.java:
##########
@@ -108,6 +112,12 @@ public GranularitySpec getGranularitySpec()
return granularitySpec;
}
+ @JsonProperty
Review Comment:
Should this be nullable? Are null and empty meaningfully different & can both happen?
##########
server/src/main/java/org/apache/druid/server/coordinator/DataSourceCompactionConfig.java:
##########
@@ -19,351 +19,74 @@
package org.apache.druid.server.coordinator;
-import com.fasterxml.jackson.annotation.JsonCreator;
-import com.fasterxml.jackson.annotation.JsonIgnore;
-import com.fasterxml.jackson.annotation.JsonProperty;
-import com.google.common.base.Preconditions;
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import org.apache.druid.data.input.impl.AggregateProjectionSpec;
import org.apache.druid.indexer.CompactionEngine;
import org.apache.druid.java.util.common.granularity.Granularity;
import org.apache.druid.query.aggregation.AggregatorFactory;
import org.apache.druid.segment.transform.CompactionTransformSpec;
import org.joda.time.Period;
import javax.annotation.Nullable;
-import java.util.Arrays;
+import java.util.List;
import java.util.Map;
-import java.util.Objects;
-public class DataSourceCompactionConfig
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl = InlineSchemaDataSourceCompactionConfig.class)
Review Comment:
Do we need the `defaultImpl` for backwards-compat reasons?
We should avoid it whenever possible, because it does this weird thing where if you provide an invalid type, rather than being an error, it's mapped to the `defaultImpl`. It tends to burn people that mistype things, forget to load extensions, etc.
##########
server/src/main/java/org/apache/druid/catalog/model/table/DatasourceDefn.java:
##########
@@ -70,13 +73,48 @@ public class DatasourceDefn extends TableDefn
public static final String TABLE_TYPE = "datasource";
+ public DatasourceDefn()
+ {
+ super(
+ "Datasource",
+ TABLE_TYPE,
+ Arrays.asList(
+ new SegmentGranularityFieldDefn(),
+ new ModelProperties.IntPropertyDefn(TARGET_SEGMENT_ROWS_PROPERTY),
+ new ClusterKeysDefn(),
+ new HiddenColumnsDefn(),
+ new ModelProperties.BooleanPropertyDefn(SEALED_PROPERTY),
+ new ProjectionsDefn()
+ ),
+ null
+ );
+ }
+
+ @Override
+ protected void validateColumn(ColumnSpec spec)
+ {
+ super.validateColumn(spec);
+ if (Columns.isTimeColumn(spec.name()) && spec.dataType() != null) {
+ // Validate type in next PR
+ }
+ }
+
+ public static boolean isDatasource(String tableType)
Review Comment:
Would be nice to have javadoc about what `tableType` is meant to be. Like, what kinds of types can be put in here? Perhaps it's obvious to people that are more familiar with the catalog than I am. But, to me, it's not obvious.
##########
extensions-core/druid-catalog/src/main/java/org/apache/druid/catalog/guice/CatalogClientModule.java:
##########
@@ -20,55 +20,59 @@
package org.apache.druid.catalog.guice;
import com.google.inject.Binder;
+import org.apache.druid.catalog.MetadataCatalog;
import org.apache.druid.catalog.http.CatalogListenerResource;
import org.apache.druid.catalog.model.SchemaRegistry;
import org.apache.druid.catalog.model.SchemaRegistryImpl;
import org.apache.druid.catalog.sql.LiveCatalogResolver;
import org.apache.druid.catalog.sync.CachedMetadataCatalog;
import org.apache.druid.catalog.sync.CatalogClient;
+import org.apache.druid.catalog.sync.CatalogClientConfig;
+import org.apache.druid.catalog.sync.CatalogSource;
import org.apache.druid.catalog.sync.CatalogUpdateListener;
import org.apache.druid.catalog.sync.CatalogUpdateReceiver;
-import org.apache.druid.catalog.sync.MetadataCatalog;
-import org.apache.druid.catalog.sync.MetadataCatalog.CatalogSource;
import org.apache.druid.discovery.NodeRole;
import org.apache.druid.guice.Jerseys;
+import org.apache.druid.guice.JsonConfigProvider;
import org.apache.druid.guice.LazySingleton;
import org.apache.druid.guice.LifecycleModule;
import org.apache.druid.guice.ManageLifecycle;
+import org.apache.druid.guice.annotations.ExcludeScope;
import org.apache.druid.guice.annotations.LoadScope;
import org.apache.druid.initialization.DruidModule;
import org.apache.druid.sql.calcite.planner.CatalogResolver;
/**
- * Configures the metadata catalog on the Broker to use a cache
+ * Configures the metadata catalog on the Broker and Overlord to use a cache
* and network communications for pull and push updates.
*/
-@LoadScope(roles = NodeRole.BROKER_JSON_NAME)
-public class CatalogBrokerModule implements DruidModule
+@LoadScope(roles = {NodeRole.BROKER_JSON_NAME, NodeRole.OVERLORD_JSON_NAME})
+@ExcludeScope(roles = {NodeRole.COORDINATOR_JSON_NAME})
Review Comment:
Why do we need both `@LoadScope` and `@ExcludeScope`? Isn't _not_ listing `COORDINATOR_JSON_NAME` in `@LoadScope` enough to not load it there?
##########
server/src/main/java/org/apache/druid/catalog/model/ResolvedTable.java:
##########
@@ -84,6 +84,15 @@ public Map<String, Object> properties()
return spec.properties();
}
+ public <T> T getProperty(String key)
Review Comment:
`@Nullable`? Also, the fact that decoding is automatic seems interesting and should be mentioned in the javadoc. Otherwise it's not clear why `getProperty` is different from calling `properties()` and doing a `get`.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java:
##########
@@ -1086,6 +1130,60 @@ private void processMultiValuedDimensions(final QueryableIndex index)
}
}
+
+ private void processProjections(final QueryableIndex index)
Review Comment:
some comments here about the logic would be useful, especially how conflicts are handled between projections of different names.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] projections + compaction + catalog + msq (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17803:
URL: https://github.com/apache/druid/pull/17803#discussion_r2022411213
##########
extensions-core/druid-catalog/src/main/java/org/apache/druid/catalog/guice/CatalogClientModule.java:
##########
@@ -20,55 +20,59 @@
package org.apache.druid.catalog.guice;
import com.google.inject.Binder;
+import org.apache.druid.catalog.MetadataCatalog;
import org.apache.druid.catalog.http.CatalogListenerResource;
import org.apache.druid.catalog.model.SchemaRegistry;
import org.apache.druid.catalog.model.SchemaRegistryImpl;
import org.apache.druid.catalog.sql.LiveCatalogResolver;
import org.apache.druid.catalog.sync.CachedMetadataCatalog;
import org.apache.druid.catalog.sync.CatalogClient;
+import org.apache.druid.catalog.sync.CatalogClientConfig;
+import org.apache.druid.catalog.sync.CatalogSource;
import org.apache.druid.catalog.sync.CatalogUpdateListener;
import org.apache.druid.catalog.sync.CatalogUpdateReceiver;
-import org.apache.druid.catalog.sync.MetadataCatalog;
-import org.apache.druid.catalog.sync.MetadataCatalog.CatalogSource;
import org.apache.druid.discovery.NodeRole;
import org.apache.druid.guice.Jerseys;
+import org.apache.druid.guice.JsonConfigProvider;
import org.apache.druid.guice.LazySingleton;
import org.apache.druid.guice.LifecycleModule;
import org.apache.druid.guice.ManageLifecycle;
+import org.apache.druid.guice.annotations.ExcludeScope;
import org.apache.druid.guice.annotations.LoadScope;
import org.apache.druid.initialization.DruidModule;
import org.apache.druid.sql.calcite.planner.CatalogResolver;
/**
- * Configures the metadata catalog on the Broker to use a cache
+ * Configures the metadata catalog on the Broker and Overlord to use a cache
* and network communications for pull and push updates.
*/
-@LoadScope(roles = NodeRole.BROKER_JSON_NAME)
-public class CatalogBrokerModule implements DruidModule
+@LoadScope(roles = {NodeRole.BROKER_JSON_NAME, NodeRole.OVERLORD_JSON_NAME})
+@ExcludeScope(roles = {NodeRole.COORDINATOR_JSON_NAME})
Review Comment:
added javadocs to link module and updated `LoadScope`/`ExcludeScope` javadoc to indicate that `ExcludeScope` takes priority if both are defined
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] projections + compaction + catalog + msq (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17803:
URL: https://github.com/apache/druid/pull/17803#discussion_r2022402999
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java:
##########
@@ -1086,6 +1130,60 @@ private void processMultiValuedDimensions(final QueryableIndex index)
}
}
+
+ private void processProjections(final QueryableIndex index)
Review Comment:
added.
This got me thinking a bit though about what behavior would be better between using the first encountered vs the last encountered? Should it warn if there is a mismatch? Right now it is using last encountered, but i didn't give it a lot of thought, since the main goal for this discovery based stuff was for compaction to not automatically wipe out projections by default. My preference is that this will all be driven via the catalog instead of relying on discovery from segments, but this at least covers that case where the setup was not done (or not explicitly specified in the inline schema compaction spec).
##########
processing/src/main/java/org/apache/druid/timeline/CompactionState.java:
##########
@@ -108,6 +112,12 @@ public GranularitySpec getGranularitySpec()
return granularitySpec;
}
+ @JsonProperty
Review Comment:
not meaningfully different in the sense that neither will result in projections being built into the segment. I modified to coerce to empty so that they are considered equivalent for comparison.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] projections + compaction + catalog + msq (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17803:
URL: https://github.com/apache/druid/pull/17803#discussion_r2022404819
##########
processing/src/main/java/org/apache/druid/timeline/CompactionState.java:
##########
@@ -108,6 +112,12 @@ public GranularitySpec getGranularitySpec()
return granularitySpec;
}
+ @JsonProperty
Review Comment:
Oh wait, i think the modification wasn't the right thing to do since it will not compare correctly, reverting that and marking nullable for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] projections + compaction + catalog + msq (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17803:
URL: https://github.com/apache/druid/pull/17803#discussion_r2022409762
##########
server/src/main/java/org/apache/druid/server/coordinator/DataSourceCompactionConfig.java:
##########
@@ -19,351 +19,74 @@
package org.apache.druid.server.coordinator;
-import com.fasterxml.jackson.annotation.JsonCreator;
-import com.fasterxml.jackson.annotation.JsonIgnore;
-import com.fasterxml.jackson.annotation.JsonProperty;
-import com.google.common.base.Preconditions;
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import org.apache.druid.data.input.impl.AggregateProjectionSpec;
import org.apache.druid.indexer.CompactionEngine;
import org.apache.druid.java.util.common.granularity.Granularity;
import org.apache.druid.query.aggregation.AggregatorFactory;
import org.apache.druid.segment.transform.CompactionTransformSpec;
import org.joda.time.Period;
import javax.annotation.Nullable;
-import java.util.Arrays;
+import java.util.List;
import java.util.Map;
-import java.util.Objects;
-public class DataSourceCompactionConfig
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl = InlineSchemaDataSourceCompactionConfig.class)
Review Comment:
yea, it is for backwards compatibility. Looking into a custom serde to see if we can better restrict it to only allow coercion where type is missing instead of cases where it is incorrect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org