cecemei commented on code in PR #18524:
URL: https://github.com/apache/druid/pull/18524#discussion_r2353741363
##########
server/src/main/java/org/apache/druid/segment/indexing/DataSchema.java:
##########
@@ -415,9 +415,73 @@ private static Set<String>
computeAndValidateOutputFieldNames(
}
}
+ return getFieldsOrThrowIfErrors(fields);
+ }
+
+ public static void validateProjections(
+ @Nullable List<AggregateProjectionSpec> projections,
+ @Nullable Granularity segmentGranularity
+ )
+ {
+ if (projections != null) {
+ final Set<String> names =
Sets.newHashSetWithExpectedSize(projections.size());
+ for (AggregateProjectionSpec projection : projections) {
+ if (names.contains(projection.getName())) {
+ throw InvalidInput.exception("projection[%s] is already defined,
projection names must be unique", projection.getName());
+ }
+ names.add(projection.getName());
+ final AggregateProjectionMetadata.Schema schema =
projection.toMetadataSchema();
+
+ if (schema.getTimeColumnName() != null) {
+ final Granularity projectionGranularity =
schema.getEffectiveGranularity();
+ if (segmentGranularity != null) {
+ if (segmentGranularity.isFinerThan(projectionGranularity)) {
+ throw InvalidInput.exception(
+ "projection[%s] has granularity[%s] which must be finer than
or equal to segment granularity[%s]",
+ projection.getName(),
+ projectionGranularity,
+ segmentGranularity
+ );
+ }
+ }
+ }
+
+ final Map<String, Multiset<String>> fields = new TreeMap<>();
Review Comment:
nit: maybe `ImmutableMultimap.Builder` is more suitable?
##########
processing/src/test/java/org/apache/druid/data/input/impl/AggregateProjectionSpecTest.java:
##########
@@ -112,9 +113,15 @@ void testComputeOrdering_granularity()
ColumnType.LONG,
TestExprMacroTable.INSTANCE
);
+ ExpressionVirtualColumn ptEvery10MinLA = new ExpressionVirtualColumn(
+ "ptEvery10MinLA",
+ "timestamp_floor(__time, 'PT10M', null, 'America/Los_Angeles')",
+ ColumnType.LONG,
+ TestExprMacroTable.INSTANCE
+ );
ExpressionVirtualColumn ptEvery10Min = new ExpressionVirtualColumn(
Review Comment:
since this is not longer pacific time zone, we can just rename it to
every10Min
##########
server/src/main/java/org/apache/druid/segment/indexing/DataSchema.java:
##########
@@ -415,9 +415,73 @@ private static Set<String>
computeAndValidateOutputFieldNames(
}
}
+ return getFieldsOrThrowIfErrors(fields);
+ }
+
+ public static void validateProjections(
+ @Nullable List<AggregateProjectionSpec> projections,
+ @Nullable Granularity segmentGranularity
+ )
+ {
+ if (projections != null) {
+ final Set<String> names =
Sets.newHashSetWithExpectedSize(projections.size());
+ for (AggregateProjectionSpec projection : projections) {
+ if (names.contains(projection.getName())) {
+ throw InvalidInput.exception("projection[%s] is already defined,
projection names must be unique", projection.getName());
+ }
+ names.add(projection.getName());
+ final AggregateProjectionMetadata.Schema schema =
projection.toMetadataSchema();
+
+ if (schema.getTimeColumnName() != null) {
+ final Granularity projectionGranularity =
schema.getEffectiveGranularity();
+ if (segmentGranularity != null) {
+ if (segmentGranularity.isFinerThan(projectionGranularity)) {
+ throw InvalidInput.exception(
+ "projection[%s] has granularity[%s] which must be finer than
or equal to segment granularity[%s]",
+ projection.getName(),
+ projectionGranularity,
+ segmentGranularity
+ );
+ }
+ }
+ }
+
+ final Map<String, Multiset<String>> fields = new TreeMap<>();
+ int position = 0;
+ for (DimensionSchema grouping : projection.getGroupingColumns()) {
+ final String field = grouping.getName();
+ if (Strings.isNullOrEmpty(field)) {
+ throw DruidException
+ .forPersona(DruidException.Persona.USER)
+ .ofCategory(DruidException.Category.INVALID_INPUT)
+ .build("Encountered grouping column with null or empty name at
position[%d]", position);
+ }
+ fields.computeIfAbsent(field, k ->
TreeMultiset.create()).add("projection[" + projection.getName() + "] grouping
column list");
+ position++;
+ }
+ for (AggregatorFactory aggregator : projection.getAggregators()) {
+ final String field = aggregator.getName();
+ if (Strings.isNullOrEmpty(field)) {
+ throw DruidException
+ .forPersona(DruidException.Persona.USER)
+ .ofCategory(DruidException.Category.INVALID_INPUT)
+ .build("Encountered aggregator with null or empty name at
position[%d]", position);
+ }
+
+ fields.computeIfAbsent(field, k ->
TreeMultiset.create()).add("projection[" + projection.getName() + "]
aggregators list");
+ position++;
+ }
+
+ getFieldsOrThrowIfErrors(fields);
+ }
+ }
+ }
+
+ private static Set<String> getFieldsOrThrowIfErrors(Map<String,
Multiset<String>> validatedFields)
+ {
final List<String> errors = new ArrayList<>();
- for (Map.Entry<String, Multiset<String>> fieldEntry : fields.entrySet()) {
+ for (Map.Entry<String, Multiset<String>> fieldEntry :
validatedFields.entrySet()) {
Review Comment:
in line 495, entry.getCount() should always > 1 right?
##########
server/src/main/java/org/apache/druid/segment/indexing/DataSchema.java:
##########
@@ -415,9 +415,73 @@ private static Set<String>
computeAndValidateOutputFieldNames(
}
}
+ return getFieldsOrThrowIfErrors(fields);
+ }
+
+ public static void validateProjections(
+ @Nullable List<AggregateProjectionSpec> projections,
+ @Nullable Granularity segmentGranularity
+ )
+ {
+ if (projections != null) {
+ final Set<String> names =
Sets.newHashSetWithExpectedSize(projections.size());
+ for (AggregateProjectionSpec projection : projections) {
+ if (names.contains(projection.getName())) {
+ throw InvalidInput.exception("projection[%s] is already defined,
projection names must be unique", projection.getName());
+ }
+ names.add(projection.getName());
+ final AggregateProjectionMetadata.Schema schema =
projection.toMetadataSchema();
+
+ if (schema.getTimeColumnName() != null) {
+ final Granularity projectionGranularity =
schema.getEffectiveGranularity();
+ if (segmentGranularity != null) {
+ if (segmentGranularity.isFinerThan(projectionGranularity)) {
+ throw InvalidInput.exception(
+ "projection[%s] has granularity[%s] which must be finer than
or equal to segment granularity[%s]",
+ projection.getName(),
+ projectionGranularity,
+ segmentGranularity
+ );
+ }
+ }
+ }
+
+ final Map<String, Multiset<String>> fields = new TreeMap<>();
+ int position = 0;
+ for (DimensionSchema grouping : projection.getGroupingColumns()) {
+ final String field = grouping.getName();
+ if (Strings.isNullOrEmpty(field)) {
+ throw DruidException
+ .forPersona(DruidException.Persona.USER)
+ .ofCategory(DruidException.Category.INVALID_INPUT)
+ .build("Encountered grouping column with null or empty name at
position[%d]", position);
+ }
+ fields.computeIfAbsent(field, k ->
TreeMultiset.create()).add("projection[" + projection.getName() + "] grouping
column list");
Review Comment:
maybe worth use separate map for dimension and aggregator?
##########
server/src/main/java/org/apache/druid/segment/indexing/DataSchema.java:
##########
@@ -415,9 +415,73 @@ private static Set<String>
computeAndValidateOutputFieldNames(
}
}
+ return getFieldsOrThrowIfErrors(fields);
+ }
+
+ public static void validateProjections(
+ @Nullable List<AggregateProjectionSpec> projections,
+ @Nullable Granularity segmentGranularity
+ )
+ {
+ if (projections != null) {
+ final Set<String> names =
Sets.newHashSetWithExpectedSize(projections.size());
+ for (AggregateProjectionSpec projection : projections) {
+ if (names.contains(projection.getName())) {
+ throw InvalidInput.exception("projection[%s] is already defined,
projection names must be unique", projection.getName());
+ }
+ names.add(projection.getName());
+ final AggregateProjectionMetadata.Schema schema =
projection.toMetadataSchema();
+
+ if (schema.getTimeColumnName() != null) {
+ final Granularity projectionGranularity =
schema.getEffectiveGranularity();
+ if (segmentGranularity != null) {
+ if (segmentGranularity.isFinerThan(projectionGranularity)) {
+ throw InvalidInput.exception(
+ "projection[%s] has granularity[%s] which must be finer than
or equal to segment granularity[%s]",
+ projection.getName(),
+ projectionGranularity,
+ segmentGranularity
+ );
+ }
+ }
+ }
+
+ final Map<String, Multiset<String>> fields = new TreeMap<>();
+ int position = 0;
+ for (DimensionSchema grouping : projection.getGroupingColumns()) {
+ final String field = grouping.getName();
+ if (Strings.isNullOrEmpty(field)) {
+ throw DruidException
+ .forPersona(DruidException.Persona.USER)
+ .ofCategory(DruidException.Category.INVALID_INPUT)
+ .build("Encountered grouping column with null or empty name at
position[%d]", position);
+ }
+ fields.computeIfAbsent(field, k ->
TreeMultiset.create()).add("projection[" + projection.getName() + "] grouping
column list");
+ position++;
+ }
+ for (AggregatorFactory aggregator : projection.getAggregators()) {
+ final String field = aggregator.getName();
+ if (Strings.isNullOrEmpty(field)) {
+ throw DruidException
+ .forPersona(DruidException.Persona.USER)
+ .ofCategory(DruidException.Category.INVALID_INPUT)
+ .build("Encountered aggregator with null or empty name at
position[%d]", position);
+ }
+
+ fields.computeIfAbsent(field, k ->
TreeMultiset.create()).add("projection[" + projection.getName() + "]
aggregators list");
+ position++;
+ }
+
+ getFieldsOrThrowIfErrors(fields);
Review Comment:
the result is not used? maybe just `validateFieldsOrThrow`
##########
server/src/main/java/org/apache/druid/segment/indexing/DataSchema.java:
##########
@@ -415,9 +415,73 @@ private static Set<String>
computeAndValidateOutputFieldNames(
}
}
+ return getFieldsOrThrowIfErrors(fields);
+ }
+
+ public static void validateProjections(
Review Comment:
maybe put a brief javadoc explain it validates:
1. segment granularity has to be at least as coarse as projection
granularity.
2. no duplicated grouping column or aggregator 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
